On Wed, Jan 5, 2022 at 2:45 PM Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Tue, Dec 28, 2021 at 6:33 PM houzj.f...@fujitsu.com > <houzj.f...@fujitsu.com> wrote: > > > > On Mon, Dec 27, 2021 9:19 PM Hou Zhijie <houzj.f...@fujitsu.com> wrote: > > > On Mon, Dec 27, 2021 9:16 PM houzj.f...@fujitsu.com > > > <houzj.f...@fujitsu.com> > > > wrote: > > > > On Thur, Dec 23, 2021 4:28 PM Peter Smith <smithpb2...@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.
