On Mon, Dec 27, 2021 9:16 PM houzj.f...@fujitsu.com <houzj.f...@fujitsu.com> 
wrote:
> 
> On Thur, Dec 23, 2021 4:28 PM Peter Smith <smithpb2...@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?

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.

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.

Regards,
Tang

Reply via email to