Not automatically create citus_columnar when creating citus extension

.github/workflows/build_and_test.yml

@@ -331,7 +331,15 @@ jobs:
           make -C src/test/regress \
             check-pg-upgrade \
             old-bindir=/usr/lib/postgresql/${{ env.old_pg_major }}/bin \
-            new-bindir=/usr/lib/postgresql/${{ env.new_pg_major }}/bin
+            new-bindir=/usr/lib/postgresql/${{ env.new_pg_major }}/bin \
+            test-with-columnar=false
+
+        gosu circleci \
+          make -C src/test/regress \
+            check-pg-upgrade \
+            old-bindir=/usr/lib/postgresql/${{ env.old_pg_major }}/bin \
+            new-bindir=/usr/lib/postgresql/${{ env.new_pg_major }}/bin \
+            test-with-columnar=true
     - name: Copy pg_upgrade logs for newData dir
       run: |-
         mkdir -p /tmp/pg_upgrade_newData_logs

src/backend/columnar/citus_columnar.control

@@ -1,6 +1,6 @@
 # Columnar extension
 comment = 'Citus Columnar extension'
-default_version = '12.2-1'
+default_version = '13.2-1'
 module_pathname = '$libdir/citus_columnar'
 relocatable = false
 schema = pg_catalog

src/backend/columnar/sql/citus_columnar--12.2-1--13.2-1.sql

@@ -0,0 +1,3 @@
+-- citus_columnar--12.2-1--13.2-1.sql
+
+#include "udfs/columnar_finish_pg_upgrade/13.2-1.sql"

src/backend/columnar/sql/downgrades/citus_columnar--13.2-1--12.2-1.sql

@@ -0,0 +1,3 @@
+-- citus_columnar--13.2-1--12.2-1.sql
+
+DROP FUNCTION IF EXISTS pg_catalog.columnar_finish_pg_upgrade();

src/backend/columnar/sql/udfs/columnar_finish_pg_upgrade/13.2-1.sql

@@ -0,0 +1,13 @@
+CREATE OR REPLACE FUNCTION pg_catalog.columnar_finish_pg_upgrade()
+    RETURNS void
+    LANGUAGE plpgsql
+    SET search_path = pg_catalog
+    AS $cppu$
+BEGIN
+    -- set dependencies for columnar table access method
+    PERFORM columnar_internal.columnar_ensure_am_depends_catalog();
+END;
+$cppu$;
+
+COMMENT ON FUNCTION pg_catalog.columnar_finish_pg_upgrade()
+    IS 'perform tasks to properly complete a Postgres upgrade for columnar extension';

src/backend/columnar/sql/udfs/columnar_finish_pg_upgrade/latest.sql

@@ -0,0 +1,13 @@
+CREATE OR REPLACE FUNCTION pg_catalog.columnar_finish_pg_upgrade()
+    RETURNS void
+    LANGUAGE plpgsql
+    SET search_path = pg_catalog
+    AS $cppu$
+BEGIN
+    -- set dependencies for columnar table access method
+    PERFORM columnar_internal.columnar_ensure_am_depends_catalog();
+END;
+$cppu$;
+
+COMMENT ON FUNCTION pg_catalog.columnar_finish_pg_upgrade()
+    IS 'perform tasks to properly complete a Postgres upgrade for columnar extension';

src/backend/distributed/commands/extension.c

@@ -25,6 +25,12 @@
 #include "utils/lsyscache.h"
 #include "utils/syscache.h"
 
+#include "pg_version_constants.h"
+
+#if PG_VERSION_NUM < PG_VERSION_17
+#include "catalog/pg_am_d.h"
+#endif
+
 #include "citus_version.h"
 
 #include "columnar/columnar.h"
@@ -52,6 +58,10 @@ static void MarkExistingObjectDependenciesDistributedIfSupported(void);
 static List * GetAllViews(void);
 static bool ShouldPropagateExtensionCommand(Node *parseTree);
 static bool IsAlterExtensionSetSchemaCitus(Node *parseTree);
+static bool HasAnyRelationsUsingOldColumnar(void);
+static Oid GetOldColumnarAMIdIfExists(void);
+static bool AccessMethodDependsOnAnyExtensions(Oid accessMethodId);
+static bool HasAnyRelationsUsingAccessMethod(Oid accessMethodId);
 static Node * RecreateExtensionStmt(Oid extensionOid);
 static List * GenerateGrantCommandsOnExtensionDependentFDWs(Oid extensionId);
 
@@ -783,7 +793,8 @@ PreprocessCreateExtensionStmtForCitusColumnar(Node *parsetree)
 		/*citus version >= 11.1 requires install citus_columnar first*/
 		if (versionNumber >= 1110 && !CitusHasBeenLoaded())
 		{
-			if (get_extension_oid("citus_columnar", true) == InvalidOid)
+			if (get_extension_oid("citus_columnar", true) == InvalidOid &&
+				(versionNumber < 1320 || HasAnyRelationsUsingOldColumnar()))
 			{
 				CreateExtensionWithVersion("citus_columnar", NULL);
 			}
@@ -894,9 +905,10 @@ PreprocessAlterExtensionCitusStmtForCitusColumnar(Node *parseTree)
 		double newVersionNumber = GetExtensionVersionNumber(pstrdup(newVersion));
 
 		/*alter extension citus update to version >= 11.1-1, and no citus_columnar installed */
-		if (newVersionNumber >= 1110 && citusColumnarOid == InvalidOid)
+		if (newVersionNumber >= 1110 && citusColumnarOid == InvalidOid &&
+			(newVersionNumber < 1320 || HasAnyRelationsUsingOldColumnar()))
 		{
-			/*it's upgrade citus to 11.1-1 or further version */
+			/*it's upgrade citus to 11.1-1 or further version and there are relations using old columnar */
 			CreateExtensionWithVersion("citus_columnar", CITUS_COLUMNAR_INTERNAL_VERSION);
 		}
 		else if (newVersionNumber < 1110 && citusColumnarOid != InvalidOid)
@@ -911,7 +923,8 @@ PreprocessAlterExtensionCitusStmtForCitusColumnar(Node *parseTree)
 		int versionNumber = (int) (100 * strtod(CITUS_MAJORVERSION, NULL));
 		if (versionNumber >= 1110)
 		{
-			if (citusColumnarOid == InvalidOid)
+			if (citusColumnarOid == InvalidOid &&
+				(versionNumber < 1320 || HasAnyRelationsUsingOldColumnar()))
 			{
 				CreateExtensionWithVersion("citus_columnar",
 										   CITUS_COLUMNAR_INTERNAL_VERSION);
@@ -921,6 +934,117 @@ PreprocessAlterExtensionCitusStmtForCitusColumnar(Node *parseTree)
 }
 
 
+/*
+ * HasAnyRelationsUsingOldColumnar returns true if there are any relations
+ * using the old columnar access method.
+ */
+static bool
+HasAnyRelationsUsingOldColumnar(void)
+{
+	Oid oldColumnarAMId = GetOldColumnarAMIdIfExists();
+	return OidIsValid(oldColumnarAMId) &&
+		   HasAnyRelationsUsingAccessMethod(oldColumnarAMId);
+}
+
+
+/*
+ * GetOldColumnarAMIdIfExists returns the oid of the old columnar access
+ * method, i.e., the columnar access method that we had as part of "citus"
+ * extension before we split it into "citus_columnar" at version 11.1, if
+ * it exists. Otherwise, it returns InvalidOid.
+ *
+ * We know that it's "old columnar" only if the access method doesn't depend
+ * on any extensions. This is because, in citus--11.0-4--11.1-1.sql, we
+ * detach the columnar objects (including the access method) from citus
+ * in preparation for splitting of the columnar into a separate extension.
+ */
+static Oid
+GetOldColumnarAMIdIfExists(void)
+{
+	Oid columnarAMId = get_am_oid("columnar", true);
+	if (OidIsValid(columnarAMId) && !AccessMethodDependsOnAnyExtensions(columnarAMId))
+	{
+		return columnarAMId;
+	}
+
+	return InvalidOid;
+}
+
+
+/*
+ * AccessMethodDependsOnAnyExtensions returns true if the access method
+ * with the given accessMethodId depends on any extensions.
+ */
+static bool
+AccessMethodDependsOnAnyExtensions(Oid accessMethodId)
+{
+	ScanKeyData key[3];
+
+	Relation pgDepend = table_open(DependRelationId, AccessShareLock);
+
+	ScanKeyInit(&key[0],
+				Anum_pg_depend_classid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(AccessMethodRelationId));
+	ScanKeyInit(&key[1],
+				Anum_pg_depend_objid,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(accessMethodId));
+
+	ScanKeyInit(&key[2],
+				Anum_pg_depend_objsubid,
+				BTEqualStrategyNumber, F_INT4EQ,
+				Int32GetDatum(0));
+
+	SysScanDesc scan = systable_beginscan(pgDepend, DependDependerIndexId, true,
+										  NULL, 3, key);
+
+	bool result = false;
+
+	HeapTuple heapTuple = NULL;
+	while (HeapTupleIsValid(heapTuple = systable_getnext(scan)))
+	{
+		Form_pg_depend dependForm = (Form_pg_depend) GETSTRUCT(heapTuple);
+
+		if (dependForm->refclassid == ExtensionRelationId)
+		{
+			result = true;
+			break;
+		}
+	}
+
+	systable_endscan(scan);
+	table_close(pgDepend, AccessShareLock);
+
+	return result;
+}
+
+
+/*
+ * HasAnyRelationsUsingAccessMethod returns true if there are any relations
+ * using the access method with the given accessMethodId.
+ */
+static bool
+HasAnyRelationsUsingAccessMethod(Oid accessMethodId)
+{
+	ScanKeyData key[1];
+	Relation pgClass = table_open(RelationRelationId, AccessShareLock);
+	ScanKeyInit(&key[0],
+				Anum_pg_class_relam,
+				BTEqualStrategyNumber, F_OIDEQ,
+				ObjectIdGetDatum(accessMethodId));
+
+	SysScanDesc scan = systable_beginscan(pgClass, InvalidOid, false, NULL, 1, key);
+
+	bool result = HeapTupleIsValid(systable_getnext(scan));
+
+	systable_endscan(scan);
+	table_close(pgClass, AccessShareLock);
+
+	return result;
+}
+
+
 /*
  * PostprocessAlterExtensionCitusStmtForCitusColumnar process the case when upgrade citus
  * to version that support citus_columnar, or downgrade citus to lower version that
@@ -959,7 +1083,7 @@ PostprocessAlterExtensionCitusStmtForCitusColumnar(Node *parseTree)
 	{
 		/*alter extension citus update, need upgrade citus_columnar from Y to Z*/
 		int versionNumber = (int) (100 * strtod(CITUS_MAJORVERSION, NULL));
-		if (versionNumber >= 1110)
+		if (versionNumber >= 1110 && citusColumnarOid != InvalidOid)
 		{
 			char *curColumnarVersion = get_extension_version(citusColumnarOid);
 			if (strcmp(curColumnarVersion, CITUS_COLUMNAR_INTERNAL_VERSION) == 0)

src/backend/distributed/metadata/dependency.c

@@ -1249,8 +1249,9 @@ IsObjectAddressOwnedByCitus(const ObjectAddress *objectAddress)
 		return false;
 	}
 
-	bool ownedByCitus = extObjectAddress.objectId == citusId;
-	bool ownedByCitusColumnar = extObjectAddress.objectId == citusColumnarId;
+	bool ownedByCitus = OidIsValid(citusId) && extObjectAddress.objectId == citusId;
+	bool ownedByCitusColumnar = OidIsValid(citusColumnarId) &&
+								extObjectAddress.objectId == citusColumnarId;
 
 	return ownedByCitus || ownedByCitusColumnar;
 }

src/backend/distributed/sql/citus--13.1-1--13.2-1.sql

@@ -1,3 +1,72 @@
 -- citus--13.1-1--13.2-1
 -- bump version to 13.2-1
 #include "udfs/worker_last_saved_explain_analyze/13.2-1.sql"
+
+#include "udfs/citus_finish_pg_upgrade/13.2-1.sql"
+
+DO $drop_leftover_old_columnar_objects$
+BEGIN
+  -- If old columnar exists, i.e., the columnar access method that we had before Citus 11.1,
+  -- and we don't have any relations using the old columnar, then we want to drop the columnar
+  -- objects. This is because, we don't want to automatically create the "citus_columnar"
+  -- extension together with the "citus" extension anymore. And for the cases where we don't
+  -- want to automatically create the "citus_columnar" extension, there is no point of keeping
+  -- the columnar objects that we had before Citus 11.1 around.
+  IF (
+    SELECT EXISTS (
+      SELECT 1 FROM pg_am
+      WHERE
+        -- looking for an access method whose name is "columnar" ..
+        pg_am.amname = 'columnar' AND
+        -- .. and there should *NOT* be such a dependency edge in pg_depend, where ..
+        NOT EXISTS (
+          SELECT 1 FROM pg_depend
+          WHERE
+            -- .. the depender is columnar access method (2601 = access method class) ..
+            pg_depend.classid = 2601 AND pg_depend.objid = pg_am.oid AND pg_depend.objsubid = 0 AND
+            -- .. and the dependee is an extension (3079 = extension class)
+            pg_depend.refclassid = 3079 AND pg_depend.refobjsubid = 0
+          LIMIT 1
+        ) AND
+        -- .. and there should *NOT* be any relations using it
+        NOT EXISTS (
+          SELECT 1
+          FROM pg_class
+          WHERE pg_class.relam = pg_am.oid
+          LIMIT 1
+        )
+    )
+  )
+  THEN
+    -- Below we drop the columnar objects in such an order that the objects that depend on
+    -- other objects are dropped first.
+
+    DROP VIEW IF EXISTS columnar.options;
+    DROP VIEW IF EXISTS columnar.stripe;
+    DROP VIEW IF EXISTS columnar.chunk_group;
+    DROP VIEW IF EXISTS columnar.chunk;
+    DROP VIEW IF EXISTS columnar.storage;
+
+    DROP ACCESS METHOD IF EXISTS columnar;
+
+    DROP SEQUENCE IF EXISTS columnar_internal.storageid_seq;
+
+    DROP TABLE IF EXISTS columnar_internal.options;
+    DROP TABLE IF EXISTS columnar_internal.stripe;
+    DROP TABLE IF EXISTS columnar_internal.chunk_group;
+    DROP TABLE IF EXISTS columnar_internal.chunk;
+
+    DROP FUNCTION IF EXISTS columnar_internal.columnar_handler;
+
+    DROP FUNCTION IF EXISTS pg_catalog.alter_columnar_table_set;
+    DROP FUNCTION IF EXISTS pg_catalog.alter_columnar_table_reset;
+    DROP FUNCTION IF EXISTS columnar.get_storage_id;
+
+    DROP FUNCTION IF EXISTS citus_internal.upgrade_columnar_storage;
+    DROP FUNCTION IF EXISTS citus_internal.downgrade_columnar_storage;
+    DROP FUNCTION IF EXISTS citus_internal.columnar_ensure_am_depends_catalog;
+
+    DROP SCHEMA IF EXISTS columnar;
+    DROP SCHEMA IF EXISTS columnar_internal;
+  END IF;
+END $drop_leftover_old_columnar_objects$;

src/backend/distributed/sql/downgrades/citus--13.2-1--13.1-1.sql

@@ -3,3 +3,11 @@
 
 DROP FUNCTION IF EXISTS pg_catalog.worker_last_saved_explain_analyze();
 #include "../udfs/worker_last_saved_explain_analyze/9.4-1.sql"
+
+#include "../udfs/citus_finish_pg_upgrade/13.1-1.sql"
+
+-- Note that we intentionally don't add the old columnar objects back to the "citus"
+-- extension in this downgrade script, even if they were present in the older version.
+--
+-- If the user wants to create "citus_columnar" extension later, "citus_columnar"
+-- will anyway properly create them at the scope of that extension.