On Mon, 8 Jul 2024 at 13:20, Peter Smith <smithpb2250@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