v44-0005-cache-the-result-of-row-filter-column-validation.patch

From cd7e65a869fdff0efdbb892df0dfed0b1d433f87 Mon Sep 17 00:00:00 2001
From: "houzj.fnst" <houzj.fnst@fujitsu.com>
Date: Thu, 2 Dec 2021 11:31:41 +0800
Subject: [PATCH] cache the result of row filter column validation

For publish mode "delete" "update", validates that any columns referenced
in the filter expression must be part of REPLICA IDENTITY or Primary Key.

Move the row filter columns invalidation to CheckCmdReplicaIdentity, so that
the invalidation is executed only when actual UPDATE or DELETE executed on the
published relation. It's consistent with the existing check about replica
identity.

Cache the results of the validation for row filter columns in relcache to
reduce the cost of the validation. It's safe because every operation that
change the row filter and replica identity will invalidate the relcache.

---
 src/backend/catalog/pg_publication.c      | 104 ++-----------
 src/backend/executor/execReplication.c    |  35 ++++-
 src/backend/utils/cache/relcache.c        | 173 +++++++++++++++++++---
 src/include/utils/rel.h                   |   6 +
 src/include/utils/relcache.h              |   1 +
 src/test/regress/expected/publication.out |  56 ++++---
 src/test/regress/sql/publication.sql      |  40 +++--
 7 files changed, 263 insertions(+), 152 deletions(-)

diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index d67023a440..b9619ef581 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -221,13 +221,6 @@ pg_relation_is_publishable(PG_FUNCTION_ARGS)
 	PG_RETURN_BOOL(result);
 }
 
-/* For rowfilter_walker. */
-typedef struct {
-	Relation	rel;
-	bool		check_replident; /* check if Var is bms_replident member? */
-	Bitmapset  *bms_replident;
-} rf_context;
-
 /*
  * The row filter walker checks that the row filter expression is legal.
  *
@@ -260,15 +253,9 @@ typedef struct {
  * We don't allow anything other than immutable built-in functions because those
  * (not immutable ones) can access database and would lead to the problem (b)
  * mentioned in the previous paragraph.
- *
- * Rules: Replica Identity validation
- * -----------------------------------
- * If the flag context.check_replident is true then validate that every variable
- * referenced by the filter expression is a valid member of the allowed set of
- * replica identity columns (context.bms_replindent)
  */
 static bool
-rowfilter_walker(Node *node, rf_context *context)
+rowfilter_walker(Node *node, Relation relation)
 {
 	char *forbidden = NULL;
 	bool too_complex = false;
@@ -283,25 +270,6 @@ rowfilter_walker(Node *node, rf_context *context)
 		/* User-defined types not allowed. */
 		if (var->vartype >= FirstNormalObjectId)
 			forbidden = _("user-defined types are not allowed");
-
-		/* Optionally, do replica identify validation of the referenced column. */
-		if (context->check_replident)
-		{
-			Oid			relid = RelationGetRelid(context->rel);
-			AttrNumber	attnum = var->varattno;
-
-			if (!bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber, context->bms_replident))
-			{
-				const char *colname = get_attname(relid, attnum, false);
-
-				ereport(ERROR,
-						(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
-						errmsg("cannot add relation \"%s\" to publication",
-							   RelationGetRelationName(context->rel)),
-						errdetail("Row filter column \"%s\" is not part of the REPLICA IDENTITY",
-								  colname)));
-			}
-		}
 	}
 	else if (IsA(node, Const) || IsA(node, BoolExpr) || IsA(node, NullIfExpr)
 			 || IsA(node, NullTest))
@@ -344,74 +312,18 @@ rowfilter_walker(Node *node, rf_context *context)
 	if (too_complex)
 		ereport(ERROR,
 				(errmsg("invalid publication WHERE expression for relation \"%s\"",
-						RelationGetRelationName(context->rel)),
+						RelationGetRelationName(relation)),
 				errhint("only simple expressions using columns, constants and immutable system functions are allowed")
 				));
 
 	if (forbidden)
 		ereport(ERROR,
 				(errmsg("invalid publication WHERE expression for relation \"%s\"",
-						RelationGetRelationName(context->rel)),
+						RelationGetRelationName(relation)),
 						errdetail("%s", forbidden)
 				));
 
-	return expression_tree_walker(node, rowfilter_walker, (void *)context);
-}
-
-/*
- * Check if the row-filter is valid according to the following rules:
- *
- * 1. Only certain simple node types are permitted in the expression. See
- * function rowfilter_walker for details.
- *
- * 2. If the publish operation contains "delete" or "update" then only columns
- * that are allowed by the REPLICA IDENTITY rules are permitted to be used in
- * the row-filter WHERE clause.
- */
-static void
-rowfilter_expr_checker(Publication *pub, Relation rel, Node *rfnode)
-{
-	rf_context	context = {0};
-
-	context.rel = rel;
-
-	/*
-	 * For "delete" or "update", check that filter cols are also valid replica
-	 * identity cols.
-	 */
-	if (pub->pubactions.pubdelete || pub->pubactions.pubupdate)
-	{
-		char replica_identity = rel->rd_rel->relreplident;
-
-		if (replica_identity == REPLICA_IDENTITY_FULL)
-		{
-			/*
-			 * FULL means all cols are in the REPLICA IDENTITY, so all cols are
-			 * allowed in the row-filter too.
-			 */
-		}
-		else
-		{
-			context.check_replident = true;
-
-			/*
-			 * Find what are the cols that are part of the REPLICA IDENTITY.
-			 * Note that REPLICA IDENTIY DEFAULT means primary key or nothing.
-			 */
-			if (replica_identity == REPLICA_IDENTITY_DEFAULT)
-				context.bms_replident = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_PRIMARY_KEY);
-			else
-				context.bms_replident = RelationGetIndexAttrBitmap(rel, INDEX_ATTR_BITMAP_IDENTITY_KEY);
-		}
-	}
-
-	/*
-	 * Walk the parse-tree of this publication row filter expression and throw an
-	 * error if anything not permitted or unexpected is encountered.
-	 */
-	rowfilter_walker(rfnode, &context);
-
-	bms_free(context.bms_replident);
+	return expression_tree_walker(node, rowfilter_walker, (void *) relation);
 }
 
 List *
@@ -525,8 +437,12 @@ publication_add_relation(Oid pubid, PublicationRelInfo *pri,
 		/* Fix up collation information */
 		whereclause = GetTransformedWhereClause(pstate, pri, true);
 
-		/* Validate the row-filter. */
-		rowfilter_expr_checker(pub, targetrel, whereclause);
+		/*
+		 * Walk the parse-tree of this publication row filter expression and
+		 * throw an error if anything not permitted or unexpected is
+		 * encountered.
+		 */
+		rowfilter_walker(whereclause, targetrel);
 	}
 
 	/* Form a tuple. */
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 574d7d27fd..0dcc9dfe68 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -568,14 +568,45 @@ void
 CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
 {
 	PublicationActions *pubactions;
+	AttrNumber			invalid_rfcol;
 
 	/* We only need to do checks for UPDATE and DELETE. */
 	if (cmd != CMD_UPDATE && cmd != CMD_DELETE)
 		return;
 
+	if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL)
+		return;
+
+	invalid_rfcol = RelationGetInvalRowFilterCol(rel);
+
+	/*
+	 * It is only safe to execute UPDATE/DELETE when all columns of the row
+	 * filters from publications which the relation is in are part of the
+	 * REPLICA IDENTITY.
+	 */
+	if (invalid_rfcol != InvalidAttrNumber)
+	{
+		const char *colname = get_attname(RelationGetRelid(rel),
+										  invalid_rfcol, false);
+
+		if (cmd == CMD_UPDATE)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					 errmsg("cannot update table \"%s\"",
+							RelationGetRelationName(rel)),
+					 errdetail("Row filter column \"%s\" is not part of the REPLICA IDENTITY",
+							   colname)));
+		else if (cmd == CMD_DELETE)
+			ereport(ERROR,
+					(errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					 errmsg("cannot delete from table \"%s\"",
+							RelationGetRelationName(rel)),
+					 errdetail("Row filter column \"%s\" is not part of the REPLICA IDENTITY",
+							   colname)));
+	}
+
 	/* If relation has replica identity we are always good. */
