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
v1-0001-Add-column-filtering-to-logical-replication.patch
Description: Binary data
