RE: Conflict detection and logging in logical replication

On Wednesday, July 31, 2024 1:36 PM shveta malik <shveta.malik@gmail.com> wrote:
> 
> On Wed, Jul 31, 2024 at 7:40 AM Zhijie Hou (Fujitsu) 
> <houzj.fnst@fujitsu.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