Here are my review comments for v14-0002.
======
src/backend/replication/logical/tablesync.c
make_copy_attnamelist:
nitpick - remove excessive parentheses in palloc0 call.
nitpick - Code is fine AFAICT except it's not immediately obvious
localgenlist is indexed by the *remote* attribute number. So I renamed
'attrnum' variable in my nitpicks diff. OTOH, if you think no change
is necessary, that is OK to (in that case maybe add a comment).
~~~
1. fetch_remote_table_info
+ if ((server_version >= 120000 && server_version <= 160000) ||
+ !MySubscription->includegencols)
+ appendStringInfo(&cmd, " AND a.attgenerated = ''");
Should this say < 180000 instead of <= 160000?
~~~
copy_table:
nitpick - uppercase in comment
nitpick - missing space after "if"
~~~
2. copy_table
+ attnamelist = make_copy_attnamelist(relmapentry, remotegenlist);
+
/* Start copy on the publisher. */
initStringInfo(&cmd);
- /* Regular table with no row filter */
- if (lrel.relkind == RELKIND_RELATION && qual == NIL)
+ /* check if remote column list has generated columns */
+ if(MySubscription->includegencols)
+ {
+ for (int i = 0; i < relmapentry->remoterel.natts; i++)
+ {
+ if(remotegenlist[i])
+ {
+ remote_has_gencol = true;
+ break;
+ }
+ }
+ }
+
There is some subtle logic going on here:
For example, the comment here says "Check if the remote column list
has generated columns", and it then proceeds to iterate the remote
attributes checking the remotegenlist[i]. But the remotegenlist[] was
returned from a prior call to make_copy_attnamelist() and according to
the make_copy_attnamelist logic, it is NOT returning all remote
generated-cols in that list. Specifically, it is stripping some of
them -- "Do not include generated columns of the subscription table in
the [remotegenlist] column list.".
So, actually this loop seems to be only finding cases (setting
remote_has_gen = true) where the remote column is generated but the
match local column is *not* generated. Maybe this was the intended
logic all along but then certainly the comment should be improved to
describe it better.
~~~
3.
+ /*
+ * Regular table with no row filter and 'include_generated_columns'
+ * specified as 'false' during creation of subscription.
+ */
+ if (lrel.relkind == RELKIND_RELATION && qual == NIL && !remote_has_gencol)
nitpick - This comment also needs improving. For example, just because
remote_has_gencol is false, it does not follow that
'include_generated_columns' was specified as 'false' -- maybe the
parameter was 'true' but the table just had no generated columns
anyway... I've modified the comment already in my nitpicks diff, but
probably you can improve on that.
~
nitpick - "else" comment is modified slightly too. Please see the nitpicks diff.
~
4.
In hindsight, I felt your variable 'remote_has_gencol' was not
well-named because it is not for saying the remote table has a
generated column -- it is saying the remote table has a generated
column **that we have to copy**. So, rather it should be named
something like 'gencol_copy_needed' (but I didn't change this name in
the nitpick diffs...)
======
src/test/subscription/t/004_sync.pl
nitpick - changes to comment style to make the test case separations
much more obvious
nitpick - minor comment wording tweaks
5.
Here, you are confirming we get an ERROR when replicating from a
non-generated column to a generated column. But I think your patch
also added exactly that same test scenario in the 011_generated (as
the sub5 test). So, maybe this one here should be removed?
======
src/test/subscription/t/011_generated.pl
nitpick - comment wrapping at 80 chars
nitpick - add/remove blank lines for readability
nitpick - typo /subsriber/subscriber/
nitpick - prior to the ALTER test, tab6 is unsubscribed. So add
another test to verify its initial data
nitpick - sometimes the msg 'add a new table to existing publication'
is misplaced
nitpick - the tests for tab6 and tab5 were in opposite to the expected
order, so swapped them.
======
99.
Please see also the attached diff which implements all the nitpicks
described in this post.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/src/backend/replication/logical/tablesync.c
b/src/backend/replication/logical/tablesync.c
index 38f3621..c264353 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -704,30 +704,30 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*remotegenlist)
TupleDesc desc;
desc = RelationGetDescr(rel->localrel);
- localgenlist = palloc0((rel->remoterel.natts * sizeof(bool)));
+ localgenlist = palloc0(rel->remoterel.natts * sizeof(bool));
/*
* This loop checks for generated columns on subscription table.
*/
for (int i = 0; i < desc->natts; i++)
{
- int attnum;
+ int remote_attnum;
Form_pg_attribute attr = TupleDescAttr(desc, i);
if (!attr->attgenerated)
continue;
- attnum = logicalrep_rel_att_by_name(&rel->remoterel,
+ remote_attnum = logicalrep_rel_att_by_name(&rel->remoterel,
NameStr(attr->attname));
- if (attnum >= 0)
+ if (remote_attnum >= 0)
{
/*
* Check if the subscription table generated column has
same
* name as a non-generated column in the corresponding
* publication table.
*/
- if (!remotegenlist[attnum])
+ if (!remotegenlist[remote_attnum])
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("logical replication
target relation \"%s.%s\" has a generated column \"%s\" "
@@ -739,7 +739,7 @@ make_copy_attnamelist(LogicalRepRelMapEntry *rel, bool
*remotegenlist)
* the subscription table. Later, we use this
information to
* skip adding this column to the column list for COPY.
*/
- localgenlist[attnum] = true;
+ localgenlist[remote_attnum] = true;
}
}
@@ -1205,12 +1205,12 @@ copy_table(Relation rel)
/* Start copy on the publisher. */
initStringInfo(&cmd);
- /* check if remote column list has generated columns */
+ /* Check if remote column list has generated columns */
if(MySubscription->includegencols)
{
for (int i = 0; i < relmapentry->remoterel.natts; i++)
{
- if(remotegenlist[i])
+ if (remotegenlist[i])
{
remote_has_gencol = true;
break;
@@ -1219,8 +1219,8 @@ copy_table(Relation rel)
}
/*
- * Regular table with no row filter and 'include_generated_columns'
- * specified as 'false' during creation of subscription.
+ * Regular table with no row filter and copy of generated columns is
+ * not necessary.
*/
if (lrel.relkind == RELKIND_RELATION && qual == NIL &&
!remote_has_gencol)
{
@@ -1258,8 +1258,9 @@ copy_table(Relation rel)
* SELECT query with OR'ed row filters for COPY.
*
* We also need to use this same COPY (SELECT ...) syntax when
- * 'include_generated_columns' is specified as true, because
copy
- * of generated columns is not supported by the normal COPY.
+ * 'include_generated_columns' is specified as true and the
remote
+ * table has generated columns, because copy of generated
columns is
+ * not supported by the normal COPY.
*/
int i = 0;
diff --git a/src/test/subscription/t/004_sync.pl
b/src/test/subscription/t/004_sync.pl
index 68052e1..62462c0 100644
--- a/src/test/subscription/t/004_sync.pl
+++ b/src/test/subscription/t/004_sync.pl
@@ -176,18 +176,24 @@ ok( $node_publisher->poll_query_until(
$node_publisher->safe_psql('postgres', "DROP TABLE tab_rep");
$node_subscriber->safe_psql('postgres', "DROP TABLE tab_rep");
-# When a subscription table have a column missing as specified on publication
table
+#
+# TEST CASE:
+#
+# When a subscription table has a column missing that was specified on
+# the publication table.
+#
+
# setup structure with existing data on publisher
$node_publisher->safe_psql('postgres', "CREATE TABLE tab_rep (a int, b int)");
$node_publisher->safe_psql('postgres',
"INSERT INTO tab_rep VALUES (1, 1), (2, 2), (3, 3)");
-# add table on subscriber
+# add table on subscriber; note column 'b' is missing
$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_rep (a int)");
my $offset = -s $node_subscriber->logfile;
-# recreate the subscription again
+# create the subscription
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr'
PUBLICATION tap_pub"
);
@@ -201,15 +207,23 @@ $node_subscriber->wait_for_log(
$node_subscriber->safe_psql('postgres', "DROP SUBSCRIPTION tap_sub");
$node_subscriber->safe_psql('postgres', "DROP TABLE tab_rep");
-# When a subscription table have a generated column corresponding to
non-generated column on publication table
-# create table on subscriber side with generated column
+#
+# TEST CASE:
+#
+# When a subscription table has a generated column corresponding to a
+# non-generated column on publication table
+#
+
+# create table on subscriber side with generated column 'b'
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab_rep (a int, b int GENERATED ALWAYS AS (a * 2)
STORED)");
+
+# create the subscription
$node_subscriber->safe_psql('postgres',
"CREATE SUBSCRIPTION tap_sub CONNECTION '$publisher_connstr'
PUBLICATION tap_pub"
);
-# check for missing column error
+# check for generated column mismatch error
$node_subscriber->wait_for_log(
qr/ERROR: ( [A-Z0-9]+:)? logical replication target relation
"public.tab_rep" has a generated column "b" but corresponding column on source
relation is not a generated column/,
$offset);
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index 2628ad3..9569f57 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -35,33 +35,37 @@ $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 tab3 has
DIFFERENT COMPUTATION generated col 'b'.
+# tab3:
+# publisher-side tab3 has generated col 'b' but
+# subscriber-side tab3 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)");
-# 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 the subscriber are in a different order
+# 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
$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)"
);
-
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab4 (c int GENERATED ALWAYS AS (a * 22) STORED, a int, b
int)"
);
-# publisher-side tab5 has non-generated col 'b' but subscriber-side tab5 has
generated col 'b'
+# tab5:
+# publisher-side tab5 has non-generated col 'b' but
+# 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)");
-# test for alter subscription ... refresh publication
+# tab6:
+# tables for testing ALTER SUBSCRIPTIO ... 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)"
);
-
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab6 (a int, b int, c int GENERATED ALWAYS AS (a * 22)
STORED)"
);
@@ -129,6 +133,10 @@ is( $result, qq(1|2|22
2|4|44
3|6|66), 'generated column initial sync');
+$result = $node_subscriber->safe_psql('postgres',
+ "SELECT a, b, c FROM tab6 ORDER BY a");
+is( $result, qq(), 'unsubscribed table initial data');
+
# data to replicate
$node_publisher->safe_psql('postgres', "INSERT INTO tab1 VALUES (4), (5)");
@@ -178,9 +186,11 @@ is( $result, qq(1|21
'confirm generated columns are NOT replicated when the subscriber-side
column is also generated'
);
+#
# 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 subsriber-side replicate data to correct columns.
+# order of columns on subscriber-side replicate data to correct columns.
+#
$node_publisher->safe_psql('postgres', "INSERT INTO tab4 VALUES (4), (5)");
$node_publisher->wait_for_catchup('sub4');
$result =
@@ -192,12 +202,26 @@ is( $result, qq(1|2|22
4|8|88
5|10|110), 'replicate generated columns with different order on subscriber');
-# TEST for ALTER SUBSCRIPTION ... REFRESH PUBLICATION
-# Add a new table to publication
+#
+# 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.
+#
+my $offset = -s $node_subscriber->logfile;
+$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");
+
+#
+# TEST tab6: After ALTER SUBSCRIPTION ... REFRESH PUBLICATION
+#
$node_publisher->safe_psql('postgres',
"ALTER PUBLICATION pub4 ADD TABLE tab6");
-
-# Refresh publication after table is added to publication
$node_subscriber->safe_psql('postgres',
"ALTER SUBSCRIPTION sub4 REFRESH PUBLICATION");
$node_publisher->wait_for_catchup('sub4');
@@ -207,7 +231,10 @@ is( $result, qq(1|2|22
2|4|44
3|6|66), 'add new table to existing publication');
-# TEST: drop generated column on subscriber side
+#
+# TEST tab6: Drop the generated column's expression on subscriber side.
+# This changes the generated column into a non-generated column.
+#
$node_subscriber->safe_psql('postgres',
"ALTER TABLE tab6 ALTER COLUMN c DROP EXPRESSION");
$node_publisher->safe_psql('postgres',
@@ -218,21 +245,7 @@ is( $result, qq(1|2|22
2|4|44
3|6|66
4|8|8
-5|10|10), 'add new table to existing publication');
-
-# 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.
-# SUBSCRIPTION sub5 is created separately as sub5 will cause table sync worker
to restart
-# repetitively
-my $offset = -s $node_subscriber->logfile;
-$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");
-
+5|10|10), 'after drop generated column expression');
# try it with a subscriber-side trigger
