Re: row filtering for logical replication

On Wed, Jan 5, 2022 at 2:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Tue, Dec 28, 2021 at 6:33 PM houzj.fnst@fujitsu.com
> <houzj.fnst@fujitsu.com> wrote:
> >
> > On Mon, Dec 27, 2021 9:19 PM Hou Zhijie <houzj.fnst@fujitsu.com> wrote:
> > > On Mon, Dec 27, 2021 9:16 PM houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
> > > wrote:
> > > > On Thur, Dec 23, 2021 4:28 PM Peter Smith <smithpb2250@gmail.com> wrote:
> > > > > Here is the v54* patch set:
> > > >
> > > > Attach the v55 patch set which add the following testcases in 0002 patch.
> >
> > When reviewing the row filter patch, I found few things that could be improved.
> > 1) We could transform the same row filter expression twice when
> >    ALTER PUBLICATION ... SET TABLE WHERE (...). Because we invoke
> >    GetTransformedWhereClause in both AlterPublicationTables() and
> >    publication_add_relation(). I was thinking it might be better if we only
> >    transform the expression once in AlterPublicationTables().
> >
> > 2) When transforming the expression, we didn’t set the correct p_sourcetext.
> >    Since we need to transforming serval expressions which belong to different
> >    relations, I think it might be better to pass queryString down to the actual
> >    transform function and set p_sourcetext to the actual queryString.
> >
>
> I have tried the following few examples to check the error_position
> and it seems to be showing correct position without your 0004 patch.
> postgres=# create publication pub for table t1 where (10);
> ERROR:  argument of PUBLICATION WHERE must be type boolean, not type integer
> LINE 1: create publication pub for table t1 where (10);
>
>          ^
>

I understand why the error position could vary even though it is
showing the correct location in the above example after reading
another related email [1].

> Also, transformPubWhereClauses() seems to be returning the same list
> as it was passed to it. Do we really need to return anything from
> transformPubWhereClauses()?
>

One more point about this function: the patch seems to be doing some
work even when where clause is not specified which can be avoided.

Another minor comment:
+static bool pgoutput_row_filter(enum ReorderBufferChangeType changetype,

Do we need to specify the 'enum' type before changetype parameter?

[1] - https://www.postgresql.org/message-id/1513381.1640626456%40sss.pgh.pa.us

--
With Regards,
Amit Kapila.