-	if (rel->rd_rel->relreplident == REPLICA_IDENTITY_FULL ||
-		OidIsValid(RelationGetReplicaIndex(rel)))
+	if (OidIsValid(RelationGetReplicaIndex(rel)))
 		return;
 
 	/*
diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
index e1ea079e9e..3d4efa802c 100644
--- a/src/backend/utils/cache/relcache.c
+++ b/src/backend/utils/cache/relcache.c
@@ -56,6 +56,7 @@
 #include "catalog/pg_opclass.h"
 #include "catalog/pg_proc.h"
 #include "catalog/pg_publication.h"
+#include "catalog/pg_publication_rel.h"
 #include "catalog/pg_rewrite.h"
 #include "catalog/pg_shseclabel.h"
 #include "catalog/pg_statistic_ext.h"
@@ -5548,28 +5549,69 @@ RelationGetExclusionInfo(Relation indexRelation,
 	MemoryContextSwitchTo(oldcxt);
 }
 
+/* For invalid_rowfilter_column_walker. */
+typedef struct {
+	AttrNumber	invalid_rfcol;
+	Bitmapset  *bms_replident;
+} rf_context;
+
 /*
- * Get publication actions for the given relation.
+ * Check if any columns used in the row-filter WHERE clause are not part of
+ * REPLICA IDENTITY and save the invalid column number in
+ * rf_context::invalid_rfcol.
  */
-struct PublicationActions *
-GetRelationPublicationActions(Relation relation)
+static bool
+invalid_rowfilter_column_walker(Node *node, rf_context *context)
 {
-	List	   *puboids;
-	ListCell   *lc;
-	MemoryContext oldcxt;
-	Oid			schemaid;
-	PublicationActions *pubactions = palloc0(sizeof(PublicationActions));
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, Var))
+	{
+		Var		   *var = (Var *) node;
+		AttrNumber	attnum = var->varattno;
+
+		if (!bms_is_member(attnum - FirstLowInvalidHeapAttributeNumber,
+						   context->bms_replident))
+		{
+			context->invalid_rfcol = attnum;
+			return true;
+		}
+	}
+
+	return expression_tree_walker(node, invalid_rowfilter_column_walker,
+								  (void *) context);
+}
+
+/*
+ * Get the invalid row filter column number for the given relation.
+ *
+ * Traverse all the publications which the relation is in to get the
+ * publication actions. If the publication actions include UPDATE or DELETE,
+ * then validate that if all columns referenced in the row filter expression
+ * are part of REPLICA IDENTITY.
+ *
+ * If not all the row filter columns are part of REPLICA IDENTITY, return the
+ * invalid column number, InvalidAttrNumber otherwise.
+ */
+AttrNumber
+RelationGetInvalRowFilterCol(Relation relation)
+{
+	List		   *puboids;
+	ListCell	   *lc;
+	MemoryContext	oldcxt;
+	Oid				schemaid;
+	rf_context		context = { 0 };
+	PublicationActions pubactions = { 0 };
+	bool			rfcol_valid = true;
+	AttrNumber		invalid_rfcol = InvalidAttrNumber;
 
 	/*
 	 * If not publishable, it publishes no actions.  (pgoutput_change() will
 	 * ignore it.)
 	 */
-	if (!is_publishable_relation(relation))
-		return pubactions;
-
-	if (relation->rd_pubactions)
-		return memcpy(pubactions, relation->rd_pubactions,
-					  sizeof(PublicationActions));
+	if (!is_publishable_relation(relation) || relation->rd_rfcol_valid)
+		return invalid_rfcol;
 
 	/* Fetch the publication membership info. */
 	puboids = GetRelationPublications(RelationGetRelid(relation));
@@ -5595,10 +5637,22 @@ GetRelationPublicationActions(Relation relation)
 	}
 	puboids = list_concat_unique_oid(puboids, GetAllTablesPublications());
 
+	/*
+	 * Find what are the cols that are part of the REPLICA IDENTITY.
+	 * Note that REPLICA IDENTITY DEFAULT means primary key or nothing.
+	 */
+	if (relation->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT)
+		context.bms_replident = RelationGetIndexAttrBitmap(relation,
+														   INDEX_ATTR_BITMAP_PRIMARY_KEY);
+	else if (relation->rd_rel->relreplident == REPLICA_IDENTITY_INDEX)
+		context.bms_replident = RelationGetIndexAttrBitmap(relation,
+														   INDEX_ATTR_BITMAP_IDENTITY_KEY);
+
 	foreach(lc, puboids)
 	{
 		Oid			pubid = lfirst_oid(lc);
 		HeapTuple	tup;
+
 		Form_pg_publication pubform;
 
 		tup = SearchSysCache1(PUBLICATIONOID, ObjectIdGetDatum(pubid));
@@ -5608,35 +5662,105 @@ GetRelationPublicationActions(Relation relation)
 
 		pubform = (Form_pg_publication) GETSTRUCT(tup);
 
-		pubactions->pubinsert |= pubform->pubinsert;
-		pubactions->pubupdate |= pubform->pubupdate;
-		pubactions->pubdelete |= pubform->pubdelete;
-		pubactions->pubtruncate |= pubform->pubtruncate;
+		pubactions.pubinsert |= pubform->pubinsert;
+		pubactions.pubupdate |= pubform->pubupdate;
+		pubactions.pubdelete |= pubform->pubdelete;
+		pubactions.pubtruncate |= pubform->pubtruncate;
 
 		ReleaseSysCache(tup);
 
 		/*
-		 * If we know everything is replicated, there is no point to check for
-		 * other publications.
+		 * If the publication action include UDDATE and DELETE, validates
+		 * that any columns referenced in the filter expression are part of
+		 * REPLICA IDENTITY index.
+		 *
+		 * FULL means all cols are in the REPLICA IDENTITY, so all cols are
+		 * allowed in the row-filter and we can skip the validation.
+		 *
+		 * If we already found the column in row filter which is not part
+		 * of REPLICA IDENTITY index, skip the validation too.
 		 */
-		if (pubactions->pubinsert && pubactions->pubupdate &&
-			pubactions->pubdelete && pubactions->pubtruncate)
+		if ((pubform->pubupdate || pubform->pubdelete) &&
+			relation->rd_rel->relreplident != REPLICA_IDENTITY_FULL &&
+			rfcol_valid)
+		{
+			HeapTuple	rftuple;
+
+			rftuple = SearchSysCache2(PUBLICATIONRELMAP,
+									  ObjectIdGetDatum(RelationGetRelid(relation)),
+									  ObjectIdGetDatum(pubid));
+
+			if (HeapTupleIsValid(rftuple))
+			{
+				Datum		rfdatum;
+				bool		rfisnull;
+				Node	   *rfnode;
+
+				rfdatum = SysCacheGetAttr(PUBLICATIONRELMAP, rftuple,
+										  Anum_pg_publication_rel_prqual,
+										  &rfisnull);
+
+				if (!rfisnull)
+				{
+					rfnode = stringToNode(TextDatumGetCString(rfdatum));
+					rfcol_valid = !invalid_rowfilter_column_walker(rfnode,
+																   &context);
+					invalid_rfcol = context.invalid_rfcol;
+				}
+
+				ReleaseSysCache(rftuple);
+			}
+		}
+
+		/*
+		 * If we know everything is replicated and some columns are not part of
+		 * replica identity, there is no point to check for other publications.
+		 */
+		if (pubactions.pubinsert && pubactions.pubupdate &&
+			pubactions.pubdelete && pubactions.pubtruncate &&
+			!rfcol_valid)
 			break;
 	}
 
+	bms_free(context.bms_replident);
+
 	if (relation->rd_pubactions)
 	{
 		pfree(relation->rd_pubactions);
 		relation->rd_pubactions = NULL;
 	}
 
+	relation->rd_rfcol_valid = rfcol_valid;
+
 	/* Now save copy of the actions in the relcache entry. */
 	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
 	relation->rd_pubactions = palloc(sizeof(PublicationActions));
-	memcpy(relation->rd_pubactions, pubactions, sizeof(PublicationActions));
+	memcpy(relation->rd_pubactions, &pubactions, sizeof(PublicationActions));
 	MemoryContextSwitchTo(oldcxt);
 
-	return pubactions;
+	return invalid_rfcol;
+}
+
+/*
+ * Get publication actions for the given relation.
+ */
+struct PublicationActions *
+GetRelationPublicationActions(Relation relation)
+{
+	PublicationActions *pubactions = palloc0(sizeof(PublicationActions));
+
+	/*
+	 * If not publishable, it publishes no actions.  (pgoutput_change() will
+	 * ignore it.)
+	 */
+	if (!is_publishable_relation(relation))
+		return pubactions;
+
+	if (!relation->rd_pubactions)
+		(void) RelationGetInvalRowFilterCol(relation);
+
+	return memcpy(pubactions, relation->rd_pubactions,
+				  sizeof(PublicationActions));
 }
 
 /*
@@ -6193,6 +6317,7 @@ load_relcache_init_file(bool shared)
 		rel->rd_idattr = NULL;
 		rel->rd_hotblockingattr = NULL;
 		rel->rd_pubactions = NULL;
+		rel->rd_rfcol_valid = false;
 		rel->rd_statvalid = false;
 		rel->rd_statlist = NIL;
 		rel->rd_fkeyvalid = false;
diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h
index 31281279cf..84c58f9fe2 100644
--- a/src/include/utils/rel.h
+++ b/src/include/utils/rel.h
@@ -163,6 +163,12 @@ typedef struct RelationData
 
 	PublicationActions *rd_pubactions;	/* publication actions */
 
