Re: row filtering for logical replication

On Tue, Dec 14, 2021 at 10:50 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Dec 14, 2021 at 4:44 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Few other comments:
> ===================

Few more comments:
==================
v46-0001/0002
===============
1. After rowfilter_walker() why do we need
EXPR_KIND_PUBLICATION_WHERE? I thought this is primarily to identify
the expressions that are not allowed in rowfilter which we are now
able to detect upfront with the help of a walker. Can't we instead use
EXPR_KIND_WHERE?

2.
+Node *
+GetTransformedWhereClause(ParseState *pstate, PublicationRelInfo *pri,
+ bool bfixupcollation)

Can we add comments atop this function?

3. In GetTransformedWhereClause, can we change the name of variables
(a) bfixupcollation to fixup_collation or assign_collation, (b)
transformedwhereclause to whereclause. I think that will make the
function more readable.

v46-0002
========
4.
+ else if (IsA(node, List) || IsA(node, Const) || IsA(node, BoolExpr)
|| IsA(node, NullIfExpr) ||
+ IsA(node, NullTest) || IsA(node, BooleanTest) || IsA(node, CoalesceExpr) ||
+ IsA(node, CaseExpr) || IsA(node, CaseTestExpr) || IsA(node, MinMaxExpr) ||
+ IsA(node, ArrayExpr) || IsA(node, ScalarArrayOpExpr) || IsA(node, XmlExpr))

Can we move this to a separate function say IsValidRowFilterExpr() or
something on those lines and use Switch (nodetag(node)) to identify
these nodes?

--
With Regards,
Amit Kapila.