RE: row filtering for logical replication

On Fri, Dec 17, 2021 6:09 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Dec 17, 2021 at 4:11 AM Peter Smith <smithpb2250@gmail.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()?

Thanks for the comments, I agree with all the comments.
Attach the V49 patch set, which addressed all the above comments on the 0002
patch.

Best regards,
Hou zj