Re: row filtering for logical replication

On Wed, Dec 15, 2021 at 6:47 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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?
>
> FYI - I have tried this locally and all tests pass.
>
> ~~
>
> If the EXPR_KIND_PUBLICATION_WHERE is removed then there will be some
> differences:
> - we would get errors for aggregate/grouping functions from the EXPR_KIND_WHERE
> - we would get errors for windows functions from the EXPR_KIND_WHERE
> - we would get errors for set-returning functions from the EXPR_KIND_WHERE
>
> Actually, IMO this would be a *good* change because AFAIK those are
> not all being checked by the row-filter walker. I think the only
> reason all tests pass is that there are no specific regression tests
> for these cases.
>
> OTOH, there would also be a difference where an error message would
> not be as nice. Please see the review comment from Vignesh. [1] The
> improved error message is only possible by checking the
> EXPR_KIND_PUBLICATION_WHERE.
>
> ~~
>
> I think the best thing to do here is to leave the
> EXPR_KIND_PUBLICATION_WHERE but simplify code so that the improved
> error message remains as the *only* difference in behaviour from the
> EXPR_KIND_WHERE. i.e. we should let the other
> aggregate/grouping/windows/set function checks give errors exactly the
> same as for the EXPR_KIND_WHERE case.
>

I am not sure if "the better error message" is a good enough reason
to introduce this new kind. I thought it is better to deal with that
in rowfilter_walker.

--
With Regards,
Amit Kapila.