Skip to content

Commit

Permalink
[BACKPORT 2.8][#9547] YSQL: alter table of parent geo-partitioned tab…
Browse files Browse the repository at this point in the history
…le 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: ff176a5

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
  • Loading branch information
deeps1991 committed Mar 28, 2022
1 parent 29ecc1d commit 4fac789
Show file tree
Hide file tree
Showing 6 changed files with 194 additions and 18 deletions.
74 changes: 62 additions & 12 deletions src/postgres/src/backend/commands/tablecmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();
Expand All @@ -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();
}
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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++)
{
/*
Expand All @@ -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 */
Expand Down
35 changes: 30 additions & 5 deletions src/postgres/src/backend/commands/ybccmds.c
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand All @@ -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);
Expand Down
3 changes: 2 additions & 1 deletion src/postgres/src/include/commands/ybccmds.h
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
61 changes: 61 additions & 0 deletions src/postgres/src/test/regress/expected/yb_partition_alter.out
Original file line number Diff line number Diff line change
@@ -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)
38 changes: 38 additions & 0 deletions src/postgres/src/test/regress/sql/yb_partition_alter.sql
Original file line number Diff line number Diff line change
@@ -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;
1 change: 1 addition & 0 deletions src/postgres/src/test/regress/yb_partitions_schedule
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@
###############################################################################
test: yb_partition_fk
test: yb_partition_pk
test: yb_partition_alter

0 comments on commit 4fac789

Please sign in to comment.