Re: Column Filtering in Logical Replication

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.

> The patch adds a new parse node PublicationTable, but doesn't add
> copyfuncs.c, equalfuncs.c, readfuncs.c, outfuncs.c support for it.
> Maybe try a compile with WRITE_READ_PARSE_PLAN_TREES and/or
> COPY_PARSE_PLAN_TREES enabled to make sure everything is covered.
> (I didn't verify that this actually catches anything ...)
>

I will test this and include these changes in the next version.

> The new column in pg_publication_rel is prrel_attr. This name seems at
> odds with existing column names (we don't use underscores in catalog
> column names). Maybe prattrs is good enough? prrelattrs? We tend to
> use plurals for columns that are arrays.
>
> Renamed it to prattrs as per suggestion.

It's not super clear to me that strlist_to_textarray() and related
> processing will behave sanely when the column names contain weird
> characters such as commas or quotes, or just when used with uppercase
> column names. Maybe it's worth having tests that try to break such
> cases.
>
> Sure, I will include these tests in the next version.

> You seem to have left a debugging "elog(LOG)" line in OpenTableList.
>
> Removed.

> I got warnings from "git am" about trailing whitespace being added by
> the patch in two places.
>
> Should be fixed now.

Thank you,
Rahila Syed