Re: Pgoutput not capturing the generated columns - Mailing list pgsql-hackers

On Tue, Sep 17, 2024 at 1:14 PM Peter Smith <smithpb2250@gmail.com> wrote:
>
> 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.
>

I have addressed all the comments in the v32-0001 Patch. Please refer
to the updated v32-0001 Patch here in [1]. See [1] for the changes
added.

[1] https://www.postgresql.org/message-id/CAHv8RjKkoaS1oMsFvPRFB9nPSVC5p_D4Kgq5XB9Y2B2xU7smbA%40mail.gmail.com

Thanks and Regards,
Shubham Khanna.