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.
======
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.
======
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.
======
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.
======
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
~~~
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.
======
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).
~~~
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).
======
[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
Kind Regards,
Peter Smith.
Fujitsu Australia
