From 528fe3064fe2ca1d40df39564748c11a8cfc2e7c Mon Sep 17 00:00:00 2001
From: Shubham Khanna <shubham.khanna@fujitsu.com>
Date: Thu, 7 Nov 2024 15:54:19 +0530
Subject: [PATCH v5] Error-message-improvement

Currently, if logical replication attempts to target a subscriber-side table
column that is either missing or generated, it produces the following
identical error message:
ERROR: logical replication target relation \"%s.%s\" is missing
replicated columns: %s

While the error itself is valid, the message wording can be misleading for
generated columns. This patch introduces a distinct error message specifically
for the generated column scenario.
---
 src/backend/replication/logical/relation.c | 98 +++++++++++++++-------
 src/test/subscription/t/011_generated.pl   | 83 ++++++++++++++++++
 2 files changed, 150 insertions(+), 31 deletions(-)

diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index f5a0ef2bd9..cdce752440 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -220,41 +220,63 @@ logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
 }
 
 /*
- * Report error with names of the missing local relation column(s), if any.
+ * Generates a comma-separated string of attribute names based on the provided
+ * relation information and a bitmap indicating which attributes are included.
+ *
+ * The result is a palloc'd string.
  */
-static void
-logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
-								Bitmapset *missingatts)
+static char *
+get_attrs_str(LogicalRepRelation *remoterel, Bitmapset *atts)
 {
-	if (!bms_is_empty(missingatts))
+	StringInfoData attsbuf;
+	int			attcnt = 0;
+	int			i = -1;
+
+	Assert(!bms_is_empty(atts));
+
+	initStringInfo(&attsbuf);
+
+	while ((i = bms_next_member(atts, i)) >= 0)
 	{
-		StringInfoData missingattsbuf;
-		int			missingattcnt = 0;
-		int			i;
+		attcnt++;
+		if (attcnt > 1)
+			appendStringInfo(&attsbuf, _(", "));
 
-		initStringInfo(&missingattsbuf);
+		appendStringInfo(&attsbuf, _("\"%s\""), remoterel->attnames[i]);
+	}
 
-		i = -1;
-		while ((i = bms_next_member(missingatts, i)) >= 0)
-		{
-			missingattcnt++;
-			if (missingattcnt == 1)
-				appendStringInfo(&missingattsbuf, _("\"%s\""),
-								 remoterel->attnames[i]);
-			else
-				appendStringInfo(&missingattsbuf, _(", \"%s\""),
-								 remoterel->attnames[i]);
-		}
+	return attsbuf.data;
+}
 
+/*
+ * If !bms_is_empty(missingatts), report the error message as 'Missing
+ * replicated columns.' Otherwise, report the error message as 'Cannot replicate
+ * to generated columns.'
+ */
+static void
+logicalrep_report_missing_and_gen_attrs(LogicalRepRelation *remoterel,
+										Bitmapset *missingatts,
+										Bitmapset *genatts)
+{
+	if (!bms_is_empty(missingatts))
 		ereport(ERROR,
-				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
-				 errmsg_plural("logical replication target relation \"%s.%s\" is missing replicated column: %s",
-							   "logical replication target relation \"%s.%s\" is missing replicated columns: %s",
-							   missingattcnt,
-							   remoterel->nspname,
-							   remoterel->relname,
-							   missingattsbuf.data)));
-	}
+				errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				errmsg_plural("logical replication target relation \"%s.%s\" is missing replicated column: %s",
+							  "logical replication target relation \"%s.%s\" is missing replicated columns: %s",
+							  bms_num_members(missingatts),
+							  remoterel->nspname,
+							  remoterel->relname,
+							  get_attrs_str(remoterel, missingatts)));
+
+	if (!bms_is_empty(genatts))
+		ereport(ERROR,
+				errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+				errmsg_plural("cannot replicate to target relation \"%s.%s\" generated column: %s",
+							  "cannot replicate to target relation \"%s.%s\" generated columns: %s",
+							  bms_num_members(genatts),
+							  remoterel->nspname,
+							  remoterel->relname,
+							  get_attrs_str(remoterel, genatts)));
 }
 
 /*
@@ -379,7 +401,9 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 		TupleDesc	desc;
 		MemoryContext oldctx;
 		int			i;
-		Bitmapset  *missingatts;
+		Bitmapset  *missingatts;	/* Bitmapset for missing attributes. */
+		Bitmapset  *generatedattrs = NULL;	/* Bitmapset for generated
+											 * attributes. */
 
 		/* Release the no-longer-useful attrmap, if any. */
 		if (entry->attrmap)
@@ -421,7 +445,7 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 			int			attnum;
 			Form_pg_attribute attr = TupleDescAttr(desc, i);
 
