Re: Conflict detection and logging in logical replication

On Thu, Jul 25, 2024 at 12:04 PM Zhijie Hou (Fujitsu)
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> Here is the V6 patch set which addressed Shveta and Nisha's comments
> in [1][2][3][4].
>

Do we need an option detect_conflict for logging conflicts? The
possible reason to include such an option is to avoid any overhead
during apply due to conflict detection. IIUC, to detect some of the
conflicts like update_differ and delete_differ, we would need to fetch
commit_ts information which could be costly but we do that only when
GUC track_commit_timestamp is enabled which would anyway have overhead
on its own. Can we do performance testing to see how much additional
overhead we have due to fetching commit_ts information during conflict
detection?

The other time we need to enquire commit_ts is to log the conflict
detection information which is an ERROR path, so performance shouldn't
matter in this case.

In general, it would be good to enable conflict detection/logging by
default but if it has overhead then we can consider adding this new
option. Anyway, adding an option could be a separate patch (at least
for review), let the first patch be the core code of conflict
detection and logging.

minor cosmetic comments:
1.
+static void
+check_conflict_detection(void)
+{
+ if (!track_commit_timestamp)
+ ereport(WARNING,
+ errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
+ errmsg("conflict detection could be incomplete due to disabled
track_commit_timestamp"),
+ errdetail("Conflicts update_differ and delete_differ cannot be
detected, and the origin and commit timestamp for the local row will
not be logged."));
+}

The errdetail string is too long. It would be better to split it into
multiple rows.

2.
-
+static void check_conflict_detection(void);

Spurious line removal.

--
With Regards,
Amit Kapila.