From 4a4a2209453227160f76be33e47156dfe85409b6 Mon Sep 17 00:00:00 2001 From: Nils Dijk Date: Mon, 7 Oct 2019 17:16:19 +0200 Subject: [PATCH] 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 | 21 +++++- .../regress/expected/distributed_types.out | 14 ++++ .../regress/expected/distributed_types_0.out | 14 ++++ .../distributed_types_xact_add_enum_value.out | 73 ++++++++++++++++++ ...istributed_types_xact_add_enum_value_0.out | 75 +++++++++++++++++++ src/test/regress/multi_schedule | 2 +- src/test/regress/sql/distributed_types.sql | 4 + .../distributed_types_xact_add_enum_value.sql | 35 +++++++++ 8 files changed, 234 insertions(+), 4 deletions(-) create mode 100644 src/test/regress/expected/distributed_types_xact_add_enum_value.out create mode 100644 src/test/regress/expected/distributed_types_xact_add_enum_value_0.out create mode 100644 src/test/regress/sql/distributed_types_xact_add_enum_value.sql diff --git a/src/backend/distributed/commands/type.c b/src/backend/distributed/commands/type.c index 86d32979b..b57f8efef 100644 --- a/src/backend/distributed/commands/type.c +++ b/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); diff --git a/src/test/regress/expected/distributed_types.out b/src/test/regress/expected/distributed_types.out index 94c5e4d2f..571e40645 100644 --- a/src/test/regress/expected/distributed_types.out +++ b/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; diff --git a/src/test/regress/expected/distributed_types_0.out b/src/test/regress/expected/distributed_types_0.out index 8acfd99c8..d27e5d686 100644 --- a/src/test/regress/expected/distributed_types_0.out +++ b/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; diff --git a/src/test/regress/expected/distributed_types_xact_add_enum_value.out b/src/test/regress/expected/distributed_types_xact_add_enum_value.out new file mode 100644 index 000000000..c5e818c36 --- /dev/null +++ b/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) + diff --git a/src/test/regress/expected/distributed_types_xact_add_enum_value_0.out b/src/test/regress/expected/distributed_types_xact_add_enum_value_0.out new file mode 100644 index 000000000..934dcaf06 --- /dev/null +++ b/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) + diff --git a/src/test/regress/multi_schedule b/src/test/regress/multi_schedule index af1bf504e..61ebe7612 100644 --- a/src/test/regress/multi_schedule +++ b/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 diff --git a/src/test/regress/sql/distributed_types.sql b/src/test/regress/sql/distributed_types.sql index 6c56dcbac..2258475d9 100644 --- a/src/test/regress/sql/distributed_types.sql +++ b/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; diff --git a/src/test/regress/sql/distributed_types_xact_add_enum_value.sql b/src/test/regress/sql/distributed_types_xact_add_enum_value.sql new file mode 100644 index 000000000..e9020f140 --- /dev/null +++ b/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;$$);