Hi, here are some review comments for v19-0002
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
nitpick - tweak function comment
nitpick - tweak other comments
~~~
fetch_remote_table_info:
nitpick - add space after "if"
nitpick - removed a comment because logic is self-evident from the variable name
======
src/test/subscription/t/004_sync.pl
1.
This new test is not related to generated columns. IIRC, this is just
some test that we discovered missing during review of this thread. As
such, I think this change can be posted/patched separately from this
thread.
======
src/test/subscription/t/011_generated.pl
nitpick - change some comment wording to be more consistent with patch 0001.
======
99.
Please see the nitpicks diff attachment which implements any nitpicks
mentioned above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 935be7f..2e90d42 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -693,8 +693,8 @@ process_syncing_tables(XLogRecPtr current_lsn)
}
/*
- * Create list of columns for COPY based on logical relation mapping. Do not
- * include generated columns of the subscription table in the column list.
+ * Create list of columns for COPY based on logical relation mapping.
+ * Exclude columns that are subscription table generated columns.
*/
static List *
make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool *remotegenlist)
@@ -707,7 +707,7 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*remotegenlist)
localgenlist = palloc0(rel->remoterel.natts * sizeof(bool));
/*
- * This loop checks for generated columns on subscription table.
+ * This loop checks for generated columns of the subscription table.
*/
for (int i = 0; i < desc->natts; i++)
{
@@ -1010,15 +1010,12 @@ fetch_remote_table_info(char *nspname, char *relname,
bool **remotegenlist_res,
" WHERE a.attnum > 0::pg_catalog.int2"
" AND NOT a.attisdropped",
lrel->remoteid);
- if(server_version >= 120000)
+ if (server_version >= 120000)
{
bool gencols_allowed = server_version >= 180000 &&
MySubscription->includegencols;
- if(!gencols_allowed)
- {
- /* Replication of generated cols is not supported. */
+ if (!gencols_allowed)
appendStringInfo(&cmd, " AND a.attgenerated = ''");
- }
}
appendStringInfo(&cmd,
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 1814628..4537c6c 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -46,9 +46,9 @@ $node_subscriber->safe_psql('postgres',
"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 20) STORED)");
# tab4:
-# publisher-side tab4 has generated cols 'b' and 'c' but
-# subscriber-side tab4 has non-generated col 'b', and generated-col 'c'
-# where columns on publisher/subscriber are in a different order
+# Publisher-side tab4 has generated cols 'b' and 'c'.
+# Subscriber-side tab4 has non-generated col 'b', and generated-col 'c'.
+# Columns on publisher/subscriber are in a different order.
$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)"
);
@@ -57,14 +57,14 @@ $node_subscriber->safe_psql('postgres',
);
# tab5:
-# publisher-side tab5 has non-generated col 'b' but
-# subscriber-side tab5 has generated col 'b'
+# Publisher-side tab5 has non-generated col 'b'.
+# Subscriber-side tab5 has 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:
-# tables for testing ALTER SUBSCRIPTION ... REFRESH PUBLICATION
+# Tables for testing ALTER SUBSCRIPTION ... REFRESH PUBLICATION
$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)"
);
@@ -73,8 +73,8 @@ $node_subscriber->safe_psql('postgres',
);
# tab7:
-# publisher-side tab7 has generated col 'b' but
-# subscriber-side tab7 do not have col 'b'
+# Publisher-side tab7 has generated col 'b'.
+# Subscriber-side tab7 does not have any col 'b'.
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab7 (a int, b int GENERATED ALWAYS AS (a * 2) STORED)"
);
@@ -209,9 +209,12 @@ is( $result, qq(1|21
);
#
-# TEST tab4: the publisher-side cols 'b' and 'c' are generated and
subscriber-side
-# col 'b' is not generated and col 'c' is generated. So confirmed that the
different
-# order of columns on subscriber-side replicate data to correct columns.
+# TEST tab4:
+# Publisher-side tab4 has generated cols 'b' and 'c'.
+# Subscriber-side tab4 has non-generated col 'b', and generated-col 'c'.
+# Columns on publisher/subscriber are in a different order.
+#
+# Confirm despite the different order columns, they still replicate correctly.
#
$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (4), (5)");
$node_publisher->wait_for_catchup('sub4');
@@ -225,10 +228,15 @@ is( $result, qq(1|2|22
5|10|110), 'replicate generated columns with different order on subscriber');
#
-# TEST tab5: publisher-side col 'b' is not-generated and subscriber-side col
'b'
-# is generated, so confirmed that col 'b' IS NOT replicated and it will throw
an error.
-# The subscription sub5 is created here, instead of earlier with the other
subscriptions,
-# because sub5 will cause the tablesync worker to restart repetitively.
+# TEST tab5:
+# Publisher-side tab5 has non-generated col 'b'.
+# Subscriber-side tab5 has generated col 'b'.
+#
+# Confirm that col 'b' is not replicated and it will throw an error.
+#
+# Note that subscription sub5 is created here, instead of earlier with the
+# other subscriptions, because sub5 will cause the tablesync worker to restart
+# repetitively.
#
my $offset = -s $node_subscriber->logfile;
$node_subscriber->safe_psql('postgres',
@@ -254,9 +262,13 @@ is( $result, qq(1|2|22
3|6|66), 'add new table to existing publication');
#
-# TEST tab6: Drop the generated column's expression on subscriber side.
+# TEST tab6:
+# Drop the generated column's expression on subscriber side.
# This changes the generated column into a non-generated column.
#
+# Confirm that replication happens after the drop expression, because now we
+# are replicating from a generated column to a non-generated column.
+#
$node_subscriber->safe_psql('postgres',
"ALTER TABLE tab6 ALTER COLUMN c DROP EXPRESSION");
$node_publisher->safe_psql('postgres',
@@ -270,10 +282,16 @@ is( $result, qq(1|2|22
5|10|10), 'after drop generated column expression');
#
-# TEST tab7: publisher-side col 'b' is generated and subscriber-side do not
have col 'b' and
-# 'include_generated_column' is 'true' so confirmed that col 'b' IS NOT
replicated and
-# it will throw an error. The subscription sub7 is created here, instead of
earlier with the
-# other subscriptions, because sub7 will cause the tablesync worker to restart
repetitively.
+# TEST tab7, false
+# Publisher-side tab7 has generated col 'b'.
+# Subscriber-side tab7 does not have any col 'b'.
+# 'include_generated_columns' is true.
+#
+# Confirm that attempted replication of col 'b' will throw an error.
+#
+# Note the subscription sub7 is created here, instead of earlier with the
+# other subscriptions, because sub7 will cause the tablesync worker to restart
+# repetitively.
#
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION sub7 CONNECTION '$publisher_connstr' PUBLICATION
pub7 with (include_generated_columns = true)"
@@ -284,8 +302,12 @@ $node_subscriber->wait_for_log(
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION sub7");
#
-# TEST tab7: publisher-side col 'b' is generated and subscriber-side do not
have col 'b' and
-# 'include_generated_column' is 'false' so confirmed that col 'b' IS NOT
replicated.
+# TEST tab7:
+# Publisher-side tab7 has generated col 'b'.
+# Subscriber-side tab7 does not have any col 'b'.
+# 'include_generated_columns' is default (false).
+#
+# Confirm that col 'b' is not replicated, and no error occurs.
#
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION sub7 CONNECTION '$publisher_connstr' PUBLICATION
pub7"
