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.