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