Re: Column Filtering in Logical Replication

Here are some v5 review comments for your consideration:

------

1. src/backend/access/common/relation.c

@@ -215,3 +217,22 @@ relation_close(Relation relation, LOCKMODE lockmode)
if (lockmode != NoLock)
UnlockRelationId(&relid, lockmode);
}
+
+/*
+ * Return a bitmapset of attributes given the list of column names
+ */
+Bitmapset*
+get_table_columnset(Oid relid, List *columns, Bitmapset *att_map)
+{

IIUC that 3rd parameter (att_map) is always passed as NULL to
get_table_columnset function because you are constructing this
Bitmapset from scratch. Maybe I am mistaken, but if not then what is
the purpose of that att_map parameter?

------

2. src/backend/catalog/pg_publication.c

+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("cannot add relation \"%s\" to publication",
+ RelationGetRelationName(targetrel)),
+ errdetail("Column filter must include REPLICA IDENTITY columns")));

Is ERRCODE_INVALID_COLUMN_REFERENCE a more appropriate errcode to use here?

------

3. src/backend/catalog/pg_publication.c

+ else
+ {
+ Bitmapset *filtermap = NULL;
+ idattrs = RelationGetIndexAttrBitmap(targetrel,
INDEX_ATTR_BITMAP_IDENTITY_KEY);

The RelationGetIndexAttrBitmap function comment says "should be
bms_free'd when not needed anymore" but it seems the patch code is not
freeing idattrs when finished using it.

------
Kind Regards,
Peter Smith.
Fujitsu Australia