Here are some review comments for v65-0001 (review of updates since v64-0001)
~~~ 1. src/include/commands/publicationcmds.h - rename func +extern bool contain_invalid_rfcolumn(Oid pubid, Relation relation, + List *ancestors, + AttrNumber *invalid_rfcolumn); I thought that function should be called "contains_..." instead of "contain_...". ~~~ 2. src/backend/commands/publicationcmds.c - rename funcs Suggested renaming (same as above #1). "contain_invalid_rfcolumn_walker" --> "contains_invalid_rfcolumn_walker" "contain_invalid_rfcolumn" --> "contains_invalid_rfcolumn" Also, update it in the comment for rf_context: +/* + * Information used to validate the columns in the row filter expression. See + * contain_invalid_rfcolumn_walker for details. + */ ~~~ 3. src/backend/commands/publicationcmds.c - bms + if (!rfisnull) + { + rf_context context = {0}; + Node *rfnode; + Bitmapset *bms = NULL; + + context.pubviaroot = pub->pubviaroot; + context.parentid = publish_as_relid; + context.relid = relid; + + /* + * Remember columns that are part of the REPLICA IDENTITY. Note that + * REPLICA IDENTITY DEFAULT means primary key or nothing. + */ + if (relation->rd_rel->relreplident == REPLICA_IDENTITY_DEFAULT) + bms = RelationGetIndexAttrBitmap(relation, + INDEX_ATTR_BITMAP_PRIMARY_KEY); + else if (relation->rd_rel->relreplident == REPLICA_IDENTITY_INDEX) + bms = RelationGetIndexAttrBitmap(relation, + INDEX_ATTR_BITMAP_IDENTITY_KEY); + + context.bms_replident = bms; There seems no need for a separate 'bms' variable here. Why not just assign directly to context.bms_replident like the code used to do? ~~~ 4. src/backend/utils/cache/relcache.c - typo? /* - * If we know everything is replicated, there is no point to check for - * other publications. + * Check, if all columns referenced in the filter expression are part + * of the REPLICA IDENTITY index or not. + * + * If we already found the column in row filter which is not part of + * REPLICA IDENTITY index, skip the validation. */ Shouldn't that comment say "already found a column" instead of "already found the column"? ~~~ 5. src/backend/replication/pgoutput/pgoutput.c - map member @@ -129,7 +169,7 @@ typedef struct RelationSyncEntry * same as 'relid' or if unnecessary due to partition and the ancestor * having identical TupleDesc. */ - TupleConversionMap *map; + AttrMap *map; } RelationSyncEntry; I wondered if you should also rename this member to something more meaningful like 'attrmap' instead of just 'map'. ------ Kind Regards, Peter Smith. Fujitsu Australia