RE: row filtering for logical replication

On Wed, Dec 29, 2021 11:16 AM Tang, Haiying <tanghy.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 0003 patch.
> > 1. Added a test to cover the case where TOASTed values are not included in the
> >    new tuple. Suggested by Euler[1].
> >
> >    Note: this test is temporarily commented because it would fail without
> >    applying another bug fix patch in another thread[2] which log the
> detoasted
> >    value in old value. I have verified locally that the test pass after
> >    applying the bug fix patch[2].
> >
> > 2. Add a test to cover the case that transform the UPDATE into INSERT.
> Provided
> >    by Tang.
> >
> 
> Thanks for updating the patches.
> 
> A few comments:
> 
> 1) v55-0001
> 
> -/*
> - * Gets the relations based on the publication partition option for a specified
> - * relation.
> - */
>  List *
>  GetPubPartitionOptionRelations(List *result, PublicationPartOpt pub_partopt,
>                                 Oid relid)
> 
> Do we need this change?

Added the comment back.

> 2) v55-0002
>      * Multiple ExprState entries might be used if there are multiple
>       * publications for a single table. Different publication actions don't
>       * allow multiple expressions to always be combined into one, so there is
> -     * one ExprSTate per publication action. Only 3 publication actions are
> +     * one ExprState per publication action. Only 3 publication actions
> +are
>       * used for row filtering ("insert", "update", "delete"). The exprstate
>       * array is indexed by ReorderBufferChangeType.
>       */
> 
> I think this change can be merged into 0001 patch.

Merged.

> 3) v55-0002
> +static bool pgoutput_row_filter_update_check(enum
> ReorderBufferChangeType changetype, Relation relation,
> +
>      HeapTuple oldtuple, HeapTuple newtuple,
> +
>      RelationSyncEntry *entry, ReorderBufferChangeType *action);
> 
> Do we need parameter changetype here? I think it could only be
> REORDER_BUFFER_CHANGE_UPDATE.

I didn't change this, I think it might be better to wait for Ajin's opinion.

Attach the v56 patch set which address above comments and comments(1. 2.) from [1]

[1]
https://www.postgresql.org/message-id/OS3PR01MB62756D18BA0FA969D5255E369E459%40OS3PR01MB6275.jpnprd01.prod.outlook.com

Best regards,
Hou zj