On Fri, Sep 13, 2024 at 10:20 PM vignesh C <vignes...@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
