Hello, here are a few comments on this patch. 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?
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. 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 ...) 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. 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. You seem to have left a debugging "elog(LOG)" line in OpenTableList. I got warnings from "git am" about trailing whitespace being added by the patch in two places. Thanks! -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/
