From 711276ecf304e5b54098211051009b18dfb5e210 Mon Sep 17 00:00:00 2001
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Thu, 20 Jun 2024 15:34:40 +0530
Subject: [PATCH v9 2/3] Support replication of generated column during initial
 sync

When 'copy_data' is true, during the initial sync, the data is replicated from
the publisher to the subscriber using the COPY command. The normal COPY
command does not copy generated columns, so when 'include_generated_columns'
is true, we need to copy using the syntax:
'COPY (SELECT column_name FROM table_name) TO STDOUT'.

Note that we don't copy columns when the subscriber-side column is also
generated. Those will be filled as normal with the subscriber-side computed or
default data.
---
 doc/src/sgml/ref/create_subscription.sgml   |   4 -
 src/backend/commands/subscriptioncmds.c     |  14 ---
 src/backend/replication/logical/relation.c  |   5 +-
 src/backend/replication/logical/tablesync.c | 111 +++++++++++++++-----
 src/include/replication/logicalrelation.h   |   3 +-
 src/test/regress/expected/subscription.out  |   3 -
 src/test/regress/sql/subscription.sql       |   3 -
 src/test/subscription/t/011_generated.pl    |  81 ++++++++++++--
 8 files changed, 163 insertions(+), 61 deletions(-)

diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index e8779dc6ff..b62c0ca931 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -443,10 +443,6 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
           has no effect; the subscriber column will be filled as normal with the
           subscriber-side computed or default data.
          </para>
-         <para>
-         This parameter can only be set <literal>true</literal> if <literal>copy_data</literal> is
-         set to <literal>false</literal>.
-         </para>
         </listitem>
        </varlistentry>
       </variablelist></para>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 7abc06b89a..44a0a43cb8 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -459,20 +459,6 @@ parse_subscription_options(ParseState *pstate, List *stmt_options,
 								"slot_name = NONE", "create_slot = false")));
 		}
 	}
-
-	/*
-	 * Do additional checking for disallowed combination when copy_data and
-	 * include_generated_columns are true. COPY of generated columns is not supported
-	 * yet.
-	 */
-	if (opts->copy_data && opts->include_generated_columns)
-	{
-		ereport(ERROR,
-				errcode(ERRCODE_SYNTAX_ERROR),
-		/*- translator: both %s are strings of the form "option = value" */
-					errmsg("%s and %s are mutually exclusive options",
-						"copy_data = true", "include_generated_columns = true"));
-	}
 }
 
 /*
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 5de1531567..9365dfae1b 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -205,7 +205,7 @@ logicalrep_relmap_update(LogicalRepRelation *remoterel)
  *
  * Returns -1 if not found.
  */
-static int
+int
 logicalrep_rel_att_by_name(LogicalRepRelation *remoterel, const char *attname)
 {
 	int			i;
@@ -421,7 +421,8 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 			int			attnum;
 			Form_pg_attribute attr = TupleDescAttr(desc, i);
 
-			if (attr->attisdropped)
+			if (attr->attisdropped ||
+				(!MySubscription->includegencols && attr->attgenerated))
 			{
 				entry->attrmap->attnums[i] = -1;
 				continue;
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index b00267f042..0fbf661ab0 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -118,6 +118,7 @@
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
+#include "utils/rel.h"
 #include "utils/rls.h"
 #include "utils/snapmgr.h"
 #include "utils/syscache.h"
@@ -692,20 +693,59 @@ process_syncing_tables(XLogRecPtr current_lsn)
 }
 
 /*
- * Create list of columns for COPY based on logical relation mapping.
+ * Create list of columns for COPY based on logical relation mapping. Do not
+ * include generated columns, of the subscription table, in the column list.
  */
 static List *
-make_copy_attnamelist(LogicalRepRelMapEntry *rel)
+make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool *attgenlist)
 {
 	List	   *attnamelist = NIL;
-	int			i;
+	bool	   *gencollist;
+	TupleDesc	desc;
 
-	for (i = 0; i < rel->remoterel.natts; i++)
+	desc = RelationGetDescr(rel->localrel);
+	gencollist = palloc0(MaxTupleAttributeNumber * sizeof(bool));
+
+	for (int i = 0; i < desc->natts; i++)
 	{
-		attnamelist = lappend(attnamelist,
-							  makeString(rel->remoterel.attnames[i]));
+		int			attnum;
+		Form_pg_attribute attr = TupleDescAttr(desc, i);
+
+		if (!attr->attgenerated)
+			continue;
+
+		attnum = logicalrep_rel_att_by_name(&rel->remoterel,
+											NameStr(attr->attname));
+
+		/*
+		 * Check if subscription table have a generated column with same
+		 * column name as a non-generated column in the corresponding
+		 * publication table.
+		 * 'gencollist' stores column if it is a generated column in
+		 * subscription table. We should not include this column in column
+		 * list for COPY.
+		 */
+		if (attnum >= 0)
+		{
+			if(!attgenlist[attnum])
+				ereport(ERROR,
+						(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+						 errmsg("logical replication target relation \"%s.%s\" has a generated column \"%s\" "
+								"but corresponding column on source relation is not a generated column",
+						 rel->remoterel.nspname, rel->remoterel.relname, NameStr(attr->attname))));
+			gencollist[attnum] = true;
+		}
 	}
 
+	/*
+	 * Construct column list for COPY.
+	 */
+	for (int i = 0; i < rel->remoterel.natts; i++)
+	{
+		if(!gencollist[i])
+			attnamelist = lappend(attnamelist,
+								  makeString(rel->remoterel.attnames[i]));
+	}
 
 	return attnamelist;
 }
