Re: Conflict Detection and Resolution - Mailing list pgsql-hackers

On Thu, 12 Sept 2024 at 14:03, Ajin Cherian <itsajin@gmail.com> wrote:
>
> On Tue, Sep 3, 2024 at 7:42 PM vignesh C <vignesh21@gmail.com> wrote:
>
>     On Fri, 30 Aug 2024 at 11:01, Nisha Moond <nisha.moond412@gmail.com> wrote:
>     >
>     > Here is the v11 patch-set. Changes are:
>
>     1) This command crashes:
>     ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR NULL;
>     #0  __strcmp_avx2 () at ../sysdeps/x86_64/multiarch/strcmp-avx2.S:116
>     #1  0x000055c67270600a in ResetConflictResolver (subid=16404,
>     conflict_type=0x0) at conflict.c:744
>     #2  0x000055c67247e0c3 in AlterSubscription (pstate=0x55c6748ff9d0,
>     stmt=0x55c67497dfe0, isTopLevel=true) at subscriptioncmds.c:1664
>
>     +                       | ALTER SUBSCRIPTION name RESET CONFLICT
>     RESOLVER FOR conflict_type
>     +                               {
>     +                                       AlterSubscriptionStmt *n =
>     +                                               makeNode(AlterSubscriptionStmt);
>     +
>     +                                       n->kind =
>     ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER;
>     +                                       n->subname = $3;
>     +                                       n->conflict_type = $8;
>     +                                       $$ = (Node *) n;
>     +                               }
>     +               ;
>     +conflict_type:
>     +                       Sconst
>                      { $$ = $1; }
>     +                       | NULL_P
>                      { $$ = NULL; }
>                     ;
>
>     May be conflict_type should be changed to:
>     +conflict_type:
>     +                       Sconst
>                      { $$ = $1; }
>                     ;
>
>
> Fixed.

Few comments:
1) Tab completion missing for:
a) ALTER SUBSCRIPTION name CONFLICT RESOLVER
b) ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER ALL
c) ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR

2) Documentation missing for:
a) ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER ALL
b) ALTER SUBSCRIPTION name RESET CONFLICT RESOLVER FOR

3) This reset is not required here, if valid was false it would have
thrown an error and exited:
a)
+       if (!valid)
+               ereport(ERROR,
+                               errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                               errmsg("%s is not a valid conflict
type", conflict_type));
+
+       /* Reset */
+       valid = false;

b)
Similarly here too:
+       if (!valid)
+               ereport(ERROR,
+                               errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+                               errmsg("%s is not a valid conflict
resolver", conflict_resolver));
+
+       /* Reset */
+       valid = false;

4) How about adding CT_MAX inside the enum itself as the last enum value:
typedef enum
{
/* The row to be inserted violates unique constraint */
CT_INSERT_EXISTS,

/* The row to be updated was modified by a different origin */
CT_UPDATE_ORIGIN_DIFFERS,

/* The updated row value violates unique constraint */
CT_UPDATE_EXISTS,

/* The row to be updated is missing */
CT_UPDATE_MISSING,

/* The row to be deleted was modified by a different origin */
CT_DELETE_ORIGIN_DIFFERS,

/* The row to be deleted is missing */
CT_DELETE_MISSING,

/*
* Other conflicts, such as exclusion constraint violations, involve more
* complex rules than simple equality checks. These conflicts are left for
* future improvements.
*/
} ConflictType;

#define CONFLICT_NUM_TYPES (CT_DELETE_MISSING + 1)

/* Min and max conflict type */
#define CT_MIN CT_INSERT_EXISTS
#define CT_MAX CT_DELETE_MISSING

and the for loop can be changed to:
for (type = 0; type < CT_MAX; type++)

This way CT_MIN can be removed and CT_MAX need not be changed every
time a new enum is added.

Also the following +1 can be removed from the variables:
ConflictTypeResolver conflictResolvers[CT_MAX + 1];

5) Similar thing can be done with ConflictResolver enum too. i.e
remove  CR_MIN and add CR_MAX as the last element of enum
typedef enum ConflictResolver
{
/* Apply the remote change */
CR_APPLY_REMOTE = 1,

/* Keep the local change */
CR_KEEP_LOCAL,

/* Apply the remote change; skip if it can not be applied */
CR_APPLY_OR_SKIP,

/* Apply the remote change; emit error if it can not be applied */
CR_APPLY_OR_ERROR,

/* Skip applying the change */
CR_SKIP,

/* Error out */
CR_ERROR,
} ConflictResolver;

/* Min and max conflict resolver */
#define CR_MIN CR_APPLY_REMOTE
#define CR_MAX CR_ERROR

6) Except scansup.h inclusion, other inclusions added are not required
in subscriptioncmds.c file.

7)The inclusions "access/heaptoast.h",  "access/table.h",
"access/tableam.h", "catalog/dependency.h",
"catalog/pg_subscription.h",  "catalog/pg_subscription_conflict.h" and
"catalog/pg_inherits.h" are not required in conflict.c file.

8) Can we change this to  use the new foreach_ptr implementations added:
+       foreach(lc, stmtresolvers)
+       {
+               DefElem    *defel = (DefElem *) lfirst(lc);
+               ConflictType type;
+               char       *resolver;

to use foreach_ptr like:
foreach_ptr(DefElem, defel, stmtresolvers)
{
+               ConflictType type;
+               char       *resolver;
....
}

Regards,
Vignesh