Re: FOR PORTION OF code review

Hi,

On 2026-09-10 10:07:02 -0400, Andres Freund wrote:
> - /*
> * Get the old pre-UPDATE/DELETE tuple. We will use its range to compute
> * untouched parts of history, and if necessary we will insert copies with
> * truncated start/end times.
> *
> * We have already locked the tuple in ExecUpdate/ExecDelete, and it has
> * passed EvalPlanQual. This ensures that concurrent updates in READ
> * COMMITTED can't insert conflicting temporal leftovers.
> *
> * It does *not* protect against concurrent update/deletes overlooking
> * each others' leftovers though. See our isolation tests for details
> * about that and a viable workaround.
> */
>
> Incorrect concurrency behavior seems like ... a problem? And I don't think
> it's good to explain the details of the problem and workarounds in the spec
> file.
>
> Spec file:
> # UPDATE/DELETE FOR PORTION OF test
> #
> # Test inserting temporal leftovers from a FOR PORTION OF update/delete.
> #
> # In READ COMMITTED mode, concurrent updates/deletes to the same records cause
> # weird results. Portions of history that should have been updated/deleted don't
> # get changed. That's because the leftovers from one operation are added too
> # late to be seen by the other. EvalPlanQual will reload the changed-in-common
> # row, but it won't re-scan to find new leftovers.
> #
> # MariaDB similarly gives undesirable results in READ COMMITTED mode (although
> # not the same results). DB2 doesn't have READ COMMITTED, but it gives correct
> # results at all levels, in particular READ STABILITY (which seems closest).
> #
> # A workaround is to lock the part of history you want before changing it (using
> # SELECT FOR UPDATE). That way the search for rows is late enough to see
> # leftovers from the other session(s). This shouldn't impose any new deadlock
> # risks, since the locks are the same as before. Adding a third/fourth/etc.
> # connection also doesn't change the semantics. The READ COMMITTED tests here
> # demonstrate the problem and also show that solving it with manual locks is
> # viable and not vitiated by any bugs. Incidentally, this approach also works in
> # MariaDB.
>
>
> And docs:
>
> + <para>
> + In <literal>READ COMMITTED</literal> mode, temporal updates and deletes can
> + yield unexpected results when they concurrently touch the same row. It is
> + possible to lose all or part of the second update or delete. The scenario
> + is illustrated in <xref linkend="temporal-isolation-figure"/>. Session 2
> + searches for rows to change, and it finds one that Session 1 has already
> + modified. It waits for Session 1 to commit. Then it re-checks whether the
> + row still matches its search criteria (including the start/end times
> + targeted by <literal>FOR PORTION OF</literal>). Session 1 may have changed
> + those times so that they no longer qualify.
> + </para>
>
> I feel like I must be missing something here. I don't think lost updates are
> acceptable whatsoever. And this note in the docs doesn't meaningfully
> make that OK.
>
>
> I also really doubt that this workaround actually works correctly. Afaict
> the FOR UPDATEs will often not actually be able to see the rows that would
> need to be locked. For normal non-FPO locking, we can follow ctid chains to
> rows that are not visible to the current session - but that doesn't work
> here, because the leftover rows aren't chained off the original row.

I haven't manually dug further into this, but a quick quest for AI to
reproduce failures for this scheme shows that the workaround doesn't seem to
work as-is:

The docs say this:
+ <para>
+ To solve these problems, precede every temporal update/delete with a
+ <literal>SELECT FOR UPDATE</literal> matching the same criteria (including
+ the targeted portion of application time). That way the actual
+ update/delete doesn't begin until the lock is held, and all concurrent
+ leftovers will be visible. In higher transaction isolation levels, this
+ lock is not required.
+ </para>

But that doesn't work, because the FOR UPDATE, following the "matching the
same criteria (including the targeted portion of application time)" advice,
will often end up *not* locking the targeted row, because the to-be-locked-row
can end up being filtered out, due to the temporal filter not matching
anymore. As EvalPlanQual happens before the row is locked, you can end up not
locking any rows - which then obviously leads to problems.

Attached is a 2 session, 3 transaction isolation schedule showing the issue.

I suspect one can, kind of, work around at least the most obvious problem by
changing the advice to *not* include the application time filter. Of course
that will trigger a lot more deadlocks and might be unacceptably expensive,
but...

I wonder if there may be additional issues with DELETE ... FOR PORTION OF, due
to not having a ctid chain to follow.

Greetings,

Andres Freund