RE: row filtering for logical replication

On Wed, Dec 29, 2021 11:16 AM Tang, Haiying <tanghy(dot)fnst(at)fujitsu(dot)com> wrote:
> On Mon, Dec 27, 2021 9:16 PM houzj(dot)fnst(at)fujitsu(dot)com <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> > On Thur, Dec 23, 2021 4:28 PM Peter Smith <smithpb2250(at)gmail(dot)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