RE: Conflict detection and logging in logical replication

On Wednesday, July 31, 2024 1:36 PM shveta malik <shveta(dot)malik(at)gmail(dot)com> wrote:
>
> On Wed, Jul 31, 2024 at 7:40 AM Zhijie Hou (Fujitsu)
> <houzj(dot)fnst(at)fujitsu(dot)com>
> wrote:
> >
> > >
> > > 2)
> > > apply_handle_delete_internal()
> > >
> > > --Do we need to check "(!edata->mtstate ||
> > > edata->mtstate->operation != CMD_UPDATE)" in the else part as
> > > well? Can there be a scenario where during update flow, it is
> > > trying to delete from a partition and comes here, but till then
> > > that row is deleted already and we end up raising 'delete_missing' additionally instead of 'update_missing'
> > > alone?
> >
> > I think this shouldn't happen because the row to be deleted should
> > have been locked before entering the apply_handle_delete_internal().
> > Actually, calling
> > apply_handle_delete_internal() for cross-partition update is a big
> > buggy because the row to be deleted has already been found in
> > apply_handle_tuple_routing(), so we could have avoid scanning the
> > tuple again. I have posted another patch to fix this issue in thread[1].
>
> Thanks for the details.
>
> >
> > Here is the V8 patch set. It includes the following changes:
> >
>
> Thanks for the patch. I verified that all the bugs reported so far are addressed.
> Few trivial comments:

Thanks for the comments!

>
> 1)
> 029_on_error.pl:
> --I did not understand the intent of this change. The existing insert
> would also have resulted in conflict (insert_exists) and we would have
> identified and skipped that. Why change to UPDATE?
>
> $node_publisher->safe_psql(
> 'postgres',
> qq[
> BEGIN;
> -INSERT INTO tbl VALUES (1, NULL);
> +UPDATE tbl SET i = 2;
> PREPARE TRANSACTION 'gtx';
> COMMIT PREPARED 'gtx';
> ]);
>

The intention of this change is to cover the code path of update_exists.
The original test only tested the code of insert_exists.

>
> 2)
> logical-replication.sgml
> --In doc, shall we have 'delete_differ' first and then
> 'delete_missing', similar to what we have for update (first
> 'update_differ' and then 'update_missing')
>
> 3)
> logical-replication.sgml: "For instance, the origin in the above log
> indicates that the existing row was modified by a local change."
>
> --This clarification about origin was required when we had 'origin 0'
> in 'DETAILS'. Now we have "locally":
> "Key (c)=(1) already exists in unique index "t_pkey", which was
> modified locally in transaction 740".
>
> And thus shall we rephrase the concerned line ?

Fixed in the V9 patch set.

Best Regards,
Hou zj