Re: POC: Lock updated tuples in tuple_update() and tuple_delete()

Hi, Pavel!

On Wed, Jan 4, 2023 at 3:43 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> On Wed, 4 Jan 2023 at 12:52, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > On Wed, 4 Jan 2023 at 12:41, vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Fri, 1 Jul 2022 at 16:49, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > > >
> > > > Hackers,
> > > >
> > > > When working in the read committed transaction isolation mode
> > > > (default), we have the following sequence of actions when
> > > > tuple_update() or tuple_delete() find concurrently updated tuple.
> > > >
> > > > 1. tuple_update()/tuple_delete() returns TM_Updated
> > > > 2. tuple_lock()
> > > > 3. Re-evaluate plan qual (recheck if we still need to update/delete
> > > > and calculate the new tuple for update)
> > > > 4. tuple_update()/tuple_delete() (this time should be successful,
> > > > since we've previously locked the tuple).
> > > >
> > > > I wonder if we should merge steps 1 and 2. We could save some efforts
> > > > already done during tuple_update()/tuple_delete() for locking the
> > > > tuple. In heap table access method, we've to start tuple_lock() with
> > > > the first tuple in the chain, but tuple_update()/tuple_delete()
> > > > already visited it. For undo-based table access methods,
> > > > tuple_update()/tuple_delete() should start from the last version, why
> > > > don't place the tuple lock immediately once a concurrent update is
> > > > detected. I think this patch should have some performance benefits on
> > > > high concurrency.
> > > >
> > > > Also, the patch simplifies code in nodeModifyTable.c getting rid of
> > > > the nested case. I also get rid of extra
> > > > table_tuple_fetch_row_version() in ExecUpdate. Why re-fetch the old
> > > > tuple, when it should be exactly the same tuple we've just locked.
> > > >
> > > > I'm going to check the performance impact. Thoughts and feedback are welcome.
> > >
> > > The patch does not apply on top of HEAD as in [1], please post a rebased patch:
> > > === Applying patches on top of PostgreSQL commit ID
> > > eb5ad4ff05fd382ac98cab60b82f7fd6ce4cfeb8 ===
> > > === applying patch
> > > ./0001-Lock-updated-tuples-in-tuple_update-and-tuple_del-v1.patch
> > > patching file src/backend/executor/nodeModifyTable.c
> > > ...
> > > Hunk #3 FAILED at 1376.
> > > ...
> > > 1 out of 15 hunks FAILED -- saving rejects to file
> > > src/backend/executor/nodeModifyTable.c.rej
> > >
> > > [1] - http://cfbot.cputube.org/patch_41_4099.log
> >
> > The rebased patch is attached. It's just a change in formatting, no
> > changes in code though.
>
> One more update of a patchset to avoid compiler warnings.

Thank you for your help.  I'm going to provide the revised version of
patch with comments and commit message in the next couple of days.

------
Regards,
Alexander Korotkov