Re: row filtering for logical replication

On Tue, Dec 14, 2021 at 10:12 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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?
>

Fixed in v47 [1]

> 2.
> +Node *
> +GetTransformedWhereClause(ParseState *pstate, PublicationRelInfo *pri,
> + bool bfixupcollation)
>
> Can we add comments atop this function?
>

Fixed in v47 [1]

> 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.
>

Fixed in v47 [1]

>
> 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?
>

Fixed in v47 [1]

------
[1] https://www.postgresql.org/message-id/CAHut%2BPtjsj_OVMWEdYp2Wq19%3DH5D4Vgta43FbFVDYr2LuS_djg%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia