On Fri, May 24, 2024 at 8:26 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Hi,
>
> Here are some review comments for the patch v3-0001.
>
> I don't think v3 addressed any of my previous review comments for
> patches v1 and v2. [1][2]
>
> So the comments below are limited only to the new code (i.e. the v3
> versus v2 differences). Meanwhile, all my previous review comments may
> still apply.
Patch v4-0001 addresses the previous review comments for patches v1
and v2. [1][2]
> ======
> GENERAL
>
> The patch applied gives whitespace warnings:
>
> [postgres@CentOS7-x64 oss_postgres_misc]$ git apply
> ../patches_misc/v3-0001-Support-generated-column-capturing-generated-colu.patch
> ../patches_misc/v3-0001-Support-generated-column-capturing-generated-colu.patch:150:
> trailing whitespace.
>
> ../patches_misc/v3-0001-Support-generated-column-capturing-generated-colu.patch:202:
> trailing whitespace.
>
> ../patches_misc/v3-0001-Support-generated-column-capturing-generated-colu.patch:730:
> trailing whitespace.
> warning: 3 lines add whitespace errors.
Fixed.
> ======
> contrib/test_decoding/test_decoding.c
>
> 1. pg_decode_change
>
> MemoryContext old;
> + bool include_generated_columns;
> +
>
> I'm not really convinced this variable saves any code.
Fixed.
> ======
> doc/src/sgml/protocol.sgml
>
> 2.
> + <varlistentry>
> + <term><replaceable
> class="parameter">include-generated-columns</replaceable></term>
> + <listitem>
> + <para>
> + The include-generated-columns option controls whether
> generated columns should be included in the string representation of
> tuples during logical decoding in PostgreSQL. This allows users to
> customize the output format based on whether they want to include
> these columns or not.
> + </para>
> + </listitem>
> + </varlistentry>
>
> 2a.
> Something is not correct when this name has hyphens and all the nearby
> parameter names do not. Shouldn't it be all uppercase like the other
> boolean parameter?
>
> ~
>
> 2b.
> Text in the SGML file should be wrapped properly.
>
> ~
>
> 2c.
> IMO the comment can be more terse and it also needs to specify that it
> is a boolean type, and what is the default value if not passed.
>
> SUGGESTION
>
> INCLUDE_GENERATED_COLUMNS [ boolean ]
>
> If true, then generated columns should be included in the string
> representation of tuples during logical decoding in PostgreSQL. The
> default is false.
Fixed.
> ======
> src/backend/replication/logical/proto.c
>
> 3. logicalrep_write_tuple
>
> - if (!column_in_column_list(att->attnum, columns))
> + if (!column_in_column_list(att->attnum, columns) && !att->attgenerated)
> + continue;
> +
> + if (att->attgenerated && !publish_generated_column)
> continue;
>
> 3a.
> This code seems overcomplicated checking the same flag multiple times.
>
> SUGGESTION
> if (att->attgenerated)
> {
> if (!publish_generated_column)
> continue;
> }
> else
> {
> if (!column_in_column_list(att->attnum, columns))
> continue;
> }
>
> ~
>
> 3b.
> The same logic occurs several times in logicalrep_write_tuple
Fixed.
> 4. logicalrep_write_attrs
>
> if (!column_in_column_list(att->attnum, columns))
> continue;
>
> + if (att->attgenerated && !publish_generated_column)
> + continue;
> +
>
> Shouldn't these code fragments (2x in this function) look the same as
> in logicalrep_write_tuple? See the above review comments.
Fixed.
> ======
> src/backend/replication/pgoutput/pgoutput.c
>
> 5. maybe_send_schema
>
> TransactionId topxid = InvalidTransactionId;
> + bool publish_generated_column = data->publish_generated_column;
>
> I'm not convinced this saves any code, and anyway, it is not
> consistent with other fields in this function that are not extracted
> to another variable (e.g. data->streaming).
Fixed.
> 6. pgoutput_change
> -
> + bool publish_generated_column = data->publish_generated_column;
> +
>
> I'm not convinced this saves any code, and anyway, it is not
> consistent with other fields in this function that are not extracted
> to another variable (e.g. data->binary).
Fixed.
> ======
> [1] My v1 review -
> https://www.postgresql.org/message-id/CAHut+PsuJfcaeg6zst=6pe5uyjv_uxvrhu3ck7w2ahb1uqy...@mail.gmail.com
> [2] My v2 review -
> https://www.postgresql.org/message-id/CAHut%2BPv4RpOsUgkEaXDX%3DW2rhHAsJLiMWdUrUGZOcoRHuWj5%2BQ%40mail.gmail.com
Patch v4-0001 contains all the changes required. See [1] for the changes added.
[1]
https://www.postgresql.org/message-id/CAHv8RjJcOsk%3Dy%2BvJ3y%2BvXhzR9ZUzUEURvS_90hQW3MNfJ5di7A%40mail.gmail.com
Thanks and Regards,
Shubham Khanna.
