Hi, here are some patch v9-0001 comments.
I saw Kuroda-san has already posted comments for this patch so there
may be some duplication here.
======
GENERAL
1.
The later patches 0002 etc are checking to support only STORED
gencols. But, doesn't that restriction belong in this patch 0001 so
VIRTUAL columns are not decoded etc in the first place... (??)
~~~
2.
The "Generated Columns" docs mentioned in my previous review comment
[2] should be modified by this 0001 patch.
~~~
3.
I think the "Message Format" page mentioned in my previous review
comment [3] should be modified by this 0001 patch.
======
Commit message
4.
The patch name is still broken as previously mentioned [1, #1]
======
doc/src/sgml/protocol.sgml
5.
Should this docs be referring to STORED generated columns, instead of
just generated columns?
======
doc/src/sgml/ref/create_subscription.sgml
6.
Should this be docs referring to STORED generated columns, instead of
just generated columns?
======
src/bin/pg_dump/pg_dump.c
getSubscriptions:
NITPICK - tabs
NITPICK - patch removed a blank line it should not be touching
NITPICK = patch altered indents it should not be touching
NITPICK - a missing blank line that was previously present
7.
+ else
+ appendPQExpBufferStr(query,
+ " false AS subincludegencols,\n");
There is an unwanted comma here.
~
dumpSubscription
NITPICK - patch altered indents it should not be touching
======
src/bin/pg_dump/pg_dump.h
NITPICK - unnecessary blank line
======
src/bin/psql/describe.c
describeSubscriptions
NITPICK - bad indentation
8.
In my previous review [1, #4b] I suggested this new column should be
in a different order (e.g. adjacent to the other ones ahead of
'Conninfo'), but this is not yet addressed.
======
src/test/subscription/t/011_generated.pl
NITPICK - missing space in comment
NITPICK - misleading "because" wording in the comment
======
99.
See also my attached nitpicks diff, for cosmetic issues. Please apply
whatever you agree with.
======
[1] My v8-0001 review -
https://www.postgresql.org/message-id/CAHut%2BPujrRQ63ju8P41tBkdjkQb4X9uEdLK_Wkauxum1MVUdfA%40mail.gmail.com
[2]
https://www.postgresql.org/message-id/CAHut%2BPvsRWq9t2tEErt5ZWZCVpNFVZjfZ_owqfdjOhh4yXb_3Q%40mail.gmail.com
[3]
https://www.postgresql.org/message-id/CAHut%2BPsHsT3V1wQ5uoH9ynbmWn4ZQqOe34X%2Bg37LSi7sgE_i2g%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia.
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 1fb19f5..9f2cac9 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -4739,7 +4739,7 @@ getSubscriptions(Archive *fout)
int i_suboriginremotelsn;
int i_subenabled;
int i_subfailover;
- int i_subincludegencols;
+ int i_subincludegencols;
int i,
ntups;
@@ -4770,6 +4770,7 @@ getSubscriptions(Archive *fout)
" s.subowner,\n"
" s.subconninfo,
s.subslotname, s.subsynccommit,\n"
" s.subpublications,\n");
+
if (fout->remoteVersion >= 140000)
appendPQExpBufferStr(query, " s.subbinary,\n");
else
@@ -4804,7 +4805,7 @@ getSubscriptions(Archive *fout)
if (dopt->binary_upgrade && fout->remoteVersion >= 170000)
appendPQExpBufferStr(query, " o.remote_lsn AS
suboriginremotelsn,\n"
- " s.subenabled,\n");
+ " s.subenabled,\n");
else
appendPQExpBufferStr(query, " NULL AS suboriginremotelsn,\n"
" false AS
subenabled,\n");
@@ -4815,12 +4816,14 @@ getSubscriptions(Archive *fout)
else
appendPQExpBuffer(query,
" false AS subfailover,\n");
+
if (fout->remoteVersion >= 170000)
appendPQExpBufferStr(query,
" s.subincludegencols\n");
else
appendPQExpBufferStr(query,
" false AS
subincludegencols,\n");
+
appendPQExpBufferStr(query,
"FROM pg_subscription s\n");
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h
index a2c35fe..8c07933 100644
--- a/src/bin/pg_dump/pg_dump.h
+++ b/src/bin/pg_dump/pg_dump.h
@@ -672,7 +672,6 @@ typedef struct _SubscriptionInfo
char *suboriginremotelsn;
char *subfailover;
char *subincludegencols;
-
} SubscriptionInfo;
/*
diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c
index 491fcb9..00f3131 100644
--- a/src/bin/psql/describe.c
+++ b/src/bin/psql/describe.c
@@ -6604,7 +6604,7 @@ describeSubscriptions(const char *pattern, bool verbose)
gettext_noop("Synchronous
commit"),
gettext_noop("Conninfo"));
- /* include_generated_columns is only supported
in v18 and higher */
+ /* include_generated_columns is only supported in v18 and
higher */
if (pset.sversion >= 170000)
appendPQExpBuffer(&buf,
",
subincludegencols AS \"%s\"\n",
diff --git a/src/test/subscription/t/011_generated.pl
b/src/test/subscription/t/011_generated.pl
index e612970..6c8d6ce 100644
--- a/src/test/subscription/t/011_generated.pl
+++ b/src/test/subscription/t/011_generated.pl
@@ -106,7 +106,7 @@ $node_publisher->safe_psql('postgres', "INSERT INTO tab2
VALUES (4), (5)");
$node_publisher->wait_for_catchup('sub2');
-# the column was NOT replicated because the result value of 'b'is the
subscriber-side computed value
+# the column was NOT replicated (the result value of 'b' is the
subscriber-side computed value)
$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');
