Re: Pgoutput not capturing the generated columns

Hi, here are some review comments about patch v17-0003

======
1.
Missing a docs change?

Previously, (v16-0002) the patch included a change to
doc/src/sgml/protocol.sgml like below to say STORED generated instead
of just generated.

        <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.
+        Boolean option to enable <literal>STORED</literal> generated columns.
+        This option controls whether <literal>STORED</literal>
generated columns
+        should be included in the string representation of tuples
during logical
+        decoding in PostgreSQL.
        </para>

Why is that v16 change no longer present in patch v17-0003?

======
src/backend/catalog/pg_publication.c

2.
Previously, (v16-0003) this patch included a change to clarify the
kind of generated cols that are allowed in a column list.

  * Translate a list of column names to an array of attribute numbers
  * and a Bitmapset with them; verify that each attribute is appropriate
- * to have in a publication column list (no system or generated attributes,
- * no duplicates).  Additional checks with replica identity are done later;
- * see pub_collist_contains_invalid_column.
+ * to have in a publication column list (no system or virtual generated
+ * attributes, no duplicates). Additional checks with replica identity
+ * are done later; see pub_collist_contains_invalid_column.

Why is that v16 change no longer present in patch v17-0003?

======
src/backend/replication/logical/tablesync.c

3. make_copy_attnamelist

- if (!attr->attgenerated)
+ if (attr->attgenerated != ATTRIBUTE_GENERATED_STORED)
  continue;

IIUC this logic is checking to make sure the subscriber-side table
column was not a generated column (because we don't replicate on top
of generated columns). So, does the distinction of STORED/VIRTUAL
really matter here?

~~~

fetch_remote_table_info:
nitpick - Should not change any spaces unrelated to the patch

======

send_relation_and_attrs:

- if (att->attgenerated && !include_generated_columns)
+ if (att->attgenerated && (att->attgenerated !=
ATTRIBUTE_GENERATED_STORED || !include_generated_columns))
  continue;

nitpick - It seems over-complicated. Conditions can be split so the
code fragment looks the same as in other places in this patch.

======
99.
Please see the attached diffs patch that implements any nitpicks
mentioned above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia