Re: row filtering for logical replication

On Thu, Nov 4, 2021 at 2:21 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> 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.

Fixed in v37-0001 [1].

>
> 2)
> +                                       if ($3)
> +                                       {
> +                                               $$->pubtable->whereClause = $3;
> +                                       }
>
> It seems we can remove the if ($3) check here.
>

Fixed in v37-0001 [1].

>
> 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.

Yes, thanks for this suggestion - it is an interesting idea. I had
thought the same as this some time ago but never acted on it. I will
try implementing this idea as a separate new patch because it probably
needs to be performance tested against the current code just in case
the extra effort to combine the expressions outweighs any execution
benefits.

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

Kind Regards,
Peter Smith.
Fujitsu Australia.