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

.Hi!

On Sat, Apr 1, 2023 at 8:21 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> On 2023-03-31 16:57:41 +0300, Alexander Korotkov wrote:
> > On Wed, Mar 29, 2023 at 8:34 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> > > On Mon, Mar 27, 2023 at 1:49 PM Alexander Korotkov <aekorotkov(at)gmail(dot)com> wrote:
> > > > On Fri, Mar 24, 2023 at 3:39 AM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > > On 2023-03-23 23:24:19 +0300, Alexander Korotkov wrote:
> > > > > > On Thu, Mar 23, 2023 at 8:06 PM Andres Freund <andres(at)anarazel(dot)de> wrote:
> > > > > > > I seriously doubt that solving this at the tuple locking level is the right
> > > > > > > thing. If we want to avoid refetching tuples, why don't we add a parameter to
> > > > > > > delete/update to generally put the old tuple version into a slot, not just as
> > > > > > > an optimization for a subsequent lock_tuple()? Then we could remove all
> > > > > > > refetching tuples for triggers. It'd also provide the basis for adding support
> > > > > > > for referencing the OLD version in RETURNING, which'd be quite powerful.
> > > >
> > > > After some thoughts, I think I like idea of fetching old tuple version
> > > > in update/delete. Everything that evades extra tuple fetching and do
> > > > more of related work in a single table AM call, makes table AM API
> > > > more flexible.
> > > >
> > > > I'm working on patch implementing this. I'm going to post it later today.
> > >
> > > Here is the patchset. I'm continue to work on comments and refactoring.
> > >
> > > My quick question is why do we need ri_TrigOldSlot for triggers?
> > > Can't we just pass the old tuple for after row trigger in
> > > ri_oldTupleSlot?
> > >
> > > Also, I wonder if we really need a LazyTupleSlot. It allows to evade
> > > extra tuple slot allocation. But as I get in the end the tuple slot
> > > allocation is just a single palloc. I bet the effect would be
> > > invisible in the benchmarks.
> >
> > Sorry, previous patches don't even compile. The fixed version is attached.
> > I'm going to post significantly revised patchset soon.
>
> Given that the in-tree state has been broken for a week, I think it probably
> is time to revert the commits that already went in.

The revised patch is attached. The most notable change is getting rid
of LazyTupleTableSlot. Also get rid of complex computations to detect
how to initialize LazyTupleTableSlot. Instead just pass the oldSlot
as an argument of ExecUpdate() and ExecDelete(). The price for this
is just preallocation of ri_oldTupleSlot before calling ExecDelete().
The slot allocation is quite cheap. After all wrappers it's
table_slot_callbacks(), which is very cheap, single palloc() and few
fields initialization. It doesn't seem reasonable to introduce an
infrastructure to evade this.

I think patch resolves all the major issues you've highlighted. Even
if there are some minor things missed, I'd prefer to push this rather
than reverting the whole work.

------
Regards,
Alexander Korotkov