+	/*
+	 * true if the columns of row filters from all the publications the
+	 * relation is in are part of replica identity.
+	 */
+	bool		rd_rfcol_valid;
+
 	/*
 	 * rd_options is set whenever rd_rel is loaded into the relcache entry.
 	 * Note that you can NOT look into rd_rel for this data.  NULL means "use
diff --git a/src/include/utils/relcache.h b/src/include/utils/relcache.h
index 82316bba54..1f091af904 100644
--- a/src/include/utils/relcache.h
+++ b/src/include/utils/relcache.h
@@ -76,6 +76,7 @@ extern void RelationInitIndexAccessInfo(Relation relation);
 /* caller must include pg_publication.h */
 struct PublicationActions;
 extern struct PublicationActions *GetRelationPublicationActions(Relation relation);
+extern AttrNumber RelationGetInvalRowFilterCol(Relation relation);
 
 extern void RelationInitTableAccessMethod(Relation relation);
 
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index d9ee9ff645..5affa973aa 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -426,21 +426,27 @@ DROP PUBLICATION testpub6;
 -- ok - "b" is a PK col
 SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (b > 99);
-RESET client_min_messages;
 DROP PUBLICATION testpub6;
--- fail - "c" is not part of the PK
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_pk" to publication
+-- fail - "c" is not part of the PK
+UPDATE rf_tbl_abcd_pk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_pk"
 DETAIL:  Row filter column "c" is not part of the REPLICA IDENTITY
