Hi Shubham, here are my review comments for patch v19-0001.
======
src/backend/replication/pgoutput/pgoutput.c
1.
/*
* Columns included in the publication, or NULL if all columns are
* included implicitly. Note that the attnums in this bitmap are not
+ * publication and include_generated_columns option: other reasons should
+ * be checked at user side. Note that the attnums in this bitmap are not
* shifted by FirstLowInvalidHeapAttributeNumber.
*/
Bitmapset *columns;
You replied [1] "The attached Patches contain all the suggested
changes." but as I previously commented [2, #1], since there is no
change to the interpretation of the 'columns' BMS caused by this
patch, then I expected this comment would be unchanged (i.e. same as
HEAD code). But this fix was missed in v19-0001.
OTOH, if you do think there was a reason to change the comment then
the above is still not good because "are not publication and
include_generated_columns option" wording doesn't make sense.
======
src/test/subscription/t/011_generated.pl
Observation -- I added (in nitpicks diffs) some more comments for
'tab1' (to make all comments consistent with the new tests added). But
when I was doing that I observed that tab1 and tab3 test scenarios are
very similar. It seems only the subscription parameter is not
specified (so 'include_generated_cols' default wll be tested). IIRC
the default for that parameter is "false", so tab1 is not really
testing that properly -- e.g. I thought maybe to test the default
parameter it's better the subscriber-side 'b' should be not-generated?
But doing that would make 'tab1' the same as 'tab2'. Anyway, something
seems amiss -- it seems either something is not tested or is duplicate
tested. Please revisit what the tab1 test intention was and make sure
we are doing the right thing for it...
======
99.
The attached nitpicks diff patch has some tweaked comments.
======
[1]
https://www.postgresql.org/message-id/CAHv8Rj%2BR0cj%3Dz1bTMAgQKQWx1EKvkMEnV9QsHGvOqTdnLUQi1A%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPtVfrbx0jb42LCmS%3D-LcMTtWxm%2BvhaoArkjg7Z0mvuXbg%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
diff --git a/src/include/catalog/pg_subscription.h
b/src/include/catalog/pg_subscription.h
index 50c5911..e066426 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -98,8 +98,8 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId)
BKI_SHARED_RELATION BKI_ROW
* slots) in
the upstream database are enabled
* to be
synchronized to the standbys. */
- bool subincludegencols; /* True if generated columns
must be
- *
published */
+ bool subincludegencols; /* True if generated columns
should
+ * be
published */
#ifdef CATALOG_VARLEN /* variable-length fields start here */
/* Connection string to the publisher */
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index fe32987..d13d0a0 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -20,24 +20,28 @@ $node_subscriber->start;
my $publisher_connstr = $node_publisher->connstr . ' dbname=postgres';
+#
+# tab1:
+# Publisher-side tab1 has generated col 'b'.
+# Subscriber-side tab1 has generated col 'b', using a different computation,
+# and also an additional column 'c'.
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a *
2) STORED)"
);
-
$node_subscriber->safe_psql('postgres',
"CREATE TABLE tab1 (a int PRIMARY KEY, b int GENERATED ALWAYS AS (a *
22) STORED, c int)"
);
# tab2:
-# publisher-side tab2 has generated col 'b'.
-# subscriber-side tab2 has non-generated col 'b'.
+# Publisher-side tab2 has generated col 'b'.
+# 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)");
# tab3:
-# publisher-side tab3 has generated col 'b'.
-# subscriber-side tab3 has generated col 'b', using a different computation.
+# Publisher-side tab3 has generated col 'b'.
+# Subscriber-side tab3 has generated col 'b', using a different computation.
$node_publisher->safe_psql('postgres',
"CREATE TABLE tab3 (a int, b int GENERATED ALWAYS AS (a + 10) STORED)");
$node_subscriber->safe_psql('postgres',
@@ -85,12 +89,19 @@ is($result, qq(), 'generated columns initial sync');
# data to replicate
+#
+# TEST tab1:
+# Publisher-side tab1 has generated col 'b'.
+# Subscriber-side tab1 has generated col 'b', using a different computation,
+# and also an additional column 'c'.
+#
+# Confirm that col 'b' is not replicated. We can know this because the result
+# value is the subscriber-side computation (which is different from the
+# publisher-side computation for this column).
+#
$node_publisher->safe_psql('postgres', "INSERT INTO tab1 VALUES (4), (5)");
-
$node_publisher->safe_psql('postgres', "UPDATE tab1 SET a = 6 WHERE a = 5");
-
$node_publisher->wait_for_catchup('sub1');
-
$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab1");
is( $result, qq(1|22|
2|44|
@@ -100,8 +111,8 @@ is( $result, qq(1|22|
#
# TEST tab2:
-# publisher-side tab2 has generated col 'b'.
-# subscriber-side tab2 has non-generated col 'b'.
+# Publisher-side tab2 has generated col 'b'.
+# Subscriber-side tab2 has non-generated col 'b'.
#
# Confirm that col 'b' is replicated.
#
@@ -111,15 +122,15 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab2 ORDER BY a");
is( $result, qq(4|8
5|10),
- 'confirm generated columns ARE replicated when the subscriber-side
column is not generated'
+ 'confirm generated columns are replicated when the subscriber-side
column is not generated'
);
#
# TEST tab3:
-# publisher-side tab3 has generated col 'b'.
-# subscriber-side tab3 has generated col 'b', using a different computation.
+# Publisher-side tab3 has generated col 'b'.
+# Subscriber-side tab3 has generated col 'b', using a different computation.
#
-# Confirm that col 'b' is NOT replicated. We can know this because the result
+# Confirm that col 'b' is not replicated. We can know this because the result
# value is the subscriber-side computation (which is different from the
# publisher-side computation for this column).
#
@@ -129,7 +140,7 @@ $result =
$node_subscriber->safe_psql('postgres', "SELECT a, b FROM tab3 ORDER BY a");
is( $result, qq(4|24
5|25),
- 'confirm generated columns are NOT replicated when the subscriber-side
column is also generated'
+ 'confirm generated columns are not replicated when the subscriber-side
column is also generated'
);
# try it with a subscriber-side trigger
