Hi, Alexander!
On 2023-04-02 03:37:19 +0300, Alexander Korotkov wrote:
> On Sat, Apr 1, 2023 at 8:21 AM Andres Freund <andres@anarazel.de> wrote:
> > 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.
I looked into the latest patch v3.
In my view, it addresses all the issues discussed in [1]. Also, with
the pushing oldslot logic outside code becomes more transparent. I've
added some very minor modifications to the code and comments in patch
v4-0001. Also, I'm for committing Andres' isolation test. I've added
some minor revisions to make the test run routinely among the other
isolation tests. The test could also be made a part of the existing
eval-plan-qual.spec, but I have left it separate yet.
Also, I think that signatures of ExecUpdate() and ExecDelete()
functions, especially the last one are somewhat overloaded with
different status bool variables added by different authors on
different occasions. If they are combined into some kind of status
variable, it would be nice. But as this doesn't touch API, is not
related to the current update/delete optimization, it could be
modified anytime in the future as well.
The changes that indeed touch API are adding TupleTableSlot and
conversion of bool wait flag into now four-state options variable for
tuple_update(), tuple_delete(), heap_update(), heap_delete() and
heap_lock_tuple() and a couple of Exec*DeleteTriggers(). I think they
are justified.
One thing that is not clear to me is that we pass oldSlot into
simple_table_tuple_update() whereas as per the comment on this
function "concurrent updates of
the target tuple is not expected (for example, because we have a lock
on the relation associated with the tuple)". It seems not to break
anything but maybe this could be simplified.
Overall I think the patch is good enough.
Regards,
Pavel Borisov,
Supabase.
[1] https://www.postgresql.org/message-id/CAPpHfdtwKb5UVXKkDQZYW8nQCODy0fY_S7mV5Z%2Bcg7urL%3DzDEA%40mail.gmail.com
