RE: row filtering for logical replication

On Wednesday, November 3, 2021 8:51 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> On Tue, Nov 2, 2021 at 10:44 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
> .
> >
> > The patch 0005 and 0006 has not yet been rebased but will be updated
> > in a few days.
> >
>
> Here's a rebase of all the 6 patches. Issue remaining:

Thanks for the patches.
I started to review the patches and here are a few comments.

1)
/*
* ALTER PUBLICATION ... ADD TABLE provides a PublicationTable List
* (Relation, Where clause). ALTER PUBLICATION ... DROP TABLE provides
* a Relation List. Check the List element to be used.
*/
if (IsA(lfirst(lc), PublicationTable))
whereclause = true;
else
whereclause = false;

I am not sure about the comments here, wouldn't it be better to always provides
PublicationTable List which could be more consistent.

2)
+ if ($3)
+ {
+ $$->pubtable->whereClause = $3;
+ }

It seems we can remove the if ($3) check here.

3)

+ oldctx = MemoryContextSwitchTo(CacheMemoryContext);
+ rfnode = stringToNode(TextDatumGetCString(rfdatum));
+ exprstate = pgoutput_row_filter_init_expr(rfnode);
+ entry->exprstates = lappend(entry->exprstates, exprstate);
+ MemoryContextSwitchTo(oldctx);
+ }

Currently in the patch, it save and execute each expression separately. I was
thinking it might be better if we can use "AND" to combine all the expressions
into one expression, then we can initialize and optimize the final expression
and execute it only once.

Best regards,
Hou zj