RE: Conflict detection and logging in logical replication

On Friday, August 2, 2024 7:03 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> 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.

Added.

>
> *
> + * 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?

The suggested name looks better. Addressed in the patch.

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

Thanks! I have reviewed and merged them in the patch.

Here is the V11 patch set which addressed above and Kuroda-san[1] comments.

Note that we may remove the 0002 patch in the next version as we didn't see
performance effect from the detection logic.

[1] https://www.postgresql.org/message-id/TYAPR01MB569224262F44875973FAF344F5B22%40TYAPR01MB5692.jpnprd01.prod.outlook.com

Best Regards,
Hou zj