From 4fac7895cbccbbdf7091c28901e8b2d4d9d30537 Mon Sep 17 00:00:00 2001 From: Deepthi Srinivasan Date: Tue, 1 Mar 2022 02:41:10 +0000 Subject: [PATCH] [BACKPORT 2.8][#9547] YSQL: alter table of parent geo-partitioned table affects select on child Summary: Failing example: ``` -- Add column to parent partitioned table. yugabyte@yugabyte=# alter table transactions add column foo varchar(30); ALTER TABLE -- Column is added to child partition in PG metadata. yugabyte@yugabyte=# \d transactions_eu; Table "public.transactions_eu" Column | Type | Collation | Nullable | Default ---------------+-----------------------------+-----------+----------+--------- user_id | integer | | not null | geo_partition | character varying | | not null | foo | character varying(30) | | | Partition of: transactions FOR VALUES IN ('EU', 'EMEA') -- Select data from child localhost:5433 yugabyte@yugabyte=# select * from transactions_eu; ERROR: Invalid argument: Invalid column number 8 ``` The above issue is happening because ALTER on parent partitioned table recursively reflects the ALTER operation on the child partitions by the PG layer. However, this does not happen on the DocDB side. This leads to mismatch in the YSQL and DocDB metadata for the child partition tables following an ALTER, resulting in failure of future DQL and DML operations. Original Diff: https://phabricator.dev.yugabyte.com/D14685 Original Commit: ff176a5b2208646572c7220a58ac60700d5837e8 Test Plan: ybd --scb --sj --java-test org.yb.pgsql.TestPgRegressPartitions Reviewers: myang Reviewed By: myang Subscribers: dfelsing, yql Differential Revision: https://phabricator.dev.yugabyte.com/D16021 --- src/postgres/src/backend/commands/tablecmds.c | 74 ++++++++++++++++--- src/postgres/src/backend/commands/ybccmds.c | 35 +++++++-- src/postgres/src/include/commands/ybccmds.h | 3 +- .../regress/expected/yb_partition_alter.out | 61 +++++++++++++++ .../test/regress/sql/yb_partition_alter.sql | 38 ++++++++++ .../src/test/regress/yb_partitions_schedule | 1 + 6 files changed, 194 insertions(+), 18 deletions(-) create mode 100644 src/postgres/src/test/regress/expected/yb_partition_alter.out create mode 100644 src/postgres/src/test/regress/sql/yb_partition_alter.sql diff --git a/src/postgres/src/backend/commands/tablecmds.c b/src/postgres/src/backend/commands/tablecmds.c index cb1c1002b282..b9e861cf7017 100644 --- a/src/postgres/src/backend/commands/tablecmds.c +++ b/src/postgres/src/backend/commands/tablecmds.c @@ -370,7 +370,7 @@ static void ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, bool recurse, bool recursing, LOCKMODE lockmode); static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, - YBCPgStatement *rollbackHandle); + List **rollbackHandles); static void ATExecCmd(List **wqueue, AlteredTableInfo *tab, Relation *mutable_rel, AlterTableCmd *cmd, LOCKMODE lockmode); static void ATRewriteTables(AlterTableStmt *parsetree, @@ -3119,6 +3119,17 @@ renameatt(RenameStmt *stmt) if (IsYugaByteEnabled()) { YBCRename(stmt, relid); + if (stmt->renameType == OBJECT_COLUMN) + { + ListCell *child; + List *children = find_all_inheritors(relid, NoLock, NULL); + foreach(child, children) + { + Oid childrelid = lfirst_oid(child); + if (childrelid != relid) + YBCRename(stmt, childrelid); + } + } } ObjectAddressSubSet(address, RelationRelationId, relid, attnum); @@ -3851,13 +3862,13 @@ ATController(AlterTableStmt *parsetree, relation_close(rel, NoLock); /* Phase 2: update system catalogs */ - YBCPgStatement rollbackHandle = NULL; + List *rollbackHandles = NIL; /* * ATRewriteCatalogs also executes changes to DocDB. * If Phase 3 fails, rollbackHandle will specify how to rollback the * changes done to DocDB. */ - ATRewriteCatalogs(&wqueue, lockmode, &rollbackHandle); + ATRewriteCatalogs(&wqueue, lockmode, &rollbackHandles); /* Phase 3: scan/rewrite tables as needed */ PG_TRY(); @@ -3867,9 +3878,11 @@ ATController(AlterTableStmt *parsetree, PG_CATCH(); { /* Rollback the DocDB changes. */ - if (rollbackHandle) + ListCell *lc = NULL; + foreach(lc, rollbackHandles) { - YBCExecAlterTable(rollbackHandle, RelationGetRelid(rel)); + YBCPgStatement handle = (YBCPgStatement) lfirst(lc); + YBCExecAlterTable(handle, RelationGetRelid(rel)); } PG_RE_THROW(); } @@ -4192,7 +4205,7 @@ ATPrepCmd(List **wqueue, Relation rel, AlterTableCmd *cmd, static void ATRewriteCatalogs(List **wqueue, LOCKMODE lockmode, - YBCPgStatement *rollbackHandle) + List **rollbackHandles) { int pass; ListCell *ltab; @@ -4208,10 +4221,42 @@ ATRewriteCatalogs(List **wqueue, */ AlteredTableInfo* info = (AlteredTableInfo *) linitial(*wqueue); Oid main_relid = info->relid; - YBCPgStatement handle = YBCPrepareAlterTable(info->subcmds, - AT_NUM_PASSES, - main_relid, - rollbackHandle); + YBCPgStatement rollbackHandle = NULL; + List *handles = NIL; + YBCPgStatement handle = YBCPrepareAlterTable(info->subcmds, + AT_NUM_PASSES, + main_relid, + &rollbackHandle, + false /* isPartitionOfAlteredTable */); + handles = lappend(handles, handle); + if (rollbackHandle) + *rollbackHandles = lappend(*rollbackHandles, rollbackHandle); + + /* + * If this is a partitioned table, prepare alter table handles for all + * the partitions as well. + */ + List *children = find_all_inheritors(main_relid, NoLock, NULL); + ListCell *child; + foreach(child, children) + { + Oid childrelid = lfirst_oid(child); + /* + * find_all_inheritors also returns the oid of the table itself. + * Skip it, as we have already added handles for it. + */ + if (childrelid == main_relid) + continue; + YBCPgStatement childRollbackHandle = NULL; + YBCPgStatement child_handle = YBCPrepareAlterTable(info->subcmds, + AT_NUM_PASSES, + childrelid, + &childRollbackHandle, + true /*isPartitionOfAlteredTable */); + handles = lappend(handles, child_handle); + if (childRollbackHandle) + *rollbackHandles = lappend(*rollbackHandles, childRollbackHandle); + } /* * We process all the tables "in parallel", one pass at a time. This is @@ -4220,6 +4265,7 @@ ATRewriteCatalogs(List **wqueue, * re-adding of the foreign key constraint to the other table). Work can * only be propagated into later passes, however. */ + ListCell *lc = NULL; for (pass = 0; pass < AT_NUM_PASSES; pass++) { /* @@ -4231,9 +4277,13 @@ ATRewriteCatalogs(List **wqueue, * However, we must also do this before we start adding indexes because * the column in question might not be there yet. */ - if (pass == AT_PASS_ADD_INDEX && handle) + if (pass == AT_PASS_ADD_INDEX) { - YBCExecAlterTable(handle, main_relid); + foreach(lc, handles) + { + YBCPgStatement handle = (YBCPgStatement) lfirst(lc); + YBCExecAlterTable(handle, main_relid); + } } /* Go through each table that needs to be processed */ diff --git a/src/postgres/src/backend/commands/ybccmds.c b/src/postgres/src/backend/commands/ybccmds.c index 19c92b683ea7..153c6d305184 100644 --- a/src/postgres/src/backend/commands/ybccmds.c +++ b/src/postgres/src/backend/commands/ybccmds.c @@ -876,9 +876,32 @@ YBCCreateIndex(const char *indexName, static void YBCPrepareAlterTableCmd(AlterTableCmd* cmd, Relation rel, YBCPgStatement handle, - int* col, bool* needsYBAlter, - YBCPgStatement* rollbackHandle) + int* col, bool* needsYBAlter, + YBCPgStatement* rollbackHandle, + bool isPartitionOfAlteredTable) { + if (isPartitionOfAlteredTable) + { + /* + * This function was invoked on a child partition table to reflect + * the effects of Alter on its parent. + */ + switch (cmd->subtype) + { + case AT_AddColumnRecurse: + case AT_DropColumnRecurse: + case AT_AddConstraintRecurse: + case AT_DropConstraintRecurse: + break; + default: + /* + * This is not an alter command on a partitioned table that + * needs to trickle down to its child partitions. Nothing to + * do. + */ + return; + } + } Oid relationId = RelationGetRelid(rel); switch (cmd->subtype) { @@ -908,7 +931,7 @@ YBCPrepareAlterTableCmd(AlterTableCmd* cmd, Relation rel, YBCPgStatement handle, const YBCPgTypeEntity *col_type = YbDataTypeFromOidMod(order, typeOid); HandleYBStatus(YBCPgAlterTableAddColumn(handle, colDef->colname, - order, col_type)); + order, col_type)); ++(*col); ReleaseSysCache(typeTuple); *needsYBAlter = true; @@ -1076,7 +1099,8 @@ YBCPgStatement YBCPrepareAlterTable(List** subcmds, int subcmds_size, Oid relationId, - YBCPgStatement *rollbackHandle) + YBCPgStatement *rollbackHandle, + bool isPartitionOfAlteredTable) { /* Appropriate lock was already taken */ Relation rel = relation_open(relationId, NoLock); @@ -1101,7 +1125,8 @@ YBCPrepareAlterTable(List** subcmds, foreach(lcmd, subcmds[cmd_idx]) { YBCPrepareAlterTableCmd((AlterTableCmd *) lfirst(lcmd), rel, handle, - &col, &needsYBAlter, rollbackHandle); + &col, &needsYBAlter, rollbackHandle, + isPartitionOfAlteredTable); } } relation_close(rel, NoLock); diff --git a/src/postgres/src/include/commands/ybccmds.h b/src/postgres/src/include/commands/ybccmds.h index 162a313376b7..8bc69739f1cc 100644 --- a/src/postgres/src/include/commands/ybccmds.h +++ b/src/postgres/src/include/commands/ybccmds.h @@ -80,7 +80,8 @@ extern void YBCDropIndex(Oid relationId); extern YBCPgStatement YBCPrepareAlterTable(List** subcmds, int subcmds_size, Oid relationId, - YBCPgStatement *rollbackHandle); + YBCPgStatement *rollbackHandle, + bool isPartitionOfAlteredTable); extern void YBCExecAlterTable(YBCPgStatement handle, Oid relationId); diff --git a/src/postgres/src/test/regress/expected/yb_partition_alter.out b/src/postgres/src/test/regress/expected/yb_partition_alter.out new file mode 100644 index 000000000000..acff33b5d97d --- /dev/null +++ b/src/postgres/src/test/regress/expected/yb_partition_alter.out @@ -0,0 +1,61 @@ +-- Create a partitioned hierarchy of LIST, RANGE and HASH. +CREATE TABLE root_list_parent (list_part_key char, hash_part_key int, range_part_key int) PARTITION BY LIST(list_part_key); +CREATE TABLE hash_parent PARTITION OF root_list_parent FOR VALUES in ('a', 'b') PARTITION BY HASH (hash_part_key); +CREATE TABLE range_parent PARTITION OF hash_parent FOR VALUES WITH (modulus 1, remainder 0) PARTITION BY RANGE (range_part_key); +CREATE TABLE child_partition PARTITION OF range_parent FOR VALUES FROM (1) TO (5); +INSERT INTO root_list_parent VALUES ('a', 1, 2); +-- Add a column to the parent table, verify that selecting data still works. +ALTER TABLE root_list_parent ADD COLUMN foo VARCHAR(2); +SELECT * FROM root_list_parent; + list_part_key | hash_part_key | range_part_key | foo +---------------+---------------+----------------+----- + a | 1 | 2 | +(1 row) + +-- Alter column type at the parent table. +ALTER TABLE root_list_parent ALTER COLUMN foo TYPE VARCHAR(3); +INSERT INTO root_list_parent VALUES ('a', 1, 2, 'abc'); +SELECT * FROM root_list_parent ORDER BY foo; + list_part_key | hash_part_key | range_part_key | foo +---------------+---------------+----------------+----- + a | 1 | 2 | abc + a | 1 | 2 | +(2 rows) + +-- Drop a column from the parent table, verify that selecting data still works. +ALTER TABLE root_list_parent DROP COLUMN foo; +SELECT * FROM root_list_parent; + list_part_key | hash_part_key | range_part_key +---------------+---------------+---------------- + a | 1 | 2 + a | 1 | 2 +(2 rows) + +-- Retry adding a column after error. +ALTER TABLE root_list_parent ADD COLUMN foo text not null; -- fails due to not null constraint +ERROR: column "foo" contains null values +ALTER TABLE root_list_parent ADD COLUMN foo text not null DEFAULT 'abc'; -- passes +-- Rename a column belonging to the parent table. +ALTER TABLE root_list_parent RENAME COLUMN list_part_key TO list_part_key_renamed; +-- Note: Incorrect output here for 'foo' because of #9970. +SELECT * FROM child_partition ORDER BY foo; + list_part_key_renamed | hash_part_key | range_part_key | foo +-----------------------+---------------+----------------+----- + a | 1 | 2 | + a | 1 | 2 | +(2 rows) + +TRUNCATE root_list_parent; +-- Add constraint to the parent table, verify that it reflects on the child partition. +ALTER TABLE root_list_parent ADD CONSTRAINT constraint_test UNIQUE (list_part_key_renamed, hash_part_key, range_part_key, foo); +INSERT INTO child_partition VALUES ('a', 1, 2), ('a', 1, 2); +ERROR: duplicate key value violates unique constraint "child_partition_list_part_key_renamed_hash_part_key_range_p_key" +-- Remove constraint from the parent table, verify that it reflects on the child partition. +ALTER TABLE root_list_parent DROP CONSTRAINT constraint_test; +INSERT INTO child_partition VALUES ('a', 1, 2), ('a', 1, 2); +SELECT * FROM root_list_parent ORDER BY foo; + list_part_key_renamed | hash_part_key | range_part_key | foo +-----------------------+---------------+----------------+----- + a | 1 | 2 | abc + a | 1 | 2 | abc +(2 rows) diff --git a/src/postgres/src/test/regress/sql/yb_partition_alter.sql b/src/postgres/src/test/regress/sql/yb_partition_alter.sql new file mode 100644 index 000000000000..7b66208d487a --- /dev/null +++ b/src/postgres/src/test/regress/sql/yb_partition_alter.sql @@ -0,0 +1,38 @@ +-- Create a partitioned hierarchy of LIST, RANGE and HASH. +CREATE TABLE root_list_parent (list_part_key char, hash_part_key int, range_part_key int) PARTITION BY LIST(list_part_key); +CREATE TABLE hash_parent PARTITION OF root_list_parent FOR VALUES in ('a', 'b') PARTITION BY HASH (hash_part_key); +CREATE TABLE range_parent PARTITION OF hash_parent FOR VALUES WITH (modulus 1, remainder 0) PARTITION BY RANGE (range_part_key); +CREATE TABLE child_partition PARTITION OF range_parent FOR VALUES FROM (1) TO (5); +INSERT INTO root_list_parent VALUES ('a', 1, 2); + +-- Add a column to the parent table, verify that selecting data still works. +ALTER TABLE root_list_parent ADD COLUMN foo VARCHAR(2); +SELECT * FROM root_list_parent; + +-- Alter column type at the parent table. +ALTER TABLE root_list_parent ALTER COLUMN foo TYPE VARCHAR(3); +INSERT INTO root_list_parent VALUES ('a', 1, 2, 'abc'); +SELECT * FROM root_list_parent ORDER BY foo; + +-- Drop a column from the parent table, verify that selecting data still works. +ALTER TABLE root_list_parent DROP COLUMN foo; +SELECT * FROM root_list_parent; + +-- Retry adding a column after error. +ALTER TABLE root_list_parent ADD COLUMN foo text not null; -- fails due to not null constraint +ALTER TABLE root_list_parent ADD COLUMN foo text not null DEFAULT 'abc'; -- passes + +-- Rename a column belonging to the parent table. +ALTER TABLE root_list_parent RENAME COLUMN list_part_key TO list_part_key_renamed; +-- Note: Incorrect output here for 'foo' because of #9970. +SELECT * FROM child_partition ORDER BY foo; +TRUNCATE root_list_parent; + +-- Add constraint to the parent table, verify that it reflects on the child partition. +ALTER TABLE root_list_parent ADD CONSTRAINT constraint_test UNIQUE (list_part_key_renamed, hash_part_key, range_part_key, foo); +INSERT INTO child_partition VALUES ('a', 1, 2), ('a', 1, 2); + +-- Remove constraint from the parent table, verify that it reflects on the child partition. +ALTER TABLE root_list_parent DROP CONSTRAINT constraint_test; +INSERT INTO child_partition VALUES ('a', 1, 2), ('a', 1, 2); +SELECT * FROM root_list_parent ORDER BY foo; diff --git a/src/postgres/src/test/regress/yb_partitions_schedule b/src/postgres/src/test/regress/yb_partitions_schedule index ccf273ed84a7..f99fe1f1fa72 100644 --- a/src/postgres/src/test/regress/yb_partitions_schedule +++ b/src/postgres/src/test/regress/yb_partitions_schedule @@ -6,3 +6,4 @@ ############################################################################### test: yb_partition_fk test: yb_partition_pk +test: yb_partition_alter