Re: row filtering for logical replication

On Fri, Dec 17, 2021 at 4:11 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> PSA the v47* patch set.
>

Few comments on v47-0002:
=======================
1. The handling to find rowfilter for ancestors in
RelationGetInvalidRowFilterCol seems complex. It seems you are
accumulating non-partition relations as well in toprelid_in_pub. Can
we simplify such that we find the ancestor only for 'pubviaroot'
publications?

2. I think the name RelationGetInvalidRowFilterCol is confusing
because the same function is also used to get publication actions. Can
we name it as GetRelationPublicationInfo() and pass a bool parameter
to indicate whether row_filter info needs to be built. We can get the
invalid_row_filter column as output from that function.

3.
+GetRelationPublicationActions(Relation relation)
{
..
+ if (!relation->rd_pubactions)
+ (void) RelationGetInvalidRowFilterCol(relation);
+
+ return memcpy(pubactions, relation->rd_pubactions,
+ sizeof(PublicationActions));
..
..
}

I think here we can reverse the check such that if actions are set
just do memcpy and return otherwise get the relationpublicationactions
info.

4.
invalid_rowfilter_column_walker
{
..

/*
* If pubviaroot is true, we need to convert the column number of
* parent to the column number of child relation first.
*/
if (context->pubviaroot)
{
char *colname = get_attname(context->parentid, attnum, false);
attnum = get_attnum(context->relid, colname);
}

Here, in the comments, you can tell why you need this conversion. Can
we name this function as rowfilter_column_walker()?

5.
+/* For invalid_rowfilter_column_walker. */
+typedef struct {
+ AttrNumber invalid_rfcolnum; /* invalid column number */
+ Bitmapset *bms_replident; /* bitset of replica identity col indexes */
+ bool pubviaroot; /* true if we are validating the parent
+ * relation's row filter */
+ Oid relid; /* relid of the relation */
+ Oid parentid; /* relid of the parent relation */
+} rf_context;

Normally, we declare structs at the beginning of the file and for the
formatting of struct declarations, see other nearby structs like
RelIdCacheEnt.

6. Can we name IsRowFilterSimpleNode() as IsRowFilterSimpleExpr()?

--
With Regards,
Amit Kapila.