@@ -791,16 +831,17 @@ copy_read_data(void *outbuf, int minread, int maxread)
  * qualifications to be used in the COPY command.
  */
 static void
-fetch_remote_table_info(char *nspname, char *relname,
+fetch_remote_table_info(char *nspname, char *relname, bool **attgenlist,
 						LogicalRepRelation *lrel, List **qual)
 {
 	WalRcvExecResult *res;
 	StringInfoData cmd;
 	TupleTableSlot *slot;
 	Oid			tableRow[] = {OIDOID, CHAROID, CHAROID};
-	Oid			attrRow[] = {INT2OID, TEXTOID, OIDOID, BOOLOID};
+	Oid			attrRow[] = {INT2OID, TEXTOID, OIDOID, BOOLOID, BOOLOID};
 	Oid			qualRow[] = {TEXTOID};
 	bool		isnull;
+	bool	   *attgenlist_res;
 	int			natt;
 	ListCell   *lc;
 	Bitmapset  *included_cols = NULL;
@@ -948,18 +989,24 @@ fetch_remote_table_info(char *nspname, char *relname,
 					 "SELECT a.attnum,"
 					 "       a.attname,"
 					 "       a.atttypid,"
-					 "       a.attnum = ANY(i.indkey)"
+					 "       a.attnum = ANY(i.indkey),"
+					 "		 a.attgenerated != ''"
 					 "  FROM pg_catalog.pg_attribute a"
 					 "  LEFT JOIN pg_catalog.pg_index i"
 					 "       ON (i.indexrelid = pg_get_replica_identity_index(%u))"
 					 " WHERE a.attnum > 0::pg_catalog.int2"
-					 "   AND NOT a.attisdropped %s"
+					 "   AND NOT a.attisdropped", lrel->remoteid);
+
+	if ((walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000 &&
+		walrcv_server_version(LogRepWorkerWalRcvConn) <= 160000) ||
+		!MySubscription->includegencols)
+			appendStringInfo(&cmd, " AND a.attgenerated = ''");
+
+	appendStringInfo(&cmd,
 					 "   AND a.attrelid = %u"
 					 " ORDER BY a.attnum",
-					 lrel->remoteid,
-					 (walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000 ?
-					  "AND a.attgenerated = ''" : ""),
 					 lrel->remoteid);
+
 	res = walrcv_exec(LogRepWorkerWalRcvConn, cmd.data,
 					  lengthof(attrRow), attrRow);
 
@@ -973,6 +1020,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 	lrel->attnames = palloc0(MaxTupleAttributeNumber * sizeof(char *));
 	lrel->atttyps = palloc0(MaxTupleAttributeNumber * sizeof(Oid));
 	lrel->attkeys = NULL;
+	attgenlist_res = palloc0(MaxTupleAttributeNumber * sizeof(bool));
 
 	/*
 	 * Store the columns as a list of names.  Ignore those that are not
@@ -1005,6 +1053,8 @@ fetch_remote_table_info(char *nspname, char *relname,
 		if (DatumGetBool(slot_getattr(slot, 4, &isnull)))
 			lrel->attkeys = bms_add_member(lrel->attkeys, natt);
 
+		attgenlist_res[natt] = DatumGetBool(slot_getattr(slot, 5, &isnull));
+
 		/* Should never happen. */
 		if (++natt >= MaxTupleAttributeNumber)
 			elog(ERROR, "too many columns in remote table \"%s.%s\"",
@@ -1015,7 +1065,7 @@ fetch_remote_table_info(char *nspname, char *relname,
 	ExecDropSingleTupleTableSlot(slot);
 
 	lrel->natts = natt;
-
+	*attgenlist = attgenlist_res;
 	walrcv_clear_result(res);
 
 	/*
@@ -1123,10 +1173,12 @@ copy_table(Relation rel)
 	List	   *attnamelist;
 	ParseState *pstate;
 	List	   *options = NIL;
+	bool 	   *attgenlist;
 
 	/* Get the publisher relation info. */
 	fetch_remote_table_info(get_namespace_name(RelationGetNamespace(rel)),
-							RelationGetRelationName(rel), &lrel, &qual);
+							RelationGetRelationName(rel), &attgenlist,
+							&lrel, &qual);
 
 	/* Put the relation into relmap. */
 	logicalrep_relmap_update(&lrel);
@@ -1135,11 +1187,17 @@ copy_table(Relation rel)
 	relmapentry = logicalrep_rel_open(lrel.remoteid, NoLock);
 	Assert(rel == relmapentry->localrel);
 
+	attnamelist = make_copy_attnamelist(relmapentry, attgenlist);
+
 	/* Start copy on the publisher. */
 	initStringInfo(&cmd);
 
-	/* Regular table with no row filter */
-	if (lrel.relkind == RELKIND_RELATION && qual == NIL)
+	/*
+	 * Regular table with no row filter and 'include_generated_columns'
+	 * specified as 'false' during creation of subscription.
+	 */
+	if (lrel.relkind == RELKIND_RELATION && qual == NIL &&
+		!MySubscription->includegencols)
 	{
 		appendStringInfo(&cmd, "COPY %s",
 						 quote_qualified_identifier(lrel.nspname, lrel.relname));
@@ -1169,17 +1227,21 @@ copy_table(Relation rel)
 	else
 	{
 		/*
-		 * For non-tables and tables with row filters, we need to do COPY
-		 * (SELECT ...), but we can't just do SELECT * because we need to not
-		 * copy generated columns. For tables with any row filters, build a
-		 * SELECT query with OR'ed row filters for COPY.
+		 * For non-tables and tables with row filters and when
+		 * 'include_generated_columns' is specified as 'true', we need to do
+		 * COPY (SELECT ...), as normal COPY of generated column is not
+		 * supported. For tables with any row filters, build a SELECT query
+		 * with OR'ed row filters for COPY.
 		 */
+		int i = 0;
+
 		appendStringInfoString(&cmd, "COPY (SELECT ");
-		for (int i = 0; i < lrel.natts; i++)
+		foreach_ptr(ListCell, l, attnamelist)
 		{
-			appendStringInfoString(&cmd, quote_identifier(lrel.attnames[i]));
-			if (i < lrel.natts - 1)
+			appendStringInfoString(&cmd, quote_identifier(strVal(l)));
+			if (i < attnamelist->length - 1)
 				appendStringInfoString(&cmd, ", ");
+			i++;
 		}
 
 		appendStringInfoString(&cmd, " FROM ");
@@ -1237,7 +1299,6 @@ copy_table(Relation rel)
 	(void) addRangeTableEntryForRelation(pstate, rel, AccessShareLock,
 										 NULL, false, false);
 
-	attnamelist = make_copy_attnamelist(relmapentry);
 	cstate = BeginCopyFrom(pstate, rel, NULL, NULL, false, copy_read_data, attnamelist, options);
 
 	/* Do the copy */
diff --git a/src/include/replication/logicalrelation.h b/src/include/replication/logicalrelation.h
index e687b40a56..797e66dfdb 100644
--- a/src/include/replication/logicalrelation.h
+++ b/src/include/replication/logicalrelation.h
@@ -41,7 +41,8 @@ typedef struct LogicalRepRelMapEntry
 
 extern void logicalrep_relmap_update(LogicalRepRelation *remoterel);
 extern void logicalrep_partmap_reset_relmap(LogicalRepRelation *remoterel);
-
+extern int logicalrep_rel_att_by_name(LogicalRepRelation *remoterel,
+									  const char *attname);
 extern LogicalRepRelMapEntry *logicalrep_rel_open(LogicalRepRelId remoteid,
 												  LOCKMODE lockmode);
 extern LogicalRepRelMapEntry *logicalrep_partition_open(LogicalRepRelMapEntry *root,
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index f308cd6ade..54b4b4195d 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -99,9 +99,6 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 ERROR:  subscription with slot_name = NONE must also set create_slot = false
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
 ERROR:  subscription with slot_name = NONE must also set enabled = false
--- fail - copy_data and include_generated_columns are mutually exclusive options
-CREATE SUBSCRIPTION sub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (include_generated_columns = true);
-ERROR:  copy_data = true and include_generated_columns = true are mutually exclusive options
 -- fail - include_generated_columns must be boolean
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
 ERROR:  include_generated_columns requires a Boolean value
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index dbf064474c..838881be50 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -60,9 +60,6 @@ CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PU
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, enabled = false);
 CREATE SUBSCRIPTION regress_testsub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (slot_name = NONE, create_slot = false);
 
--- fail - copy_data and include_generated_columns are mutually exclusive options
-CREATE SUBSCRIPTION sub2 CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (include_generated_columns = true);
-
 -- fail - include_generated_columns must be boolean
 CREATE SUBSCRIPTION regress_testsub CONNECTION 'dbname=regress_doesnotexist' PUBLICATION testpub WITH (connect = false, include_generated_columns = foo);
 
diff --git a/src/test/subscription/t/011_generated.pl b/src/test/subscription/t/011_generated.pl
index 92b3dbf0b7..788b5dc532 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -16,6 +16,8 @@ $node_publisher->start;
 
 my $node_subscriber = PostgreSQL::Test::Cluster->new('subscriber');
 $node_subscriber->init;
+$node_subscriber->append_conf('postgresql.conf',
+	"max_logical_replication_workers = 10");
 $node_subscriber->start;
 
 my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
@@ -34,18 +36,33 @@ $node_publisher->safe_psql('postgres',
 	"CREATE TABLE tab3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a + 10) STORED)"
 );
 
+# tab4: publisher-side generated col 'b' and 'c'  ==> subscriber-side non-generated col 'b', and generated-col 'c'
+$node_publisher->safe_psql('postgres',
+	"CREATE TABLE tab4 (a int , b int GENERATED ALWAYS AS (a * 2) STORED, c int GENERATED ALWAYS AS (a * 2) STORED)"
+);
+
+# tab5: publisher-side non-generated col 'b' --> subscriber-side non-generated col 'b'
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab5 (a int, b int)");
+
 $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 22) STORED, c int)"
 );
 
 $node_subscriber->safe_psql('postgres',
-	"CREATE TABLE tab2 (a int PRIMARY KEY, b int)"
-);
+	"CREATE TABLE tab2 (a int PRIMARY KEY, b int)");
 
 $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab3 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a + 20) STORED)"
 );
 
