v12-0003-Fix-behaviour-for-Virtual-Generated-columns.patch

From 881eb624409497d957763ba92ffc6b34f421d02e Mon Sep 17 00:00:00 2001
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
Date: Tue, 25 Jun 2024 16:49:13 +0530
Subject: [PATCH v11 3/3] Fix behaviour for Virtual Generated columns

Currently during tablesync Virtual generated columns are also
replicated. Also during decoding a 'null' value appears for virtual
generated column. We are not supporting replication of virtual generated
columns for now. This patch fixes the behaviour for the same.

This patch has a dependency on Virtual Generated Columns
https://www.postgresql.org/message-id/flat/787a962749e7a822a44803ffbbdf021d8573ff53.camel%40post.pl#b64569231c9e1768e07f6bdc36c4070b
---
 contrib/test_decoding/expected/binary.out     |  6 +++++-
 .../expected/decoding_into_rel.out            |  6 ++++++
 .../expected/generated_columns.out            | 13 +++++++++++-
 .../test_decoding/sql/generated_columns.sql   |  4 +++-
 contrib/test_decoding/test_decoding.c         |  8 +++++++-
 doc/src/sgml/protocol.sgml                    |  8 ++++----
 doc/src/sgml/ref/create_subscription.sgml     |  4 ++--
 src/backend/catalog/pg_publication.c          | 13 +++++++++++-
 src/backend/replication/logical/proto.c       |  8 ++++----
 src/backend/replication/logical/relation.c    |  3 +++
 src/backend/replication/logical/tablesync.c   | 19 ++++++++++++++----
 src/backend/replication/pgoutput/pgoutput.c   |  5 ++++-
 src/test/subscription/t/011_generated.pl      | 20 ++++++++++---------
 13 files changed, 88 insertions(+), 29 deletions(-)

diff --git a/contrib/test_decoding/expected/binary.out b/contrib/test_decoding/expected/binary.out
index c30abc7692..b3a3509595 100644
--- a/contrib/test_decoding/expected/binary.out
+++ b/contrib/test_decoding/expected/binary.out
@@ -1,7 +1,11 @@
 -- predictability
 SET synchronous_commit = on;
 SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
-ERROR:  replication slot "regression_slot" already exists
+ ?column? 
+----------
+ init
+(1 row)
+
 -- succeeds, textual plugin, textual consumer
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'force-binary', '0', 'skip-empty-xacts', '1');
  data 
diff --git a/contrib/test_decoding/expected/decoding_into_rel.out b/contrib/test_decoding/expected/decoding_into_rel.out
index f763e05dc7..8fd3390066 100644
--- a/contrib/test_decoding/expected/decoding_into_rel.out
+++ b/contrib/test_decoding/expected/decoding_into_rel.out
@@ -103,3 +103,9 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
  COMMIT
 (14 rows)
 
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
+ ?column? 
+----------
+ stop
+(1 row)
+
diff --git a/contrib/test_decoding/expected/generated_columns.out b/contrib/test_decoding/expected/generated_columns.out
index 3f8d6ead96..268dce1f6a 100644
--- a/contrib/test_decoding/expected/generated_columns.out
+++ b/contrib/test_decoding/expected/generated_columns.out
@@ -1,3 +1,12 @@
+-- test that we can insert the result of a 'include_generated_columns'
+-- into the tables created. That's really not a good idea in practical terms,
+-- but provides a nice test.
+SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_decoding');
+ ?column? 
+----------
+ init
+(1 row)
+
 -- check include-generated-columns option with generated column
 CREATE TABLE gencoltable (a int, b int GENERATED ALWAYS AS (a * 2) STORED);
 -- when 'include-generated-columns' is not set then columns will not be replicated
@@ -39,6 +48,8 @@ SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
 DROP TABLE gencoltable;
 SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
  ?column? 
------------
+----------
  stop
 (1 row)
+
+-- TODO: Add tests related to decoding of VIRTUAL GENERATED columns
diff --git a/contrib/test_decoding/sql/generated_columns.sql b/contrib/test_decoding/sql/generated_columns.sql
index bb50fc1fa4..9e707c5125 100644
--- a/contrib/test_decoding/sql/generated_columns.sql
+++ b/contrib/test_decoding/sql/generated_columns.sql
@@ -17,4 +17,6 @@ INSERT INTO gencoltable (a) VALUES (4), (5), (6);
 SELECT data FROM pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'include-xids', '0', 'skip-empty-xacts', '1', 'include-generated-columns', '0');
 DROP TABLE gencoltable;
 
-SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
\ No newline at end of file
+SELECT 'stop' FROM pg_drop_replication_slot('regression_slot');
+
+-- TODO: Add tests related to decoding of VIRTUAL GENERATED columns
\ No newline at end of file
diff --git a/contrib/test_decoding/test_decoding.c b/contrib/test_decoding/test_decoding.c
index eaa3dbf9db..7aca5a19ac 100644
--- a/contrib/test_decoding/test_decoding.c
+++ b/contrib/test_decoding/test_decoding.c
@@ -557,7 +557,13 @@ tuple_to_stringinfo(StringInfo s, TupleDesc tupdesc, HeapTuple tuple,
 		if (attr->attisdropped)
 			continue;
 
-		if (attr->attgenerated && !include_generated_columns)
+		/*
+		 * Don't print virtual generated column. Don't print stored
+		 * generated column if 'include_generated_columns' is false.
+		 *
+		 * TODO: can use ATTRIBUTE_GENERATED_VIRTUAL to simpilfy
+		 */
+		if (attr->attgenerated && (attr->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		/*
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 39207a6755..dd03aab60b 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -3310,10 +3310,10 @@ psql "dbname=postgres replication=database" -c "IDENTIFY_SYSTEM;"
      <term>include_generated_columns</term>
       <listitem>
        <para>
-        Boolean option to enable generated columns. This option controls
-        whether generated columns should be included in the string
-        representation of tuples during logical decoding in PostgreSQL.
-        The default is true.
+        Boolean option to enable <literal>STORED</literal> generated columns.
+        This option controls whether <literal>STORED</literal> generated columns
+        should be included in the string representation of tuples during logical
+        decoding in PostgreSQL. The default is true.
        </para>
       </listitem>
     </varlistentry>
diff --git a/doc/src/sgml/ref/create_subscription.sgml b/doc/src/sgml/ref/create_subscription.sgml
index 8fb4491b65..91e33174dc 100644
--- a/doc/src/sgml/ref/create_subscription.sgml
+++ b/doc/src/sgml/ref/create_subscription.sgml
@@ -433,8 +433,8 @@ CREATE SUBSCRIPTION <replaceable class="parameter">subscription_name</replaceabl
         <term><literal>include_generated_columns</literal> (<type>boolean</type>)</term>
         <listitem>
          <para>
-          Specifies whether the generated columns present in the tables
-          associated with the subscription should be replicated.
+          Specifies whether the <literal>STORED</literal> generated columns present
+          in the tables associated with the subscription should be replicated.
           The default is <literal>false</literal>.
          </para>
          <para>
diff --git a/src/backend/catalog/pg_publication.c b/src/backend/catalog/pg_publication.c
index f611148472..e5e5aef243 100644
--- a/src/backend/catalog/pg_publication.c
+++ b/src/backend/catalog/pg_publication.c
@@ -506,6 +506,7 @@ publication_translate_columns(Relation targetrel, List *columns,
 	Bitmapset  *set = NULL;
 	ListCell   *lc;
 	int			n = 0;
+	TupleDesc	tupdesc = RelationGetDescr(targetrel);
 
 	/* Bail out when no column list defined. */
 	if (!columns)
@@ -533,6 +534,16 @@ publication_translate_columns(Relation targetrel, List *columns,
 					errmsg("cannot use system column \"%s\" in publication column list",
 						   colname));
 
+		/*
+		 * TODO: simplify the expression
+		 */
+		if (TupleDescAttr(tupdesc, attnum - 1)->attgenerated &&
+			TupleDescAttr(tupdesc, attnum - 1)->attgenerated != ATTRIBUTE_GENERATED_STORED)
+			ereport(ERROR,
+					errcode(ERRCODE_INVALID_COLUMN_REFERENCE),
+					errmsg("cannot use virtual generated column \"%s\" in publication column list",
+						   colname));
+
 		if (bms_is_member(attnum, set))
 			ereport(ERROR,
 					errcode(ERRCODE_DUPLICATE_OBJECT),
@@ -1225,7 +1236,7 @@ pg_get_publication_tables(PG_FUNCTION_ARGS)
 			{
 				Form_pg_attribute att = TupleDescAttr(desc, i);
 
-				if (att->attisdropped)
+				if (att->attisdropped || (att->attgenerated && att->attgenerated != ATTRIBUTE_GENERATED_STORED))
 					continue;
 
 				attnums[nattnums++] = att->attnum;
diff --git a/src/backend/replication/logical/proto.c b/src/backend/replication/logical/proto.c
index 7405eb3deb..e82e53e384 100644
--- a/src/backend/replication/logical/proto.c
+++ b/src/backend/replication/logical/proto.c
@@ -793,7 +793,7 @@ logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot,
 		if (att->attisdropped)
 			continue;
 
-		if (att->attgenerated && !include_generated_columns)
+		if (att->attgenerated && (att->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		if (!column_in_column_list(att->attnum, columns))
@@ -817,7 +817,7 @@ logicalrep_write_tuple(StringInfo out, Relation rel, TupleTableSlot *slot,
 		if (att->attisdropped)
 			continue;
 
-		if (att->attgenerated && !include_generated_columns)
+		if (att->attgenerated && (att->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		if (!column_in_column_list(att->attnum, columns))
@@ -957,7 +957,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel, Bitmapset *columns,
 		if (att->attisdropped)
 			continue;
 
-		if (att->attgenerated && !include_generated_columns)
+		if (att->attgenerated && (att->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		if (!column_in_column_list(att->attnum, columns))
@@ -981,7 +981,7 @@ logicalrep_write_attrs(StringInfo out, Relation rel, Bitmapset *columns,
 		if (att->attisdropped)
 			continue;
 
-		if (att->attgenerated && !include_generated_columns)
+		if (att->attgenerated && (att->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		if (!column_in_column_list(att->attnum, columns))
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 27c34059af..e1b1693700 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -427,6 +427,9 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 				continue;
 			}
 
+			if (attr->attgenerated && attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
+				continue;
+
 			/*
 			 * In case 'include_generated_columns' is 'false', we should skip the
 			 * check of missing attrs for generated columns.
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index b3fde6afb3..d44f10901e 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -712,7 +712,7 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool *remotegenlist)
 		int			attnum;
 		Form_pg_attribute attr = TupleDescAttr(desc, i);
 
-		if (!attr->attgenerated)
+		if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
 			continue;
 
 		attnum = logicalrep_rel_att_by_name(&rel->remoterel,
@@ -1001,10 +1001,21 @@ fetch_remote_table_info(char *nspname, char *relname, bool **remotegenlist,
 					 " WHERE a.attnum > 0::pg_catalog.int2"
 					 "   AND NOT a.attisdropped", lrel->remoteid);
 
-	if ((walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000 &&
-		walrcv_server_version(LogRepWorkerWalRcvConn) <= 160000) ||
-		!MySubscription->includegencols)
+	if (walrcv_server_version(LogRepWorkerWalRcvConn) >= 120000)
+	{
+		bool gencols_allowed = walrcv_server_version(LogRepWorkerWalRcvConn) >= 170000
+							   && MySubscription->includegencols;
+		if (gencols_allowed)
+		{
+			/* Replication of generated cols is supported, but not VIRTUAL cols. */
+			appendStringInfo(&cmd, " AND a.attgenerated != 'v'");
+		}
+		else
+		{
+			/* Replication of generated cols is not supported. */
 			appendStringInfo(&cmd, " AND a.attgenerated = ''");
+		}
+	}
 
 	appendStringInfo(&cmd,
 					 "   AND a.attrelid = %u"
diff --git a/src/backend/replication/pgoutput/pgoutput.c b/src/backend/replication/pgoutput/pgoutput.c
index 00c6566959..69aaf849e4 100644
--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
@@ -784,7 +784,7 @@ send_relation_and_attrs(Relation relation, TransactionId xid,
 		if (att->attisdropped)
 			continue;
 
-		if (att->attgenerated && !include_generated_columns)
+		if (att->attgenerated && (att->attgenerated != ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
 			continue;
 
 		if (att->atttypid < FirstGenbkiObjectId)
@@ -1106,6 +1106,9 @@ pgoutput_column_list_init(PGOutputData *data, List *publications,
 						if (att->attisdropped)
 							continue;
 
+						if (att->attgenerated && att->attgenerated != ATTRIBUTE_GENERATED_STORED)
+							continue;
+
 						nliveatts++;
 					}
 
diff --git a/src/test/subscription/t/011_generated.pl b/src/test/subscription/t/011_generated.pl
index 3ab004429f..bb086791a3 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -30,20 +30,20 @@ $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 22) STORED, c int)"
 );
 
-# publisher-side tab2 has generated col 'b' but subscriber-side tab2 has NON-generated col 'b'.
+# publisher-side tab2 has stored generated col 'b' but subscriber-side tab2 has NON-generated col 'b'.
 $node_publisher->safe_psql('postgres',
 	"CREATE TABLE tab2 (a int, b int GENERATED ALWAYS AS (a * 2) STORED)");
 
 $node_subscriber->safe_psql('postgres', "CREATE TABLE tab2 (a int, b int)");
 
-# publisher-side tab3 has generated col 'b' but subscriber-side tab2 has DIFFERENT COMPUTATION generated col 'b'.
+# publisher-side tab3 has stored generated col 'b' but subscriber-side tab2 has DIFFERENT COMPUTATION generated col 'b'.
 $node_publisher->safe_psql('postgres',
 	"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 10) STORED)");
 
 $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 20) STORED)");
 
-# tab4: publisher-side generated col 'b' and 'c' --> subscriber-side non-generated col 'b', and generated-col 'c'
+# tab4: publisher-side stored generated col 'b' and 'c' --> subscriber-side non-generated col 'b', and stored 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)"
 );
@@ -52,19 +52,21 @@ $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab4 (a int, b int, c int GENERATED ALWAYS AS (a * 22) STORED)"
 );
 
-# tab5: publisher-side non-generated col 'b' --> subscriber-side generated col 'b'
+# tab5: publisher-side non-generated col 'b' --> subscriber-side stored generated col 'b'
 $node_publisher->safe_psql('postgres', "CREATE TABLE tab5 (a int, b int)");
 
 $node_subscriber->safe_psql('postgres',
 	"CREATE TABLE tab5 (a int, b int GENERATED ALWAYS AS (a * 22) STORED)");
 
-# tab6: publisher-side generated col 'b' and 'c' --> subscriber-side non-generated col 'b', and generated-col 'c'
+# tab6: publisher-side stored generated col 'b' and 'c' --> subscriber-side non-generated col 'b', and stored generated-col 'c'
 # columns on subscriber in different order
 $node_publisher->safe_psql('postgres',
 	"CREATE TABLE tab6 (a int, b int GENERATED ALWAYS AS (a * 2) STORED, c int GENERATED ALWAYS AS (a * 2) STORED)");
 
 $node_subscriber->safe_psql('postgres', "CREATE TABLE tab6 (c int GENERATED ALWAYS AS (a * 22) STORED, b int, a int)");
 
+# TODO: Add tests related to replication of VIRTUAL GNERATED COLUMNS
+
 # data for initial sync
 
 $node_publisher->safe_psql('postgres',
@@ -169,8 +171,8 @@ $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
+# stored gen-col 'b' in publisher replicating to NOT gen-col 'b' on subscriber
+# stored gen-col 'c' in publisher not replicating to stored gen-col 'c' on subscriber
 $result =
   $node_subscriber->safe_psql('postgres',
 	"SELECT a, b, c FROM tab4 ORDER BY a");
@@ -184,7 +186,7 @@ $node_publisher->safe_psql('postgres', "INSERT INTO tab6 VALUES (4), (5)");
 
 $node_publisher->wait_for_catchup('sub6');
 
-# gen-col 'b' and 'c' in publisher replicating to NOT gen-col 'b' and gen-col 'c' on subscriber
+# stored gen-col 'b' and 'c' in publisher replicating to NOT gen-col 'b' and gen-col 'c' on subscriber
 # order of column is different on subscriber
 $result =
   $node_subscriber->safe_psql('postgres', "SELECT a, b, c FROM tab6 ORDER BY a");
@@ -194,7 +196,7 @@ is( $result, qq(1|2|22
 4|8|88
 5|10|110), 'replicate generated column with initial sync different column order');
 
-# NOT gen-col 'b' in publisher not replicating to gen-col 'b' on subscriber
+# NOT gen-col 'b' in publisher not replicating to stored gen-col 'b' on subscriber
 my $offset = -s $node_subscriber->logfile;
 
 # sub5 will cause table sync worker to restart repetitively
-- 
2.41.0.windows.3