Re: Conflict detection and logging in logical replication

On Thu, Aug 1, 2024 at 5:23 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Aug 1, 2024 at 2:26 PM Hayato Kuroda (Fujitsu)
> <kuroda(dot)hayato(at)fujitsu(dot)com> wrote:
> >
> > 04. general
> >
> > According to the documentation [1], there is another constraint "exclude", which
> > can cause another type of conflict. But this pattern cannot be logged in detail.
> >
>
> As per docs, "exclusion constraints can specify constraints that are
> more general than simple equality", so I don't think it satisfies the
> kind of conflicts we are trying to LOG and then in the future patch
> allows automatic resolution for the same. For example, when we have
> last_update_wins strategy, we will replace the rows with remote rows
> when the key column values match which shouldn't be true in general
> for exclusion constraints. Similarly, we don't want to consider other
> constraint violations like CHECK to consider as conflicts. We can
> always extend the basic functionality for more conflicts if required
> but let's go with reporting straight-forward stuff first.
>

It is better to document that exclusion constraints won't be
supported. We can even write a comment in the code and or commit
message that we can extend it in the future.

*
+ * Return true if the commit timestamp data was found, false otherwise.
+ */
+bool
+GetTupleCommitTs(TupleTableSlot *localslot, TransactionId *xmin,
+ RepOriginId *localorigin, TimestampTz *localts)

This API returns both xmin and commit timestamp, so wouldn't it be
better to name it as GetTupleTransactionInfo or something like that?

I have made several changes in the attached top-up patch. These
include changes in the comments, docs, function names, etc.

--
With Regards,
Amit Kapila.