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: