Fix enum add value order and pg12 (#3082)

DESCRIPTION: Fix order for enum values and correctly support pg12

PG 12 introduces `ALTER TYPE ... ADD VALUE ...` during transactions. Earlier versions would error out when called in a transaction, hence we connect to workers outside of the transaction which could cause inconsistencies on pg12 now that postgres doesn't error with this syntax anymore.

During the implementation of this fix it became apparent there was an error with the ordering of enum labels when the type was recreated. A patch and test have been included.

src/backend/distributed/commands/type.c

@@ -355,7 +355,12 @@ PlanAlterEnumStmt(AlterEnumStmt *stmt, const char *queryString)
 	QualifyTreeNode((Node *) stmt);
 	alterEnumStmtSql = DeparseTreeNode((Node *) stmt);
 
-	/* TODO this is not needed anymore for pg12, alter enum can actually run in a xact */
+	/*
+	 * Before pg12 ALTER ENUM ... ADD VALUE could not be within a xact block. Instead of
+	 * creating a DDLTaksList we won't return anything here. During the processing phase
+	 * we directly connect to workers and execute the commands remotely.
+	 */
+#if PG_VERSION_NUM < 120000
 	if (AlterEnumIsAddValue(stmt))
 	{
 		/*
@@ -364,6 +369,7 @@ PlanAlterEnumStmt(AlterEnumStmt *stmt, const char *queryString)
 		 */
 		return NIL;
 	}
+#endif
 
 	commands = list_make3(DISABLE_DDL_PROPAGATION,
 						  (void *) alterEnumStmtSql,
@@ -400,7 +406,15 @@ ProcessAlterEnumStmt(AlterEnumStmt *stmt, const char *queryString)
 		return;
 	}
 
-	/* TODO this is not needed anymore for pg12, alter enum can actually run in a xact */
+	/*
+	 * Before pg12 ALTER ENUM ... ADD VALUE could not be within a xact block. Normally we
+	 * would propagate the statements in a xact block to perform 2pc on changes via ddl.
+	 * Instead we need to connect directly to the workers here and execute the command.
+	 *
+	 * From pg12 and up we use the normal infrastructure and create the ddl jobs during
+	 * planning.
+	 */
+#if PG_VERSION_NUM < 120000
 	if (AlterEnumIsAddValue(stmt))
 	{
 		/*
@@ -437,6 +451,7 @@ ProcessAlterEnumStmt(AlterEnumStmt *stmt, const char *queryString)
 									  "all workers")));
 		}
 	}
+#endif
 }
 
 
@@ -832,7 +847,7 @@ EnumValsList(Oid typeOid)
 
 	enum_rel = heap_open(EnumRelationId, AccessShareLock);
 	enum_scan = systable_beginscan(enum_rel,
-								   EnumTypIdLabelIndexId,
+								   EnumTypIdSortOrderIndexId,
 								   true, NULL,
 								   1, &skey);
 

src/test/regress/expected/distributed_types.out

@@ -100,6 +100,20 @@ SELECT * FROM t4;
 
 -- ALTER TYPE ... ADD VALUE does not work in transactions
 COMMIT;
+-- verify order of enum labels
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;
+ string_agg 
+------------
+ yes,no
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;$$);
+    run_command_on_workers    
+------------------------------
+ (localhost,57637,t,"yes,no")
+ (localhost,57638,t,"yes,no")
+(2 rows)
+
 -- test some combination of types without ddl propagation, this will prevent the workers
 -- from having those types created. They are created just-in-time on table distribution
 SET citus.enable_ddl_propagation TO off;

src/test/regress/expected/distributed_types_0.out

@@ -100,6 +100,20 @@ SELECT * FROM t4;
 
 -- ALTER TYPE ... ADD VALUE does not work in transactions
 COMMIT;
+-- verify order of enum labels
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;
+ string_agg 
+------------
+ yes,no
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;$$);
+    run_command_on_workers    
+------------------------------
+ (localhost,57637,t,"yes,no")
+ (localhost,57638,t,"yes,no")
+(2 rows)
+
 -- test some combination of types without ddl propagation, this will prevent the workers
 -- from having those types created. They are created just-in-time on table distribution
 SET citus.enable_ddl_propagation TO off;

src/test/regress/expected/distributed_types_xact_add_enum_value.out

@@ -0,0 +1,73 @@
+SHOW server_version \gset
+SELECT substring(:'server_version', '\d+')::int > 11 AS version_above_eleven;
+ version_above_eleven 
+----------------------
+ t
+(1 row)
+
+SET citus.next_shard_id TO 20040000;
+CREATE SCHEMA xact_enum_type;
+SET search_path TO xact_enum_type;
+SET citus.shard_count TO 4;
+-- transaction block with simple type
+BEGIN;
+CREATE TYPE xact_enum_edit AS ENUM ('yes', 'no');
+CREATE TABLE t1 (a int PRIMARY KEY, b xact_enum_edit);
+SELECT create_distributed_table('t1','a');
+ create_distributed_table 
+--------------------------
+ 
+(1 row)
+
+INSERT INTO t1 VALUES (1, 'yes');
+SELECT * FROM t1;
+ a |  b  
+---+-----
+ 1 | yes
+(1 row)
+
+COMMIT;
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+ABORT;
+-- maybe should not be on the workers
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+ string_agg 
+------------
+ yes,no
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+    run_command_on_workers    
+------------------------------
+ (localhost,57637,t,"yes,no")
+ (localhost,57638,t,"yes,no")
+(2 rows)
+
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+COMMIT;
+-- maybe should be on the workers (pg12 and above)
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+  string_agg  
+--------------
+ yes,no,maybe
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+       run_command_on_workers       
+------------------------------------
+ (localhost,57637,t,"yes,no,maybe")
+ (localhost,57638,t,"yes,no,maybe")
+(2 rows)
+
+-- clear objects
+SET client_min_messages TO error; -- suppress cascading objects dropping
+DROP SCHEMA xact_enum_type CASCADE;
+SELECT run_command_on_workers($$DROP SCHEMA xact_enum_type CASCADE;$$);
+      run_command_on_workers       
+-----------------------------------
+ (localhost,57637,t,"DROP SCHEMA")
+ (localhost,57638,t,"DROP SCHEMA")
+(2 rows)
+

src/test/regress/expected/distributed_types_xact_add_enum_value_0.out

@@ -0,0 +1,75 @@
+SHOW server_version \gset
+SELECT substring(:'server_version', '\d+')::int > 11 AS version_above_eleven;
+ version_above_eleven 
+----------------------
+ f
+(1 row)
+
+SET citus.next_shard_id TO 20040000;
+CREATE SCHEMA xact_enum_type;
+SET search_path TO xact_enum_type;
+SET citus.shard_count TO 4;
+-- transaction block with simple type
+BEGIN;
+CREATE TYPE xact_enum_edit AS ENUM ('yes', 'no');
+CREATE TABLE t1 (a int PRIMARY KEY, b xact_enum_edit);
+SELECT create_distributed_table('t1','a');
+ create_distributed_table 
+--------------------------
+ 
+(1 row)
+
+INSERT INTO t1 VALUES (1, 'yes');
+SELECT * FROM t1;
+ a |  b  
+---+-----
+ 1 | yes
+(1 row)
+
+COMMIT;
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
+ABORT;
+-- maybe should not be on the workers
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+ string_agg 
+------------
+ yes,no
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+    run_command_on_workers    
+------------------------------
+ (localhost,57637,t,"yes,no")
+ (localhost,57638,t,"yes,no")
+(2 rows)
+
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+ERROR:  ALTER TYPE ... ADD cannot run inside a transaction block
+COMMIT;
+-- maybe should be on the workers (pg12 and above)
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+ string_agg 
+------------
+ yes,no
+(1 row)
+
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+    run_command_on_workers    
+------------------------------
+ (localhost,57637,t,"yes,no")
+ (localhost,57638,t,"yes,no")
+(2 rows)
+
+-- clear objects
+SET client_min_messages TO error; -- suppress cascading objects dropping
+DROP SCHEMA xact_enum_type CASCADE;
+SELECT run_command_on_workers($$DROP SCHEMA xact_enum_type CASCADE;$$);
+      run_command_on_workers       
+-----------------------------------
+ (localhost,57637,t,"DROP SCHEMA")
+ (localhost,57638,t,"DROP SCHEMA")
+(2 rows)
+

src/test/regress/multi_schedule

@@ -280,7 +280,7 @@ test: ssl_by_default
 # ---------
 # object distribution tests
 # ---------
-test: distributed_types distributed_types_conflict disable_object_propagation
+test: distributed_types distributed_types_conflict disable_object_propagation distributed_types_xact_add_enum_value
 test: distributed_functions
 test: distributed_procedure
 

src/test/regress/sql/distributed_types.sql

@@ -57,6 +57,10 @@ SELECT * FROM t4;
 -- ALTER TYPE ... ADD VALUE does not work in transactions
 COMMIT;
 
+-- verify order of enum labels
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'type_tests.te2'::regtype;$$);
+
 -- test some combination of types without ddl propagation, this will prevent the workers
 -- from having those types created. They are created just-in-time on table distribution
 SET citus.enable_ddl_propagation TO off;

src/test/regress/sql/distributed_types_xact_add_enum_value.sql

@@ -0,0 +1,35 @@
+SHOW server_version \gset
+SELECT substring(:'server_version', '\d+')::int > 11 AS version_above_eleven;
+SET citus.next_shard_id TO 20040000;
+
+CREATE SCHEMA xact_enum_type;
+SET search_path TO xact_enum_type;
+SET citus.shard_count TO 4;
+
+-- transaction block with simple type
+BEGIN;
+CREATE TYPE xact_enum_edit AS ENUM ('yes', 'no');
+CREATE TABLE t1 (a int PRIMARY KEY, b xact_enum_edit);
+SELECT create_distributed_table('t1','a');
+INSERT INTO t1 VALUES (1, 'yes');
+SELECT * FROM t1;
+COMMIT;
+
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+ABORT;
+-- maybe should not be on the workers
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+
+BEGIN;
+ALTER TYPE xact_enum_edit ADD VALUE 'maybe';
+COMMIT;
+-- maybe should be on the workers (pg12 and above)
+SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;
+SELECT run_command_on_workers($$SELECT string_agg(enumlabel, ',' ORDER BY enumsortorder ASC) FROM pg_enum WHERE enumtypid = 'xact_enum_type.xact_enum_edit'::regtype;$$);
+
+-- clear objects
+SET client_min_messages TO error; -- suppress cascading objects dropping
+DROP SCHEMA xact_enum_type CASCADE;
+SELECT run_command_on_workers($$DROP SCHEMA xact_enum_type CASCADE;$$);