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