+$node_subscriber->safe_psql('postgres',
+	"CREATE TABLE tab4 (a int, b int, c int GENERATED ALWAYS AS (a * 22) STORED)"
+);
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE TABLE tab5 (a int, b int GENERATED ALWAYS AS (a * 22) STORED)");
+
+
 # data for initial sync
 
 $node_publisher->safe_psql('postgres',
@@ -54,6 +71,10 @@ $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab2 (a) VALUES (1), (2), (3)");
 $node_publisher->safe_psql('postgres',
 	"INSERT INTO tab3 (a) VALUES (1), (2), (3)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab4 (a) VALUES (1), (2), (3)");
+$node_publisher->safe_psql('postgres',
+	"INSERT INTO tab5 (a, b) VALUES (1, 1), (2, 2), (3, 3)");
 
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION pub1 FOR TABLE tab1");
@@ -61,17 +82,26 @@ $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION pub2 FOR TABLE tab2");
 $node_publisher->safe_psql('postgres',
 	"CREATE PUBLICATION pub3 FOR TABLE tab3");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION pub4 FOR TABLE tab4");
+$node_publisher->safe_psql('postgres',
+	"CREATE PUBLICATION pub5 FOR TABLE tab5");
 
 $node_subscriber->safe_psql('postgres',
-	"CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1");
+	"CREATE SUBSCRIPTION sub1 CONNECTION '$publisher_connstr' PUBLICATION pub1"
+);
 
 $node_subscriber->safe_psql('postgres',
 	"CREATE SUBSCRIPTION sub2 CONNECTION '$publisher_connstr' PUBLICATION pub2 WITH (include_generated_columns = true, copy_data = false)"
-	);
+);
 
 $node_subscriber->safe_psql('postgres',
 	"CREATE SUBSCRIPTION sub3 CONNECTION '$publisher_connstr' PUBLICATION pub3 WITH (include_generated_columns = true, copy_data = false)"
-	);
+);
+
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION sub4 CONNECTION '$publisher_connstr' PUBLICATION pub4 WITH (include_generated_columns = true)"
+);
 
 # Wait for initial sync of all subscriptions
 $node_subscriber->wait_for_subscription_sync;
