From fc2a6043cef8a48bf3145e5e1bca90ac33e7efa9 Mon Sep 17 00:00:00 2001 From: Shlok Kyal Date: Tue, 5 Nov 2024 11:11:25 +0530 Subject: [PATCH v9] Disallow UPDATE/DELETE on table with generated column as REPLICA IDENTITY UPDATE/DELETE on table having unpublished generated column as REPLICA IDENTITY is allowed. UPDATE/DELETE on such tables should not be allowed --- src/backend/commands/publicationcmds.c | 78 +++++++++++++++++++++++ src/backend/executor/execReplication.c | 12 ++++ src/backend/utils/cache/relcache.c | 30 +++++++++ src/include/catalog/pg_publication.h | 7 ++ src/include/commands/publicationcmds.h | 2 + src/test/regress/expected/publication.out | 25 ++++++++ src/test/regress/sql/publication.sql | 26 ++++++++ src/test/subscription/t/100_bugs.pl | 4 +- 8 files changed, 182 insertions(+), 2 deletions(-) diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c index 0129db18c6..053877c524 100644 --- a/src/backend/commands/publicationcmds.c +++ b/src/backend/commands/publicationcmds.c @@ -444,6 +444,84 @@ pub_collist_contains_invalid_column(Oid pubid, Relation relation, List *ancestor return result; } +/* + * Check if REPLICA IDENTITY consists of any unpublished generated column. + * + * Returns true if any replica identity column is an unpublished generated + * column. + */ +bool +replident_has_unpublished_gen_col(Oid pubid, Relation relation, List *ancestors, + bool pubviaroot) +{ + Oid relid = RelationGetRelid(relation); + Oid publish_as_relid = RelationGetRelid(relation); + bool result = false; + bool found; + Publication *pub; + + /* Return if the table does not contain any generated columns */ + if (!relation->rd_att->constr || + !relation->rd_att->constr->has_generated_stored) + return false; + + /* + * For a partition, if pubviaroot is true, find the topmost ancestor that + * is published via this publication as we need to use its column list for + * the changes. + * + * Note that even though the column list used is for an ancestor, the + * REPLICA IDENTITY used will be for the actual child table. + */ + if (pubviaroot && relation->rd_rel->relispartition) + { + publish_as_relid = GetTopMostAncestorInPublication(pubid, ancestors, NULL); + + if (!OidIsValid(publish_as_relid)) + publish_as_relid = relid; + } + + pub = GetPublication(pubid); + found = check_and_fetch_column_list(pub, publish_as_relid, NULL, NULL); + + if (!found) + { + TupleDesc desc = RelationGetDescr(relation); + Bitmapset *idattrs; + int x; + + /* + * REPLICA IDENTITY can be FULL only if there is no column list for + * publication. If REPLICA IDENTITY is set as FULL and relation has a + * generated column we should error out. + */ + if (relation->rd_rel->relreplident == REPLICA_IDENTITY_FULL) + return true; + + /* Remember columns that are part of the REPLICA IDENTITY */ + idattrs = RelationGetIndexAttrBitmap(relation, + INDEX_ATTR_BITMAP_IDENTITY_KEY); + + x = -1; + while ((x = bms_next_member(idattrs, x)) >= 0) + { + AttrNumber attnum = (x + FirstLowInvalidHeapAttributeNumber); + Form_pg_attribute att = TupleDescAttr(desc, attnum - 1); + + /* Check if generated column is part of REPLICA IDENTITY */ + if (!att->attisdropped && att->attgenerated) + { + result = true; + break; + } + } + + bms_free(idattrs); + } + + return result; +} + /* check_functions_in_node callback */ static bool contain_mutable_or_user_functions_checker(Oid func_id, void *context) diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c index 54025c9f15..57599df153 100644 --- a/src/backend/executor/execReplication.c +++ b/src/backend/executor/execReplication.c @@ -809,6 +809,12 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) errmsg("cannot update table \"%s\"", RelationGetRelationName(rel)), errdetail("Column list used by the publication does not cover the replica identity."))); + else if (cmd == CMD_UPDATE && !pubdesc.replident_valid_for_update) + ereport(ERROR, + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("cannot update table \"%s\"", + RelationGetRelationName(rel)), + errdetail("Replica identity consists of an unpublished generated column."))); else if (cmd == CMD_DELETE && !pubdesc.rf_valid_for_delete) ereport(ERROR, (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), @@ -821,6 +827,12 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd) errmsg("cannot delete from table \"%s\"", RelationGetRelationName(rel)), errdetail("Column list used by the publication does not cover the replica identity."))); + else if (cmd == CMD_DELETE && !pubdesc.replident_valid_for_delete) + ereport(ERROR, + (errcode(ERRCODE_INVALID_COLUMN_REFERENCE), + errmsg("cannot delete from table \"%s\"", + RelationGetRelationName(rel)), + errdetail("Replica identity consists of an unpublished generated column."))); /* If relation has replica identity we are always good. */ if (OidIsValid(RelationGetReplicaIndex(rel))) diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c index 342467fd18..be8f8eea8f 100644 --- a/src/backend/utils/cache/relcache.c +++ b/src/backend/utils/cache/relcache.c @@ -5736,6 +5736,8 @@ RelationBuildPublicationDesc(Relation relation, PublicationDesc *pubdesc) pubdesc->rf_valid_for_delete = true; pubdesc->cols_valid_for_update = true; pubdesc->cols_valid_for_delete = true; + pubdesc->replident_valid_for_update = true; + pubdesc->replident_valid_for_delete = true; return; } @@ -5750,6 +5752,8 @@ RelationBuildPublicationDesc(Relation relation, PublicationDesc *pubdesc) pubdesc->rf_valid_for_delete = true; pubdesc->cols_valid_for_update = true; pubdesc->cols_valid_for_delete = true; + pubdesc->replident_valid_for_update = true; + pubdesc->replident_valid_for_delete = true; /* Fetch the publication membership info. */ puboids = GetRelationPublications(relid); @@ -5827,6 +5831,21 @@ RelationBuildPublicationDesc(Relation relation, PublicationDesc *pubdesc) pubdesc->cols_valid_for_delete = false; } + /* + * Check if all generated columns included in the REPLICA IDENTITY are + * published. + */ + if (!pubform->pubgencols && + (pubform->pubupdate || pubform->pubdelete) && + replident_has_unpublished_gen_col(pubid, relation, ancestors, + pubform->pubviaroot)) + { + if (pubform->pubupdate) + pubdesc->replident_valid_for_update = false; + if (pubform->pubdelete) + pubdesc->replident_valid_for_delete = false; + } + ReleaseSysCache(tup); /* @@ -5848,6 +5867,17 @@ RelationBuildPublicationDesc(Relation relation, PublicationDesc *pubdesc) pubdesc->pubactions.pubdelete && pubdesc->pubactions.pubtruncate && !pubdesc->cols_valid_for_update && !pubdesc->cols_valid_for_delete) break; + + /* + * If we know everything is replicated and replica identity has an + * unpublished generated column, there is no point to check for other + * publications. + */ + if (pubdesc->pubactions.pubinsert && pubdesc->pubactions.pubupdate && + pubdesc->pubactions.pubdelete && pubdesc->pubactions.pubtruncate && + !pubdesc->replident_valid_for_update && + !pubdesc->replident_valid_for_delete) + break; } if (relation->rd_pubdesc) diff --git a/src/include/catalog/pg_publication.h b/src/include/catalog/pg_publication.h index 9a83a72d6b..486f609a9a 100644 --- a/src/include/catalog/pg_publication.h +++ b/src/include/catalog/pg_publication.h @@ -98,6 +98,13 @@ typedef struct PublicationDesc */ bool cols_valid_for_update; bool cols_valid_for_delete; + + /* + * true if all generated columns which are part of replica identity are + * published or the publication actions do not include UPDATE or DELETE. + */ + bool replident_valid_for_update; + bool replident_valid_for_delete; } PublicationDesc; typedef struct Publication diff --git a/src/include/commands/publicationcmds.h b/src/include/commands/publicationcmds.h index 5487c571f6..b18e576b77 100644 --- a/src/include/commands/publicationcmds.h +++ b/src/include/commands/publicationcmds.h @@ -35,5 +35,7 @@ extern bool pub_rf_contains_invalid_column(Oid pubid, Relation relation, List *ancestors, bool pubviaroot); extern bool pub_collist_contains_invalid_column(Oid pubid, Relation relation, List *ancestors, bool pubviaroot); +extern bool replident_has_unpublished_gen_col(Oid pubid, Relation relation, + List *ancestors, bool pubviaroot); #endif /* PUBLICATIONCMDS_H */ diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out index 5de2d64d01..caf6e31554 100644 --- a/src/test/regress/expected/publication.out +++ b/src/test/regress/expected/publication.out @@ -672,6 +672,31 @@ DROP TABLE rf_tbl_abcd_pk; DROP TABLE rf_tbl_abcd_nopk; DROP TABLE rf_tbl_abcd_part_pk; -- ====================================================== +-- ====================================================== +-- test with generated column +SET client_min_messages = 'ERROR'; +CREATE TABLE testpub_gencol (a INT, b INT GENERATED ALWAYS AS (a + 1) STORED NOT NULL); +CREATE UNIQUE INDEX testpub_gencol_idx ON testpub_gencol (b); +ALTER TABLE testpub_gencol REPLICA IDENTITY USING index testpub_gencol_idx; +-- error - generated column "b" is not published but part of index set as REPLICA IDENTITY +CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol; +UPDATE testpub_gencol SET a = 100 WHERE a = 1; +ERROR: cannot update table "testpub_gencol" +DETAIL: Replica identity consists of an unpublished generated column. +-- error - generated column "b" is not published and REPLICA IDENTITY is set FULL +ALTER TABLE testpub_gencol REPLICA IDENTITY FULL; +UPDATE testpub_gencol SET a = 100 WHERE a = 1; +ERROR: cannot update table "testpub_gencol" +DETAIL: Replica identity consists of an unpublished generated column. +DROP PUBLICATION pub_gencol; +-- ok - generated column "b" is published and is part of REPLICA IDENTITY +CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol with (publish_generated_columns = true); +UPDATE testpub_gencol SET a = 100 WHERE a = 1; +DROP PUBLICATION pub_gencol; +DROP INDEX testpub_gencol_idx; +DROP TABLE testpub_gencol; +RESET client_min_messages; +-- ====================================================== -- fail - duplicate tables are not allowed if that table has any column lists SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub_dups FOR TABLE testpub_tbl1 (a), testpub_tbl1 WITH (publish = 'insert'); diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql index 48e68bcca2..3b1c661440 100644 --- a/src/test/regress/sql/publication.sql +++ b/src/test/regress/sql/publication.sql @@ -396,6 +396,32 @@ DROP TABLE rf_tbl_abcd_nopk; DROP TABLE rf_tbl_abcd_part_pk; -- ====================================================== +-- ====================================================== +-- test with generated column +SET client_min_messages = 'ERROR'; +CREATE TABLE testpub_gencol (a INT, b INT GENERATED ALWAYS AS (a + 1) STORED NOT NULL); +CREATE UNIQUE INDEX testpub_gencol_idx ON testpub_gencol (b); +ALTER TABLE testpub_gencol REPLICA IDENTITY USING index testpub_gencol_idx; + +-- error - generated column "b" is not published but part of index set as REPLICA IDENTITY +CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol; +UPDATE testpub_gencol SET a = 100 WHERE a = 1; + +-- error - generated column "b" is not published and REPLICA IDENTITY is set FULL +ALTER TABLE testpub_gencol REPLICA IDENTITY FULL; +UPDATE testpub_gencol SET a = 100 WHERE a = 1; +DROP PUBLICATION pub_gencol; + +-- ok - generated column "b" is published and is part of REPLICA IDENTITY +CREATE PUBLICATION pub_gencol FOR TABLE testpub_gencol with (publish_generated_columns = true); +UPDATE testpub_gencol SET a = 100 WHERE a = 1; +DROP PUBLICATION pub_gencol; + +DROP INDEX testpub_gencol_idx; +DROP TABLE testpub_gencol; +RESET client_min_messages; +-- ====================================================== + -- fail - duplicate tables are not allowed if that table has any column lists SET client_min_messages = 'ERROR'; CREATE PUBLICATION testpub_dups FOR TABLE testpub_tbl1 (a), testpub_tbl1 WITH (publish = 'insert'); diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl index cb36ca7b16..64b902db73 100644 --- a/src/test/subscription/t/100_bugs.pl +++ b/src/test/subscription/t/100_bugs.pl @@ -391,7 +391,7 @@ $node_publisher->safe_psql( ALTER TABLE dropped_cols REPLICA IDENTITY FULL; CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); ALTER TABLE generated_cols REPLICA IDENTITY FULL; - CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols; + CREATE PUBLICATION pub_dropped_cols FOR TABLE dropped_cols, generated_cols with (publish_generated_columns = true); -- some initial data INSERT INTO dropped_cols VALUES (1, 1, 1); INSERT INTO generated_cols (a, c) VALUES (1, 1); @@ -400,7 +400,7 @@ $node_publisher->safe_psql( $node_subscriber->safe_psql( 'postgres', qq( CREATE TABLE dropped_cols (a int, b_drop int, c int); - CREATE TABLE generated_cols (a int, b_gen int GENERATED ALWAYS AS (5 * a) STORED, c int); + CREATE TABLE generated_cols (a int, b_gen int, c int); )); $publisher_connstr = $node_publisher->connstr . ' dbname=postgres'; -- 2.34.1