-			if (attr->attisdropped || attr->attgenerated)
+			if (attr->attisdropped)
 			{
 				entry->attrmap->attnums[i] = -1;
 				continue;
@@ -432,12 +456,24 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 
 			entry->attrmap->attnums[i] = attnum;
 			if (attnum >= 0)
+			{
+				/* Remember which subscriber columns are generated. */
+				if (attr->attgenerated)
+					generatedattrs = bms_add_member(generatedattrs, attnum);
+
 				missingatts = bms_del_member(missingatts, attnum);
+			}
 		}
 
-		logicalrep_report_missing_attrs(remoterel, missingatts);
+		/*
+		 * Report any missing and generated columns. Note, if there are both
+		 * kinds then the 'missing' error takes precedence.
+		 */
+		logicalrep_report_missing_and_gen_attrs(remoterel, missingatts,
+												generatedattrs);
 
 		/* be tidy */
+		bms_free(generatedattrs);
 		bms_free(missingatts);
 
 		/*
diff --git a/src/test/subscription/t/011_generated.pl b/src/test/subscription/t/011_generated.pl
index 211b54c316..977e3b1fd4 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -326,4 +326,87 @@ is( $result, qq(|2|
 $node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
 $node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
 
+# =============================================================================
+# The following tests for expected errors when attempting to replicate to a
+# missing or a generated subscriber column. Test the following combinations
+# - regular -> generated
+# - generated -> generated
+# - regular -> missing
+# - generated -> missing
+# - test if both errors exist, then the missing error is reported first.
+# =============================================================================
+
+# --------------------------------------------------
+# A "regular -> generated" or "generated -> generated" replication fails,
+# reporting an error that the generated column on the subscriber side
+# cannot be replicated.
+#
+# Test Case: regular -> generated and generated -> generated
+# Publisher table has regular column 'c2' and generated column 'c3'.
+# Subscriber table has generated columns 'c2' and 'c3'.
+# --------------------------------------------------
+
+# Create table and publication. Insert data into the table.
+$node_publisher->safe_psql(
+	'postgres', qq(
+	CREATE TABLE t1(c1 int, c2 int, c3 int GENERATED ALWAYS AS (c1 * 2) STORED);
+	CREATE PUBLICATION pub1 for table t1(c1, c2, c3);
+	INSERT INTO t1 VALUES (1);
+));
+
+# Create table and subscription.
+$node_subscriber->safe_psql(
+	'postgres', qq(
+	CREATE TABLE t1(c1 int, c2 int GENERATED ALWAYS AS (c1 + 2) STORED, c3 int GENERATED ALWAYS AS (c1 + 2) STORED);
+	CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+# Verify that an error occurs.
+my $offset = -s $node_subscriber->logfile;
+$node_subscriber->wait_for_log(
+	qr/ERROR: ( [A-Z0-9]:)? cannot replicate to target relation "public.t1" generated columns: "c2", "c3"/,
+	$offset);
+
+# cleanup
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+
+# --------------------------------------------------
+# A "regular -> missing" or "generated -> missing" replication fails,
+# reporting an error that the subscriber side is missing replicated columns.
+#
+# Testcase: regular -> missing and generated -> missing
+# Publisher table has regular column 'c4' and generated column 'c5'.
+# Subscriber table is missing columns 'c4' and 'c5'.
+#
+# Notice how the first 3 columns of t2 are identical to the previous test,
+# so this table also has generated column errors, but they are not reported
+# because the 'missing' column error takes precedence.
+# --------------------------------------------------
+
+# Create table and publication. Insert data into the table.
+$node_publisher->safe_psql(
+	'postgres', qq(
+	CREATE TABLE t2(c1 int, c2 int, c3 int GENERATED ALWAYS AS (c1 * 2) STORED, c4 int, c5 int GENERATED ALWAYS AS (c1 * 2) STORED);
+	CREATE PUBLICATION pub1 for table t2(c1, c2, c3, c4, c5);
+	INSERT INTO t2 VALUES (1);
+));
+
+# Create table and subscription.
+$node_subscriber->safe_psql(
+	'postgres', qq(
+	CREATE TABLE t2(c1 int, c2 int GENERATED ALWAYS AS (c1 + 2) STORED, c3 int GENERATED ALWAYS AS (c1 + 2) STORED);
+	CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1;
+));
+
+# Verify that an error occurs.
+$offset = -s $node_subscriber->logfile;
+$node_subscriber->wait_for_log(
+	qr/ERROR: ( [A-Z0-9]:)? logical replication target relation "public.t2" is missing replicated columns: "c4", "c5"/,
+	$offset);
+
+# cleanup
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub1");
+$node_publisher->safe_psql('postgres', "DROP PUBLICATION pub1");
+
 done_testing();
-- 
2.34.1