@@ -82,10 +112,10 @@ is( $result, qq(1|22
 3|66), 'generated columns initial sync');
 
 $result = $node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab2");
-is( $result, qq(), 'generated columns initial sync');
+is($result, qq(), 'generated columns initial sync');
 
 $result = $node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab3");
-is( $result, qq(), 'generated columns initial sync');
+is($result, qq(), 'generated columns initial sync');
 
 # data to replicate
 
@@ -108,7 +138,9 @@ $node_publisher->wait_for_catchup('sub2');
 
 $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab2");
 is( $result, qq(4|8
-5|10), 'confirm generated columns ARE replicated when the subscriber-side column is not generated');
+5|10),
+	'confirm generated columns ARE replicated when the subscriber-side column is not generated'
+);
 
 $node_publisher->safe_psql('postgres', "INSERT INTO tab3 VALUES (4), (5)");
 
@@ -116,7 +148,38 @@ $node_publisher->wait_for_catchup('sub3');
 
 $result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab3");
 is( $result, qq(4|24
-5|25), 'confirm generated columns are NOT replicated when the subscriber-side column is also generated');
+5|25),
+	'confirm generated columns are NOT replicated when the subscriber-side column is also generated'
+);
+
+$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (4), (5)");
+
+$node_publisher->wait_for_catchup('sub4');
+
+# gen-col 'b' in publisher replicating to NOT gen-col 'b' on subscriber
+# gen-col 'c' in publisher not replicating to gen-col 'c' on subscriber
+$result =
+  $node_subscriber->safe_psql('postgres', "SELECT * FROM tab4 ORDER BY a");
+is( $result, qq(1|2|22
+2|4|44
+3|6|66
+4|8|88
+5|10|110), 'replicate generated column with initial sync');
+
+# NOT gen-col 'b' in publisher not replicating to gen-col 'b' on subscriber
+my $offset = -s $node_subscriber->logfile;
+
+# sub5 will cause table sync worker to restart repetitively
+# So SUBSCRIPTION sub5 is created separately
+$node_subscriber->safe_psql('postgres',
+	"CREATE SUBSCRIPTION sub5 CONNECTION '$publisher_connstr' PUBLICATION pub5 WITH (include_generated_columns = true)"
+);
+
+$node_subscriber->wait_for_log(
+	qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation "public.tab5" has a generated column "b" but corresponding column on source relation is not a generated column/,
+	$offset);
+
+$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub5");
 
 # try it with a subscriber-side trigger
 
-- 
2.34.1

