Here are some review comments for v31-0001 (for the docs only)
There may be some overlap here with some comments already made for
v30-0001 which are not yet addressed in v31-0001.
======
Commit message
1.
When introducing the 'publish_generated_columns' parameter, you must
also say this is a PUBLICATION parameter.
~~~
2.
With this enhancement, users can now include the 'include_generated_columns'
option when querying logical replication slots using either the pgoutput
plugin or the test_decoding plugin. This option, when set to 'true' or '1',
instructs the replication system to include generated column information
and data in the replication stream.
~
The above is stale information because it still refers to the old name
'include_generated_columns', and to test_decoding which was already
removed in this patch.
======
doc/src/sgml/ddl.sgml
3.
+ Generated columns may be skipped during logical replication
according to the
+ <command>CREATE PUBLICATION</command> option
+ <link
linkend="sql-createpublication-params-with-include-generated-columns">
+ <literal>publish_generated_columns</literal></link>.
3a.
nit - The linkend is based on the old name instead of the new name.
3b.
nit - Better to call this a parameter instead of an option because
that is what the CREATE PUBLICATION docs call it.
======
doc/src/sgml/protocol.sgml
4.
+ <varlistentry>
+ <term>publish_generated_columns</term>
+ <listitem>
+ <para>
+ 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.
+ </para>
+ </listitem>
+ </varlistentry>
+
Is this even needed anymore? Now that the implementation is using a
PUBLICATION parameter, isn't everything determined just by that
parameter? I don't see the reason why a protocol change is needed
anymore. And, if there is no protocol change needed, then this
documentation change is also not needed.
~~~~
5.
<para>
- Next, the following message part appears for each column included in
- the publication (except generated columns):
+ Next, the following message parts appear for each column included in
+ the publication (generated columns are excluded unless the parameter
+ <link linkend="protocol-logical-replication-params">
+ <literal>publish_generated_columns</literal></link> specifies otherwise):
</para>
Like the previous comment above, I think everything is now determined
by the PUBLICATION parameter. So maybe this should just be referring
to that instead.
======
doc/src/sgml/ref/create_publication.sgml
6.
+ <varlistentry
id="sql-createpublication-params-with-include-generated-columns">
+ <term><literal>publish_generated_columns</literal>
(<type>boolean</type>)</term>
+ <listitem>
nit - the ID is based on the old parameter name.
~
7.
+ <para>
+ This option is only available for replicating generated
column data from the publisher
+ to a regular, non-generated column in the subscriber.
+ </para>
IMO remove this paragraph. I really don't think you should be
mentioning the subscriber here at all. AFAIK this parameter is only
for determining if the generated column will be published or not. What
happens at the other end (e.g. logic whether it gets ignored or not by
the subscriber) is more like a matrix of behaviours that could be
documented in the "Logical Replication" section. But not here.
(I removed this in my nitpicks attachment)
~~~
8.
+ <para>
+ This parameter can only be set <literal>true</literal> if
<literal>copy_data</literal> is
+ set to <literal>false</literal>.
+ </para>
IMO remove this paragraph too. The user can create a PUBLICATION
before a SUBSCRIPTION even exists so to say it "can only be set..." is
not correct. Sure, your patch 0001 does not support the COPY of
generated columns but if you want to document that then it should be
documented in the CREATE SUBSCRIBER docs. But not here.
(I removed this in my nitpicks attachment)
TBH, it would be better if patches 0001 and 0002 were merged then you
can avoid all this. IIUC they were only separate in the first place
because 2 different people wrote them. It is not making reviews easier
with them split.
======
Please see the attachment which implements some of the nits above.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml
index 2e7804e..cca54bc 100644
--- a/doc/src/sgml/ddl.sgml
+++ b/doc/src/sgml/ddl.sgml
@@ -515,8 +515,8 @@ CREATE TABLE people (
<listitem>
<para>
Generated columns may be skipped during logical replication according to
the
- <command>CREATE PUBLICATION</command> option
- <link
linkend="sql-createpublication-params-with-include-generated-columns">
+ <command>CREATE PUBLICATION</command> parameter
+ <link
linkend="sql-createpublication-params-with-publish-generated-columns">
<literal>publish_generated_columns</literal></link>.
</para>
</listitem>
diff --git a/doc/src/sgml/ref/create_publication.sgml
b/doc/src/sgml/ref/create_publication.sgml
index e133dc3..cd20bd4 100644
--- a/doc/src/sgml/ref/create_publication.sgml
+++ b/doc/src/sgml/ref/create_publication.sgml
@@ -223,7 +223,7 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>
</listitem>
</varlistentry>
- <varlistentry
id="sql-createpublication-params-with-include-generated-columns">
+ <varlistentry
id="sql-createpublication-params-with-publish-generated-columns">
<term><literal>publish_generated_columns</literal>
(<type>boolean</type>)</term>
<listitem>
<para>
@@ -231,14 +231,6 @@ CREATE PUBLICATION <replaceable
class="parameter">name</replaceable>
associated with the publication should be replicated.
The default is <literal>false</literal>.
</para>
- <para>
- This option is only available for replicating generated column data
from the publisher
- to a regular, non-generated column in the subscriber.
- </para>
- <para>
- This parameter can only be set <literal>true</literal> if
<literal>copy_data</literal> is
- set to <literal>false</literal>.
- </para>
</listitem>
</varlistentry>