--- fail - "d" is not part of the PK
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (d > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_pk" to publication
+-- fail - "d" is not part of the PK
+UPDATE rf_tbl_abcd_pk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_pk"
 DETAIL:  Row filter column "d" is not part of the REPLICA IDENTITY
+DROP PUBLICATION testpub6;
 -- 1b. REPLICA IDENTITY is DEFAULT and table has no PK
--- fail - "a" is not part of REPLICA IDENTITY
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_nopk" to publication
+-- fail - "a" is not part of REPLICA IDENTITY
+UPDATE rf_tbl_abcd_nopk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_nopk"
 DETAIL:  Row filter column "a" is not part of the REPLICA IDENTITY
+DROP PUBLICATION testpub6;
+RESET client_min_messages;
 -- Case 2. REPLICA IDENTITY FULL
 ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY FULL;
 ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY FULL;
@@ -454,21 +460,29 @@ SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
 DROP PUBLICATION testpub6;
 RESET client_min_messages;
+SET client_min_messages = 'ERROR';
 -- Case 3. REPLICA IDENTITY NOTHING
 ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY NOTHING;
 ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY NOTHING;
--- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_pk" to publication
+-- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_pk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_pk"
 DETAIL:  Row filter column "a" is not part of the REPLICA IDENTITY
--- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_pk" to publication
+-- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_pk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_pk"
 DETAIL:  Row filter column "c" is not part of the REPLICA IDENTITY
--- fail - "a" is not in REPLICA IDENTITY NOTHING
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_nopk" to publication
+-- fail - "a" is not in REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_nopk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_nopk"
 DETAIL:  Row filter column "a" is not part of the REPLICA IDENTITY
+DROP PUBLICATION testpub6;
+RESET client_min_messages;
 -- Case 4. REPLICA IDENTITY INDEX
 ALTER TABLE rf_tbl_abcd_pk ALTER COLUMN c SET NOT NULL;
 CREATE UNIQUE INDEX idx_abcd_pk_c ON rf_tbl_abcd_pk(c);
@@ -476,21 +490,23 @@ ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY USING INDEX idx_abcd_pk_c;
 ALTER TABLE rf_tbl_abcd_nopk ALTER COLUMN c SET NOT NULL;
 CREATE UNIQUE INDEX idx_abcd_nopk_c ON rf_tbl_abcd_nopk(c);
 ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY USING INDEX idx_abcd_nopk_c;
--- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX
+SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_pk" to publication
+-- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX
+update rf_tbl_abcd_pk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_pk"
 DETAIL:  Row filter column "a" is not part of the REPLICA IDENTITY
+DROP PUBLICATION testpub6;
 -- ok - "c" is not in PK but it is part of REPLICA IDENTITY INDEX
-SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
 DROP PUBLICATION testpub6;
-RESET client_min_messages;
--- fail - "a" is not in REPLICA IDENTITY INDEX
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
-ERROR:  cannot add relation "rf_tbl_abcd_nopk" to publication
+-- fail - "a" is not in REPLICA IDENTITY INDEX
+update rf_tbl_abcd_nopk set a = 1;
+ERROR:  cannot update table "rf_tbl_abcd_nopk"
 DETAIL:  Row filter column "a" is not part of the REPLICA IDENTITY
+DROP PUBLICATION testpub6;
 -- ok - "c" is part of REPLICA IDENTITY INDEX
-SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (c > 99);
 DROP PUBLICATION testpub6;
 RESET client_min_messages;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index fcc09b1c23..559bc267f0 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -245,15 +245,21 @@ DROP PUBLICATION testpub6;
 -- ok - "b" is a PK col
 SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (b > 99);
-RESET client_min_messages;
 DROP PUBLICATION testpub6;
--- fail - "c" is not part of the PK
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
--- fail - "d" is not part of the PK
+-- fail - "c" is not part of the PK
+UPDATE rf_tbl_abcd_pk set a = 1;
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (d > 99);
+-- fail - "d" is not part of the PK
+UPDATE rf_tbl_abcd_pk set a = 1;
+DROP PUBLICATION testpub6;
 -- 1b. REPLICA IDENTITY is DEFAULT and table has no PK
--- fail - "a" is not part of REPLICA IDENTITY
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
+-- fail - "a" is not part of REPLICA IDENTITY
+UPDATE rf_tbl_abcd_nopk set a = 1;
+DROP PUBLICATION testpub6;
+RESET client_min_messages;
 
 -- Case 2. REPLICA IDENTITY FULL
 ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY FULL;
@@ -269,15 +275,23 @@ CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
 DROP PUBLICATION testpub6;
 RESET client_min_messages;
 
+SET client_min_messages = 'ERROR';
 -- Case 3. REPLICA IDENTITY NOTHING
 ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY NOTHING;
 ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY NOTHING;
--- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99);
--- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING
+-- fail - "a" is in PK but it is not part of REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_pk set a = 1;
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
--- fail - "a" is not in REPLICA IDENTITY NOTHING
+-- fail - "c" is not in PK and not in REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_pk set a = 1;
+DROP PUBLICATION testpub6;
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
+-- fail - "a" is not in REPLICA IDENTITY NOTHING
+UPDATE rf_tbl_abcd_nopk set a = 1;
+DROP PUBLICATION testpub6;
+RESET client_min_messages;
 
 -- Case 4. REPLICA IDENTITY INDEX
 ALTER TABLE rf_tbl_abcd_pk ALTER COLUMN c SET NOT NULL;
@@ -286,17 +300,19 @@ ALTER TABLE rf_tbl_abcd_pk REPLICA IDENTITY USING INDEX idx_abcd_pk_c;
 ALTER TABLE rf_tbl_abcd_nopk ALTER COLUMN c SET NOT NULL;
 CREATE UNIQUE INDEX idx_abcd_nopk_c ON rf_tbl_abcd_nopk(c);
 ALTER TABLE rf_tbl_abcd_nopk REPLICA IDENTITY USING INDEX idx_abcd_nopk_c;
--- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX
+SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (a > 99);
+-- fail - "a" is in PK but it is not part of REPLICA IDENTITY INDEX
+update rf_tbl_abcd_pk set a = 1;
+DROP PUBLICATION testpub6;
 -- ok - "c" is not in PK but it is part of REPLICA IDENTITY INDEX
-SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_pk WHERE (c > 99);
 DROP PUBLICATION testpub6;
-RESET client_min_messages;
--- fail - "a" is not in REPLICA IDENTITY INDEX
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (a > 99);
+-- fail - "a" is not in REPLICA IDENTITY INDEX
+update rf_tbl_abcd_nopk set a = 1;
+DROP PUBLICATION testpub6;
 -- ok - "c" is part of REPLICA IDENTITY INDEX
-SET client_min_messages = 'ERROR';
 CREATE PUBLICATION testpub6 FOR TABLE rf_tbl_abcd_nopk WHERE (c > 99);
 DROP PUBLICATION testpub6;
 RESET client_min_messages;
-- 
2.18.4