Re: row filtering for logical replication

On Thu, Nov 25, 2021 at 12:03 AM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Nov 23, 2021 at 4:58 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> >
> > Attaching a new patchset v41 which includes changes by both Peter and myself.
> >
> > Patches v40-0005 and v40-0006 have been merged to create patch
> > v41-0005 which reduces the patches to 6 again.
> > This patch-set contains changes addressing the following review comments:
> >
> > On Mon, Nov 15, 2021 at 5:48 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > >
> > > What I meant was that with this new code we have regressed the old
> > > behavior. Basically, imagine a case where no filter was given for any
> > > of the tables. Then after the patch, we will remove all the old tables
> > > whereas before the patch it will remove the oldrels only when they are
> > > not specified as part of new rels. If you agree with this, then we can
> > > retain the old behavior and for the new tables, we can always override
> > > the where clause for a SET variant of command.
> >
> > Fixed and modified the behaviour to match with what the schema patch
> > implemented.
> >
>
> +
> + /*
> + * If the new relation or the old relation has a where clause,
> + * we need to remove it so that it can be added afresh later.
> + */
> + if (RelationGetRelid(newpubrel->relation) == oldrelid &&
> + newpubrel->whereClause == NULL && rfisnull)
>
> Can't we use _equalPublicationTable() here? It compares the whereClause as well.
>

Fixed in v44 [1]

> Few more comments:
> =================
> 0001
...

.
> 3. In the function header comment of rowfilter_walker, you mentioned
> the simple expressions allowed but we should write why we are doing
> so. It has been discussed in detail in various emails in this thread.
> AFAIR, below are the reasons:
> A. We don't want to allow user-defined functions or operators because
> (a) if the user drops such a function/operator or if there is any
> other error via that function, the walsender won't be able to recover
> from such an error even if we fix the function's problem because it
> uses a historic snapshot to access row-filter; (b) any other table
> could be accessed via a function which won't work because of historic
> snapshots in logical decoding environment.
>
> B. We don't allow anything other immutable built-in functions as those
> can access database and would lead to the problem (b) mentioned in the
> previous paragraph.
>

Updated comment in v44 [1]

> Don't we need to check for user-defined types similar to user-defined
> functions and operators? If not why?

Fixed in v44 [1]

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

Kind Regards,
Peter Smith.
Fujitsu Australia