On 7/12/21 11:38 AM, Rahila Syed wrote:
> Hi Alvaro,
>
> Thank you for comments.
>
> The patch adds a function get_att_num_by_name; but we have a lsyscache.c
> function for that purpose, get_attnum. Maybe that one should be used
> instead?
>
> Thank you for pointing that out, I agree it makes sense to reuse the
> existing function.
> Changed it accordingly in the attached patch.
>
>
> get_tuple_columns_map() returns a bitmapset of the attnos of the columns
> in the given list, so its name feels wrong. I propose
> get_table_columnset(). However, this function is invoked for every
> insert/update change, so it's going to be far too slow to be usable. I
> think you need to cache the bitmapset somewhere, so that the function is
> only called on first use. I didn't look very closely, but it seems that
> struct RelationSyncEntry may be a good place to cache it.
>
> Makes sense, changed accordingly.
>
To nitpick, I find "Bitmapset *att_list" a bit annoying, because it's
not really a list ;-)
FWIW "make check" fails for me with this version, due to segfault in
OpenTableLists. Apparenly there's some confusion - the code expects the
list to contain PublicationTable nodes, and tries to extract the
RangeVar from the elements. But the list actually contains RangeVar, so
this crashes and burns. See the attached backtrace.
I'd bet this is because the patch uses list of RangeVar in some cases
and list of PublicationTable in some cases, similarly to the "row
filtering" patch nearby. IMHO this is just confusing and we should
always pass list of PublicationTable nodes.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
