Re: Conflict Detection and Resolution

Hi Ajin/Nisha -- Here are my review comments for patch v15-0001 (code).

(AFAIK v16-0001 is the same as v15-0001, so this review is up to date)

Please also see the "nits" attachment to this post, which has many
more review comments of a more cosmetic nature.

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

1.
+static const char *const ConflictResolverNames[] = {
+ [CR_APPLY_REMOTE] = "apply_remote",
+ [CR_KEEP_LOCAL] = "keep_local",
+ [CR_APPLY_OR_SKIP] = "apply_or_skip",
+ [CR_APPLY_OR_ERROR] = "apply_or_error",
+ [CR_SKIP] = "skip",
+ [CR_ERROR] = "error"
+};
+

Add missing static assertions:

StaticAssertDecl(lengthof(ConflictTypeNames) == CONFLICT_NUM_TYPES,
"array length mismatch");

StaticAssertDecl(lengthof(ConflictResolverNames) == CONFLICT_NUM_TYPES,
"array length mismatch");

~~~

2.
+#define CONFLICT_TYPE_MAX_RESOLVERS 4

Now unused. Remove this.

~~~

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

If you are planning to keep this implementation of the Map, then that
-1 end of List marker is critical to the logic for the scanner part to
know when to stop looking for valid resolvers. So, I think you should
add another comment about that.

~~~

4.
+/*
+ * Report a warning about incomplete conflict detection and resolution if
+ * track_commit_timestamp is disabled.
+ */
+static void
+conf_detection_check_prerequisites(void)
+{
+ if (!track_commit_timestamp)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("conflict detection and resolution could be incomplete due to
disabled track_commit_timestamp"),
+ errdetail("Conflicts update_origin_differs and delete_origin_differs
cannot be "
+ "detected, and the origin and commit timestamp for the local row "
+ "will not be logged."));
+}
+

The "could be incomplete" wording seems vague to me. Why now just say
the WARNING reason in the errmsg?

SUGGESTION:
Conflicts types 'update_origin_differs' and 'delete_origin_differs'
cannot be detected unless "track_commit_timestamp" is enabled.

~~~

ParseAndGetSubConflictResolvers:

5.
+{
+ List *SeenTypes = NIL;
+ List *res = NIL;

All the list/string processing to determine if we have "seen" a
conflict type before looks inefficient to me. I had already
demonstrated in a previous review how this can be ditched in favour of
a simple boolean array. Again, I have implemented this in the NITPICKS
attachment to show how it can be implemented.

~~~

6.
+/*
+ * Get the list of conflict types and their corresponding default resolvers.
+ */
+List *
+GetDefaultConflictResolvers()

I'm wondering if it is worthwhile caching this default list. It will
never change, so if you cache it then you don't need to recalculate it
on subsequent calls.

======
src/bin/pg_dump/pg_dump.c

7.
Where are the test cases for the CONFLICT RESOLVER dump code?

~~~

getSubscriptions:

8.
+ ntups,
+ ntuples;

These var names are too similar. I can't tell them apart. Please
rename the new one (e.g. 'conf_ntuples').

~

9.
+ /* Initialize pointers in the list to NULL */
+ subinfo[i].conflict_resolver = (SimplePtrList)
+ {
+ 0
+ };
+

I didn't find anything else like this in PG source. IMO, it is better
to initialize both members explicitly to make it clear what this code
is actually doing. Or maybe memset() it.

SUGGESTION:
subinfo[i].conflict_resolver = (SimplePtrList)
{
.head = NULL, .tail = NULL
};

~~~

dumpSubscription:

+ /* Add conflict resolvers, if any */
+ if (fout->remoteVersion >= 180000)
+ {
+ bool first_resolver = true;

10a.
AFAICT this code is misplaced/broken. In the CREATE SUBSCRIPTION
syntax, the CONFLICT RESOLVER should come *before* any WITH clause, so
I think this is doomed to give a syntax error. The (missing) test
cases would have found this.

10b.
And, I expect when you fix that clause ordering then there will be
other fixes needed to correctly handle the closing parenthesis ')'.

~~~

11.
+/*
+ * destroyConflictResolverList - frees up the SimplePtrList containing
+ * cells pointing to struct ConflictTypeResolver nodes.
+ */
+static void
+destroyConcflictResolverList(SimplePtrList *conflictlist)
+{
+ SimplePtrListCell *cell = NULL;
+
+ /* Free the list items */
+ for (cell = conflictlist->head; cell; cell = cell->next)
+ pfree((ConflictTypeResolver *) cell->ptr);
+
+ /* Destroy the pointer list */
+ simple_ptr_list_destroy(conflictlist);
+}

Hmm. Is this even needed? AFAICT this entire function seems to be
doing the same as just calling simple_ptr_list_destroy(conflictlist)
directly.

======
src/bin/pg_dump/pg_dump.h

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

This should be renamed (e.g. _ConflictTypeResolver or similar) with
underscore for consistency with other dump typedefs

~~~

13.
char *subfailover;
+ SimplePtrList conflict_resolver;
} SubscriptionInfo;

Rename this field to 'conflict_resolvers' (plural). Also, a comment
might help to say what the list elements are.

======
MISSING src/bin/psql/tab-complete.c

14.
Where is the tab-completion implementation for all the new syntax of
the v15 patch?

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