From 881eb624409497d957763ba92ffc6b34f421d02e Mon Sep 17 00:00:00 2001 From: Shlok Kyal 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;" include_generated_columns - 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 STORED generated columns. + This option controls whether STORED generated columns + should be included in the string representation of tuples during logical + decoding in PostgreSQL. The default is true. 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 subscription_nameinclude_generated_columns (boolean) - Specifies whether the generated columns present in the tables - associated with the subscription should be replicated. + Specifies whether the STORED generated columns present + in the tables associated with the subscription should be replicated. The default is false. 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