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?

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.

------
[1] https://www.postgresql.org/message-id/CALDaNm08Ynr_FzNg%2BdoHj%3D_nBet%2BKZAvNbqmkEEw7M2SPpPEAw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia