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