Re: Conflict Detection and Resolution

Hi, here are my code/test review comments for patch v14-0001.

(the v14-0001 docs review was already previously posted)

======
src/backend/commands/subscriptioncmds.c

parse_subscription_conflict_resolvers:

1.
+ /* Check if the conflict type already exists in the list */
+ if (list_member(SeenTypes, makeString(defel->defname)))

This 'SeenTypes' logic of string comparison of list elements all seems
overkill to me.

There is a known number of conflict types, so why not use a more
efficient bool array:
e.g. bool seen_conflict_type[CONFLICT_NUM_TYPES] = {0};

NOTE - I've already made this change in the nits attachment to
demonstrate that it works fine.

~

2.
+ foreach(lc, stmtresolvers)
+ {
+ DefElem *defel = (DefElem *) lfirst(lc);

There are macros to do this, so you don't need to use lfirst(). e.g.
foreach_ptr?

~

3.
nit (a) - remove excessive blank lines
nit (b) - minor comment reword + periods.

~~~

CreateSubscription:

4.
bits32 supported_opts;
SubOpts opts = {0};
AclResult aclresult;
+ ConflictTypeResolver conflictResolvers[CONFLICT_NUM_TYPES];

nit - Suggest rename to 'conflict_resolvers' to keep similar style to
other variables.

~

5.
nit - add a missing period to some comments.

~~~

AlterSubscription:

6.
nit (a) - remove excessing blank lines.
nit (b) - add missing period in comments.
nit (c) - rename 'conflictResolvers' to 'conflict_resolvers' for
consistent variable style

======
src/backend/parser/gram.y

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

Why does this only support resetting resolvers for one conflict type at a time?

~

8.
nit - add whitespace blank line

======
src/backend/replication/logical/conflict.c

9.
Add some static assertions for all of the arrays to ensure everything
is declared as expected.

(I've made this change in the nit attachment)

~~~

10.
+#define CONFLICT_TYPE_MAX_RESOLVERS 4

It's a bit fiddly introducing this constant just for the map
dimensions. You might as well use the already defined
CONFLICT_NUM_RESOLVERS.

Yes, I know they are not exactly the same. But, the extra few ints of
space wasted reusing this is insignificant, and there is no
performance loss in the lookup logic (after my other review comment
later). IMO it is easier to use what you already have instead of
introducing yet another hard-to-explain constant.

(I've made this change in the nit attachment)

~~~

11.
+static const int ConflictTypeResolverMap[][CONFLICT_TYPE_MAX_RESOLVERS] = {
+ [CT_INSERT_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR},
+ [CT_UPDATE_MISSING] = {CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR, CR_SKIP,
CR_ERROR},
+ [CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR},
+ [CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR}
+};

FYI - There is a subtle quirk (and potential bug) lurking in the way
this map is declared. The array elements for resolvers that are not
defined will default to value 0. So "{CR_SKIP, CR_ERROR}" is
essentially saying "{CR_SKIP, CR_ERROR, 0, 0}", and that means that
the enums for resolvers cannot have a value 0 because then this
ConflictTypeResolverMap would be broken. I see that this was already
handled (enums started at 1) but there was no explanatory comment so
it would be easy to unknowingly break it.

A better way (what I am suggesting here) would be to have a -1 marker
to indicate the end of the list of valid resolvers. This removes any
doubt, and it means the enums can start from 0 as normal, and it means
you can quick-exit from the lookup code (suggested later below) for a
performance gain. Basically, win-win-win.

For example:
* NOTE: -1 is an end marker for the list of valid resolvers for each conflict
* type.
*/
static const int ConflictTypeResolverMap[][CONFLICT_NUM_RESOLVERS+1] = {
[CT_INSERT_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_EXISTS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1},
[CT_UPDATE_MISSING] = {CR_APPLY_OR_SKIP, CR_APPLY_OR_ERROR, CR_SKIP,
CR_ERROR, -1},
[CT_DELETE_MISSING] = {CR_SKIP, CR_ERROR, -1},
[CT_DELETE_ORIGIN_DIFFERS] = {CR_APPLY_REMOTE, CR_KEEP_LOCAL, CR_ERROR, -1}
};

(I have made this change already in the nit attachment)

~~~

12.
+/*
+ * Default conflict resolver for each conflict type.
+ */
+static const int ConflictTypeDefaultResolvers[] = {
+ [CT_INSERT_EXISTS] = CR_ERROR,
+ [CT_UPDATE_EXISTS] = CR_ERROR,
+ [CT_UPDATE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE,
+ [CT_UPDATE_MISSING] = CR_SKIP,
+ [CT_DELETE_MISSING] = CR_SKIP,
+ [CT_DELETE_ORIGIN_DIFFERS] = CR_APPLY_REMOTE
+
+};
+

IMO you can throw all this away. Instead, simply rearrange the
resolvers in ConflictTypeResolverMap[] and provide a comment to say
that the resolver at index [0] of the valid resolver lists is the
"default" resolver for each conflict type.

(I have made this change already in the nit attachment)

~~~

SetDefaultResolvers:

13.
+/*
+ * Set default values for CONFLICT RESOLVERS for each conflict type
+ */
+void
+SetDefaultResolvers(ConflictTypeResolver * conflictResolvers)
+{
+ ConflictType type;
+
+ for (type = CT_MIN; type <= CT_MAX; type++)
+ {
+ conflictResolvers[type].conflict_type = ConflictTypeNames[type];
+ conflictResolvers[type].resolver =
+ ConflictResolverNames[ConflictTypeDefaultResolvers[type]];
+ }
+}

nit (a) - remove extra blank line about this function
nit (b) - add a period to the function comment
nit (c) - tidy the param.
nit (d) - use for-loop variable
nit (e) - don't need CT_MIN and CT_MAX

~~~

validate_conflict_type_and_resolver:

14.
AFAIK, you could also break out of the resolver loop early if you hit
an invalid resolver, because that means you have already fallen off
the end of the valid resolvers.

e.g. do this (in conjunction with the other suggested
ConflictTypeResolverMap changes)

if (candidate < 0)
{
/* No more possible resolvers for this conflict type. */
break;
}

~

15.
nit (a) - use for-loop variable
nit (b) - improve the comments a bit
nit (c) - don't need CT_MIN and CT_MAX
nit (d) - don't need CR_MIN and CR_MAX
nit (e) - use loop variable 'i'
nit (f) - add a blank line before the return
nit (g) - remove excessive blank lines in the function (after the return)

~~~

GetAndValidateSubsConflictResolverList:

16.
nit (a) - minor tweak function comment
nit (b) - put the "ConflictTypeResolver *conftyperesolver = NULL" in
lower scope.
nit (c) - tweak comments for periods etc.

~

17.
+ /* Add the conflict type to the list of seen types */
+ conflictTypes = lappend(conflictTypes,
+ makeString((char *) conftyperesolver->conflict_type));

All this messing with lists of strings seems somewhat inefficient. We
already know the enum by this point (e.g.
validate_conflict_type_and_resolver returns it) so the "seen" logic
should be possible with a simple bool array.

(I've made this change in the nits attachment to give an example of
what I mean. It seems to work just fine).

~~~

UpdateSubConflictResolvers:

18.
+ if (HeapTupleIsValid(oldtup))
+ {
+ /* Update the new resolver */
+ values[Anum_pg_subscription_conflict_confrres - 1] =
+ CStringGetTextDatum(conftyperesolver->resolver);
+ replaces[Anum_pg_subscription_conflict_confrres - 1] = true;
+
+ newtup = heap_modify_tuple(oldtup, RelationGetDescr(pg_subscription_conflict),
+ values, nulls, replaces);
+ CatalogTupleUpdate(pg_subscription_conflict, &oldtup->t_self, newtup);
+ ReleaseSysCache(oldtup);
+ heap_freetuple(newtup);
+ }
+ else
+ elog(ERROR, "cache lookup failed for table conflict %s for subid %u",
+ conftyperesolver->conflict_type, subid);

I think this might be more easily expressed by reversing the condition:

if (!HeapTupleIsValid(oldtup))
elog(ERROR, "cache lookup failed for table conflict %s for subid %u",
conftyperesolver->conflict_type, subid);
...

~~~

ResetConflictResolver:

19.
+ResetConflictResolver(Oid subid, char *conflict_type)
+{
+ ConflictType idx;
+ ConflictTypeResolver conflictResolver;

No point in declaring the variable as a ConflictTypeResolver because
the code is only interested in the resolver name field.

~

20.
nit (a) - Add period to code comments
nit (b) - Remove excess blank lines
nit (c) - Change 'conflict_type' to the words 'conflict type' in a
couple of comments

~~~

conf_detection_check_prerequisites:

21.
Why do we get this warning repeated when just setting the same
resolver as the conflict already has? Even overwriting the resolver
default does this.

~~~

SetSubConflictResolver:

22.
nit (a) - Add period to function comment.
nit (b) - Tidy the spaces in the param declaration
nit (c) - Add period in code comments.

~~~

RemoveSubscriptionConflictResolvers:

23.
nit (a) - Add period to function comment.
nit (b) - Minor change to comment wording.
nit (c) - Add period code to comment.

======
src/bin/pg_dump/pg_dump.c
SKIP REVIEW
======
src/bin/pg_dump/pg_dump.h
SKIP REVIEW

======
src/bin/psql/describe.c

24.
+ if (pset.sversion >= 180000)
+ appendPQExpBuffer(&buf,
+ ", (SELECT string_agg(confrtype || ' = ' || confrres, ', ') \n"
+ " FROM pg_catalog.pg_subscription_conflict c \n"
+ " WHERE c.confsubid = s.oid) AS \"%s\"\n",
+ gettext_noop("Conflict Resolvers"));

Should this SQL include an ORDER BY so the result is the same each time?

~~~

25. Present the information separately.

The current result is a lot of text, which is not particularly
readable. Even when using the "expanded display" mode.

Name | sub1
Owner | postgres
Enabled | t
Publication | {pub1}
Binary | f
Streaming | off
Two-phase commit | d
Disable on error | f
Origin | any
Password required | t
Run as owner? | f
Failover | f
Synchronous commit | off
Conninfo | dbname=test_pub
Skip LSN | 0/0
Conflict Resolvers | insert_exists = error, update_origin_differs =
apply_remote, update_exists = error, update_missing = skip,
delete_origin_differs = apply_remote, delete_missing = skip

I am thinking perhaps the "Conflict Resolver" information should be
presented separately in a list *below* the normal "describe" output.
Other kinds of describe present information this way too (e.g. the
publication lists tables below).

~

26. Identify what was changed by the user

IMO it may also be useful to know which of the conflict resolvers are
the defaults anyway, versus which have been changed to something else
by the user. If you present the information listed separately like
suggested (above) then there would be room to add this additional
info.

======
.../catalog/pg_subscription_conflict.h

27.
+#ifdef CATALOG_VARLEN /* variable-length fields start here */
+ text confrtype BKI_FORCE_NOT_NULL; /* conflict type */
+ text confrres BKI_FORCE_NOT_NULL; /* conflict resolver */
+#endif

These seem strange field names. What do that mean? e.g. for
'confrtype' what is the 'r'?

======
src/include/nodes/parsenodes.h

28.
+ List *resolvers; /* List of conflict resolvers */
+ char *conflict_type; /* conflict_type to be reset */
} AlterSubscriptionStmt;

That 'resolvers' comment should clarify what kind of a list that is.
e.g. Conflict type enums or strings etc.

~~~

29.
+ ALTER_SUBSCRIPTION_CONFLICT_RESOLVERS,
+ ALTER_SUBSCRIPTION_RESET_ALL_CONFLICT_RESOLVERS,
+ ALTER_SUBSCRIPTION_RESET_CONFLICT_RESOLVER,
} AlterSubscriptionType;

I think these names should mimic the actual syntax more closely.

e.g.
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER,
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER_RESET
ALTER_SUBSCRIPTION_CONFLICT_RESOLVER_RESET_ALL

~~~

30.
+ List *resolvers; /* List of conflict resolvers */
+ char *conflict_type; /* conflict_type to be reset */
} AlterSubscriptionStmt;

30a.
(same as earlier review comment #28)
That 'resolvers' comment should clarify what kind of a list that is.
e.g. Conflict type enums or strings etc.

~

30b.
nit (a) - better name here might be "conflict_type_name", so it is not
confused with the enum
nit (b) - fix the comment: e.g. you can't define 'conflict_type' in
terms of 'conflict_type'

======
src/include/replication/conflict.h

31.
+/* Min and max conflict type */
+#define CT_MIN CT_INSERT_EXISTS
+#define CT_MAX CT_DELETE_MISSING
+
+/*
+ * Conflict resolvers that can be used to resolve various conflicts.
+ *
+ * See ConflictTypeResolverMap in conflict.c to find out which all
+ * resolvers are supported for each conflict type.
+ */
+typedef enum ConflictResolver
+{
+ /* Apply the remote change */
+ CR_APPLY_REMOTE = 1,

nit - these CT_MIN and CT_MAX are unnecessary. If the enums start at 0
(which they do in C by default anyhow) then you only need to know
'CONFLICT_NUM_TYPES'. Code previously using CT_MIN and CT_MAX will
become simpler too.

~~~

32.
+/*
+ * Conflict resolvers that can be used to resolve various conflicts.
+ *
+ * See ConflictTypeResolverMap in conflict.c to find out which all
+ * resolvers are supported for each conflict type.
+ */
+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 conflict resolver */
+#define CR_MIN CR_APPLY_REMOTE
+#define CR_MAX CR_ERROR
+

nit (a) - /can not/cannot/

nit (b) - as above, those CR_MIN and CR_MAX are not necessary. It is
simpler to start the enum values at 0 and then you only need to know
CONFLICT_NUM_RESOLVERS. Code previously using CT_MIN and CT_MAX will
become simpler too.

(I made these changes already -- see the attachment).

~~~

33.
+typedef struct ConflictTypeResolver
+{
+ const char *conflict_type;
+ const char *resolver;
+} ConflictTypeResolver;

These seem poor field names that are too easily confused with the
enums etc. Let's name them to make it obvious what they really are:

/conflict_type/conflict_type_name/
/resolver/conflict_resolver_name/

~~~

34.
+extern void SetSubConflictResolver(Oid subId, ConflictTypeResolver *
resolvers, int max_types);
+extern void RemoveSubscriptionConflictById(Oid confid);
+extern void RemoveSubscriptionConflictResolvers(Oid confid);
+extern List *GetAndValidateSubsConflictResolverList(List *stmtresolvers);
+extern void UpdateSubConflictResolvers(List *conflict_resolvers, Oid subid);
+extern ConflictType validate_conflict_type_and_resolver(const char
*conflict_type,
+ const char *conflict_resolver);
+extern void SetDefaultResolvers(ConflictTypeResolver * conflictResolvers);
+extern void ResetConflictResolver(Oid subid, char *conflict_type);
+extern void conf_detection_check_prerequisites(void);

nit (a) - remove some spaces that should not be there
nit (b) - make the parameter names more consistent

======
src/test/regress/sql/subscription.sql

35. General - quotes?

Why are all the resolver names single-quoted? (e.g. "update_missing =
'skip'"). AFAIK it is unnecessary.

~

36. General - why not use describe?

Would the text code be easier if you just validated the changes by
using the "describe" code (e.g. \dRs+ regress_testsub) instead of SQL
select of the pg_subscription_conflict table?

~

37.
nit - The test cases seemed mostly good to me. My nits are only for
small changes/typos or clarifications to the comments.

======
Please see the PS nits attachment. It already implements most of the
nits plus some other review comments were suggested above.

======
Kind Regards,
Peter Smith.
Fujitsu Australia