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

From Ajin Cherian
Subject Re: Conflict Detection and Resolution
Date
Msg-id CAFPTHDaHu1eBb-jvdXjOOPMq0bPyb80uwYNy2H3usyKYX1PW=g@mail.gmail.com
Whole thread Raw
In response to Re: Conflict Detection and Resolution  (vignesh C <vignesh21@gmail.com>)
List pgsql-hackers


On Fri, Sep 13, 2024 at 10:20 PM vignesh C <vignesh21@gmail.com> wrote:

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


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


Added.
 
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;


Actually, the reset is for when valid becomes true. I think it it is required here.
 
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];


I tried changing this, but the enums are used in swicth cases and this throws a compiler warning that CT_MAX is not checked in the switch case. However, I have changed the use of (CT_MAX +1)  and instead used CONFLICT_NUM_TYPES in those places.

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


 same as previous comment.
 
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.


Removed.
 
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;
....
}

Changed accordingly.

regards,
Ajin Cherian
Fujitsu Australia

pgsql-hackers by date:

Previous
From: Amit Kapila
Date:
Subject: Re: Pgoutput not capturing the generated columns
Next
From: Tom Lane
Date:
Subject: Re: attndims, typndims still not enforced, but make the value within a sane threshold