Re: Conflict Detection and Resolution

On Tue, Oct 1, 2024 at 9:54 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Tue, Oct 1, 2024 at 9:48 AM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
> >
>
> I have started reviewing patch002, it is WIP, but please find initial
> set of comments:
>

Please find second set of comments for patch002:

9)
can_create_full_tuple():
+ for (i = 0; i < newtup->ncols; i++)
+ {
+ if (newtup->colstatus[i] == LOGICALREP_COLUMN_UNCHANGED)
+ return false;
+ }

Why are we comparing it with 'LOGICALREP_COLUMN_UNCHANGED'? I assume
toast-values come as LOGICALREP_COLUMN_UNCHANGED. In any case, please
add comments.

10)
There are some alignment changes in
GetAndValidateSubsConflictResolverList() and the next few functions in
the same file which belongs to patch-001. Please move these changes to
patch001.

11)
+ * Find the resolver of the conflict type set under the given subscription.

Suggestion:
Find the resolver for the given conflict type and subscription

12)
+ #include "replication/logicalproto.h"

The code compiles even without the above new inclusion.

13)
ReportApplyConflict:
+ errmsg("conflict detected on relation \"%s.%s\": conflict=%s, Resolution=%s.",

We can have 'resolution' instead of 'Resolution', similar to
lower-case 'conflict'

14)
errdetail_apply_conflict:
CT_UPDATE_MISSING logs should be improved. As an example:

LOG: conflict detected on relation "public.t1":
conflict=update_missing, Resolution=apply_or_skip.
DETAIL: Could not find the row to be updated, Convert UPDATE to
INSERT and applying the remote changes.

Suggestion:
Could not find the row to be updated, thus converting the UPDATE to an
INSERT and applying the remote changes.

Similarly for other lines:
Could not find the row to be updated, and the UPDATE cannot be
converted to an INSERT, thus skipping the remote changes.

Could not find the row to be updated, and the UPDATE cannot be
converted to an INSERT, thus raising the error.

15)
errdetail_apply_conflict:
Can we pull out the sentence 'Could not find the row to be updated',
as it is common for all the cases of 'CT_UPDATE_MISSING' and then
append the rest of the string to it case-wise?

16)

+ConflictResolver
+GetConflictResolver(Relation localrel, ConflictType type, bool *apply_remote,
+ LogicalRepTupleData *newtup, Oid subid)

Can we please change the order of args to:
Oid subid, ConflictType type, Relation localrel, LogicalRepTupleData
*newtup, bool *apply_remote

Since we are getting resolvers for 'subid' and 'type', I have kept
those as initial args and OUT argument as last one.

17)
apply_handle_insert_internal:

+ /*
+ * If a conflict is detected and resolver is in favor of applying the
+ * remote changes, update the conflicting tuple by converting the remote
+ * INSERT to an UPDATE.
+ */
+ if (conflictslot)

The comment conveys 2 conditions while the code checks only one
condition, thus it is slightly misleading.

Perhaps change comment to:
/*
* If a conflict is detected, update the conflicting tuple by
converting the remote
* INSERT to an UPDATE. Note that conflictslot will have the
conflicting tuple only if
* the resolver is in favor of applying the changes, otherwise it will be NULL.
*/

<Rephrase if needed>

18)
apply_handle_update_internal():
* Report the conflict and configured resolver if the tuple was

Remove extra space after conflict.

thanks
Shveta