On Mon, 8 Jul 2024 at 13:20, Peter Smith <smithpb2...@gmail.com> wrote: > > 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? Yes, we should use VIRTUAL generated attributes, I have modified it.
> ====== > 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; The current BMS logic do not handle the Virtual Generated Columns. There can be cases where we do not want a virtual generated column but it would be present in BMS. To address this I have added the above logic. I have added this logic similar to the checks of 'attr->attisdropped'. > ====== > 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; Same explanation as above. I have addressed all the comments in v16-0003 patch. Please refer [1]. [1]: https://www.postgresql.org/message-id/CANhcyEXw%3DBFFVUqohWES9EPkdq-ZMC5QRBVQqQPzrO%3DQ7uzFQw%40mail.gmail.com Thanks and Regards, Shlok Kyal
