Re: Pgoutput not capturing the generated columns

Hi Shlok, Here are some review comments for patch v15-0003.

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

1. publication_translate_columns

The function comment says:
 * 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.

That part about "[no] generated attributes" seems to have gone stale
-- e.g. not quite correct anymore. Should it say no VIRTUAL generated
attributes?

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

2. logicalrep_write_tuple and logicalrep_write_attrs

I thought all the code fragments like this:

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

don't need to be in the code anymore, because of the BitMapSet (BMS)
processing done to make the "column list" for publication where
disallowed generated cols should already be excluded from the BMS,
right?

So shouldn't all these be detected by the following statement:
if (!column_in_column_list(att->attnum, columns))
  continue;

======
src/backend/replication/logical/tablesync.c
3.
+ if(server_version >= 120000)
+ {
+ bool gencols_allowed = server_version >= 170000 &&
MySubscription->includegencols;
+
+ if (gencols_allowed)
+ {

Should say server_version >= 180000, instead of 170000

======
src/backend/replication/pgoutput/pgoutput.c

4. send_relation_and_attrs

(this is a similar comment for #2 above)

IIUC of the advantages of the BitMapSet (BMS) idea in patch 0001 to
process the generated columns up-front means there is no need to check
them again in code like this.

They should be discovered anyway in the subsequent check:
/* Skip this attribute if it's not present in the column list */
if (columns != NULL && !bms_is_member(att->attnum, columns))
  continue;

======
src/test/subscription/t/011_generated.pl

5.
AFAICT there are still multiple comments (e.g. for the "TEST tab<n>"
comments) where it still says "generated" instead of "stored
generated". I did not make a "nitpicks" diff for these because those
comments are inherited from the prior patch 0002 which still has
outstanding review comments on it too. Please just search/replace
them.

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