FOR PORTION OF code review
Hi,
I was pinged about whether I think FOR PORTION OF is in a good state. I
hadn't read the code to any meaningful degree, so I just started
reading. Manually looking through FOR PORTION OF code I noticed a few things:
- /*
* 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.
-
/*
* Get the range's type cache entry. This is worth caching for the whole
* UPDATE/DELETE as range functions do.
*/
typcache = fpoState->fp_leftoverstypcache;
if (typcache == NULL)
{
typcache = lookup_type_cache(forPortionOf->rangeType, 0);
fpoState->fp_leftoverstypcache = typcache;
}
Hm. This immediately makes me worried about that typecache entry going away
during the execution. What provides protection against that?
Also, why is this done in ExecForPortionOfLeftovers(), rather than
ExecInitForPortionOf()?
And, uh, what is that caching for? I don't see *anything* using it except
the above lines? And a quick git log -G doesn't show other uses? I don't
think fp_rangeType is used either. There might be more, I didn't look
further.
- fmgr_info(forPortionOf->withoutPortionProc, &flinfo);
rsi.type = T_ReturnSetInfo;
rsi.econtext = mtstate->ps.ps_ExprContext;
rsi.expectedDesc = NULL;
rsi.allowedModes = (int) (SFRM_ValuePerCall);
rsi.returnMode = SFRM_ValuePerCall;
/* isDone is filled below */
rsi.setResult = NULL;
rsi.setDesc = NULL;
InitFunctionCallInfoData(*fcinfo, &flinfo, 2, InvalidOid, NULL, (Node *) &rsi);
fcinfo->args[0].value = oldRange;
fcinfo->args[0].isnull = false;
fcinfo->args[1].value = fpoState->fp_targetRange;
fcinfo->args[1].isnull = false;
Why is this done in ExecForPortionOfLeftovers(), rather than
ExecInitForPortionOf()?
-
/* Call the function one time */
pgstat_init_function_usage(fcinfo, &fcusage);
fcinfo->isnull = false;
rsi.isDone = ExprSingleResult;
leftover = FunctionCallInvoke(fcinfo);
pgstat_end_function_usage(&fcusage,
rsi.isDone != ExprMultipleResult);
if (rsi.returnMode != SFRM_ValuePerCall)
elog(ERROR, "without_portion function violated function call protocol");
Why are we insisting on a specific SRF protocol? I guess the set of
functions that can be referenced here is small, but the code still should
comment on why this is a sane assumption.
- A lot of this code lacks high-level comments. It's pointless to have a
comment that explains obvious code like
/* Are we done? */
if (rsi.isDone == ExprEndResult)
break;
But there are a lot of higher-level things - like how all of this actually
is supposed to work - that are not commented upon.
- if (!didInit)
{
/*
* Make a copy of the pre-UPDATE row. Then we'll overwrite the
* range column below. Only partitioned targets need conversion to
* the root table's format, because they reinsert through the root
* relation for tuple routing.
*/
if (map != NULL)
{
leftoverSlot = execute_attr_map_slot(map->attrMap,
oldtupleSlot,
leftoverSlot);
}
else
{
oldtuple = ExecFetchSlotHeapTuple(oldtupleSlot, false, &shouldFree);
ExecForceStoreHeapTuple(oldtuple, leftoverSlot, false);
}
...
}
else
{
/*
* Re-copy the original row into leftoverSlot because ExecInsert
* might pass leftoverSlot to BEFORE ROW INSERT triggers, which
* can modify the slot contents.
*/
if (map != NULL)
execute_attr_map_slot(map->attrMap, oldtupleSlot, leftoverSlot);
else
ExecForceStoreHeapTuple(oldtuple, leftoverSlot, false);
Why is so much of this code duplicated between the !didInit and else
branches? Isn't the only thing that actually needs to happen in the
!didInit branch the ExecFetchSlotHeapTuple?
- /*
* Save some mtstate things so we can restore them below. XXX:
* Should we create our own ModifyTableState instead?
*/
oldOperation = mtstate->operation;
mtstate->operation = CMD_INSERT;
oldTcs = mtstate->mt_transition_capture;
It seems not great to just randomly change the mtstate for a while and then
later change it back. Even if it doesn't cause problems today, I would bet
it will lead to bugs in the future. There's code that makes part of the
initialization depend on the operation, for example.
- /*
* The standard says that each temporal leftover should execute its
* own INSERT statement, firing all statement and row triggers, but
* skipping insert permission checks. Therefore we give each insert
* its own transition table. If we just push & pop a new trigger level
* for each insert, we get exactly what we need.
*
* We have to make sure that the inserts don't add to the ROW_COUNT
* diagnostic or the command tag, so we pass false for canSetTag.
*/
AfterTriggerBeginQuery();
ExecSetupTransitionCaptureState(mtstate, estate);
fireBSTriggers(mtstate);
ExecInsert(context, resultRelInfo, leftoverSlot, false, NULL, NULL);
fireASTriggers(mtstate);
AfterTriggerEndQuery(estate);
Not your fault, but this seems kinda terrible. This basically seems like
it's making statement level triggers not really work as they're intended
anymore :(.
-
/* Eval the FOR PORTION OF target */
if (!(eflags & EXEC_FLAG_EXPLAIN_ONLY))
{
bool isNull;
ExprContext *econtext;
ExprState *exprState;
if (mtstate->ps.ps_ExprContext == NULL)
ExecAssignExprContext(estate, &mtstate->ps);
econtext = mtstate->ps.ps_ExprContext;
exprState = ExecPrepareExpr((Expr *) forPortionOf->targetRange, estate);
targetRange = ExecEvalExpr(exprState, econtext, &isNull);
It doesn't seem right to me that EXEC_FLAG_EXPLAIN_ONLY short-circuits not
just the ExecEvalExpr() but also the preparation of the expression.
But also, why is this invoking ExecPrepareExpr()? That's "planning" the
expression from scratch. Note the function's comment:
* ExecPrepareExpr --- initialize for expression execution outside a normal
* Plan tree context.
*
* This differs from ExecInitExpr in that we don't assume the caller is
* already running in the EState's per-query context. Also, we run the
* passed expression tree through expression_planner() to prepare it for
* execution. (In ordinary Plan trees the regular planning process will have
* made the appropriate transformations on expressions, but for standalone
* expressions this won't have happened.)
If you get here without the expression having already been prepared for
execution, something has gone wrong imo. I now see there are a few other
pieces of such broken code around, but I don't think that's OK. For one
this breaks things like gathering the set of dependencies that should
trigger statements to be replanned if the dependency is just in the
expression that you're not handling during planning.
I think this also means that the expression won't be able to reference
parameters from e.g. an outer query?
-
/* Create state for FOR PORTION OF operation */
fpoState = makeNode(ForPortionOfState);
fpoState->fp_rangeType = forPortionOf->rangeType;
fpoState->fp_rangeAttno = forPortionOf->rangeVar->varattno;
fpoState->fp_targetRange = targetRange;
Why are we re-storing information that already precisely exists before?
-
/* Initialize slot for the existing tuple */
fpoState->fp_Existing =
table_slot_create(rootRelInfo->ri_RelationDesc,
&mtstate->ps.state->es_tupleTable);
Do we really need to have fp_[a-z] in the same new struct as fp_[A-Z]?
-
/* Create the tuple slot for INSERTing the temporal leftovers */
fpoState->fp_Leftover =
ExecInitExtraTupleSlot(mtstate->ps.state, tupDesc, &TTSOpsVirtual);
It doesn't matter terribly, but why is this using a virtual slot? That just
guarantees that we will have to copy during insertion, even when
partitioning is not present?
Ran out of energy & time at this point. There's plenty more to look at.
I'll also trigger some AI review.
Greetings,
Andres
Hi,
On 2026-09-10 10:07:02 -0400, Andres Freund wrote:
Ran out of energy & time at this point. There's plenty more to look at.
Haven't yet found time to do that, except one thing I was wondering about when
re-reading my email:
- /*
* The standard says that each temporal leftover should execute its
* own INSERT statement, firing all statement and row triggers, but
* skipping insert permission checks. Therefore we give each insert
* its own transition table. If we just push & pop a new trigger level
* for each insert, we get exactly what we need.
*
* We have to make sure that the inserts don't add to the ROW_COUNT
* diagnostic or the command tag, so we pass false for canSetTag.
*/
AfterTriggerBeginQuery();
ExecSetupTransitionCaptureState(mtstate, estate);
fireBSTriggers(mtstate);
ExecInsert(context, resultRelInfo, leftoverSlot, false, NULL, NULL);
fireASTriggers(mtstate);
AfterTriggerEndQuery(estate);Not your fault, but this seems kinda terrible. This basically seems like
it's making statement level triggers not really work as they're intended
anymore :(.
ExecSetupTransitionCaptureState() is called once per leftover row, with an
update/delete affecting many rows, that can be a lot of times. Each time it
allocates memory in query context and then overwrites the existing
mtstate->mt_transition_capture (which is later retored, as I earlier
complained about). That's obviously a query-level memory leak?
I'll also trigger some AI review.
I ran this with the following fixes added:
- Support concrete-typed range opclasses in FOR PORTION OF
- Fire leftover INSERT triggers on the table the leftovers go into
- Reject FOR PORTION OF on views with unqualified INSTEAD rules
- Fix memory leak in FOR PORTION OF domain lookup
This found some things:
- Crash due to wholerow references:
DROP TABLE IF EXISTS fpo_star CASCADE;
CREATE TABLE fpo_star (r int4range);
DELETE FROM fpo_star FOR PORTION OF r ((ROW(fpo_star.*)).f1);
table.* doesn't go through transformColumnRef() when called via
transformExpressionList(), and thus isn't prohibited. I suspect this may be
a wider issue and should be fixed by improving the general infrastructure,
even if it's not a problem today for other places, it seems likely to become
one in the future.
- PL/pgSQL variables and named parameters cannot be bounds
DROP TABLE IF EXISTS plv CASCADE;
DROP FUNCTION IF EXISTS plv_upd(int, int);
CREATE TABLE plv (r int4range, name text);
INSERT INTO plv VALUES ('[1,100)', 'x');
CREATE FUNCTION plv_upd(lo int, hi int) RETURNS void LANGUAGE plpgsql AS $$
BEGIN UPDATE plv FOR PORTION OF r FROM lo TO hi SET name = 'upd'; END $$;
SELECT plv_upd(10, 20);
ERROR: 0A000: cannot use column reference in FOR PORTION OF expression
There are no column references here though...
This, I guess, again might be a more general problem, I haven't looked into
it.
- `pg_get_functiondef()` output does not replay in edge case
Related to the prior one:
DROP FUNCTION IF EXISTS fpo_named_delete(int, int);
DROP TABLE IF EXISTS fpo_named CASCADE;
CREATE TABLE fpo_named (r int4range);
CREATE FUNCTION fpo_named_delete(lo integer, hi integer) RETURNS void LANGUAGE SQL
BEGIN ATOMIC
DELETE FROM fpo_named FOR PORTION OF r FROM $1 TO $2;
END;
SELECT pg_get_functiondef('fpo_named_delete(int,int)'::regprocedure) \gexec
ERROR: 0A000: cannot use column reference in FOR PORTION OF expression
LINE 5: DELETE FROM fpo_named FOR PORTION OF r FROM fpo_named_delet...
- Query level memory leaks
There's at least two:
- the ExecSetupTransitionCaptureState() leak described above
The fix here is to avoid allocating the capture state over and over or at
least to free it.
- With a BEFORE INSERT row trigger returning NEW unmodified,
ExecBRInsertTriggers() copies the tuple out of a virtual slot with
ExecFetchSlotHeapTuple( &should_free) and frees it only when the trigger returns
NULL or a different tuple. plpgsql returns tg_trigtuple itself, so the
copy leaks.
I think this might be a problem in some corner cases before, but is more
easily reached with FPO.
The fix here is to free the tuple in ExecBRInsertTriggers() if it's
allocated.
- DO ALSO rules aren't rejected
They can cause very similar issues to DO INSTEAD.
- DO ALSO doesn't deparse correctly:
DROP TABLE IF EXISTS ivl_t CASCADE;
DROP TYPE IF EXISTS intervalrange CASCADE;
CREATE TYPE intervalrange AS RANGE (subtype = interval);
CREATE TABLE ivl_t (r intervalrange);
CREATE RULE ivl_r AS ON INSERT TO ivl_t DO ALSO
DELETE FROM ivl_t FOR PORTION OF r FROM (INTERVAL '1' HOUR) TO INTERVAL '2' HOUR;
SELECT 'DROP RULE ivl_r ON ivl_t' UNION ALL SELECT pg_get_ruledef(oid) FROM pg_rewrite WHERE rulename = 'ivl_r' \gexec
ERROR: syntax error at or near "'02:00:00'"
- AFTER triggers for leftover tuples fire while outer statement is still
running
Normally AFTER triggers should be able to modify rows. But with the current
nesting of when leftover rows fire triggers that is problematic:
DROP TABLE IF EXISTS armod CASCADE;
DROP FUNCTION IF EXISTS armod_trg() CASCADE;
CREATE TABLE armod (id int, valid_at daterange, name text);
INSERT INTO armod VALUES (1, '[2020-01-01,2021-01-01)', 'a'), (2, '[2020-01-01,2021-01-01)', 'b'), (3, '[2020-01-01,2021-01-01)', 'c');
CREATE FUNCTION armod_trg() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
IF pg_trigger_depth() = 1 THEN UPDATE armod SET name = name || '!' WHERE id <> NEW.id; END IF;
RETURN NULL;
END $$;
CREATE TRIGGER armod_ai AFTER INSERT ON armod FOR EACH ROW EXECUTE FUNCTION armod_trg();
UPDATE armod FOR PORTION OF valid_at FROM '2020-03-01' TO '2020-06-01' SET name = name || '*';
ERROR: tuple to be updated was already modified by an operation triggered by the current command
HINT: Consider using an AFTER trigger instead of a BEFORE trigger to propagate changes to other rows.
Note this is suggesting the use of an AFTER trigger despite already using
one.
Several of these seem more like general infrastructure faults than this
patch's...
Greetings,
Andres Freund
[RMT hat]
On Fri, Sep 11, 2026 at 08:50:07AM -0400, Andres Freund wrote:
[detailed review]
Thanks for doing this, Andres. Peter/Paul, can you please respond with
your thoughts?
--
nathan
On Fri, Sep 11, 2026 at 6:10 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
[RMT hat]
On Fri, Sep 11, 2026 at 08:50:07AM -0400, Andres Freund wrote:
[detailed review]
Thanks for doing this, Andres. Peter/Paul, can you please respond with
your thoughts?
Hi Andres, thank you for taking the time to give such a thorough
review. I've gone through everything in your first email and should
have some patches and item-by-item feedback this morning. I've also
read through your second list; I'll work on that after addressing the
first items.
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
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
Attachments:
for-portion-of-lost.spectext/plain; charset=us-asciiDownload
Hi,
On 2026-09-11 12:49:54 -0400, Andres Freund wrote:
I wonder if there may be additional issues with DELETE ... FOR PORTION OF, due
to not having a ctid chain to follow.
Yep. There's lost updates even with full-key locks, once DELETE FPO enters the
picture.
See the AI generated spec file (although I really needed to force both Opus 5
and Fable 5.1 to get to it, they both swore up and down that this isn't a real
issue at first).
The problem is that with UPDATE FPO different backends serialize on the
surviving row, allowing only one backend to acquire the FOR UPDATE lock on
that row, with the other transaction waiting for the second transaction to
either abort, or to lock the subsequent row.
But with DELETE FPO, there's no such serialization, once the first transaction
commits all concurrent FOR UPDATEs complete, *without* needing a row lock.
So I think either FPO needs a fair bit more work (e.g. using the speculative
insert infrastructure from ON CONFLICT and/or perhaps some careful scanning
with a dirty snapshot), or the feature ought to just refuse to be used with
READ COMMITTED. I'm a bit sceptical that the latter is acceptable. And the
former seems very clearly out of scope for 19.
Greetings,
Andres Freund
Attachments:
for-portion-of-lost-delete2.spectext/plain; charset=us-asciiDownload
On Fri, Sep 11, 2026 at 10:30 AM Andres Freund <andres@anarazel.de> wrote:
On 2026-09-11 12:49:54 -0400, Andres Freund wrote:
I wonder if there may be additional issues with DELETE ... FOR PORTION OF, due
to not having a ctid chain to follow.Yep. There's lost updates even with full-key locks, once DELETE FPO enters the
picture.See the AI generated spec file (although I really needed to force both Opus 5
and Fable 5.1 to get to it, they both swore up and down that this isn't a real
issue at first).The problem is that with UPDATE FPO different backends serialize on the
surviving row, allowing only one backend to acquire the FOR UPDATE lock on
that row, with the other transaction waiting for the second transaction to
either abort, or to lock the subsequent row.But with DELETE FPO, there's no such serialization, once the first transaction
commits all concurrent FOR UPDATEs complete, *without* needing a row lock.So I think either FPO needs a fair bit more work (e.g. using the speculative
insert infrastructure from ON CONFLICT and/or perhaps some careful scanning
with a dirty snapshot), or the feature ought to just refuse to be used with
READ COMMITTED. I'm a bit sceptical that the latter is acceptable. And the
former seems very clearly out of scope for 19.
Thanks for diving into the concurrency issues. I think it is the most
serious issue here. I was a little surprised that it was originally
considered acceptable, actually, and I would be happy to change it.
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry. At first I thought
we never did that under READ COMMITTED, but actually it is possible
with MERGE or cross-partition UPDATE. So it is not unprecedented, and
FOR PORTION OF has a similar "compound" effect. I'm working on a patch
for that right now. So far I think it correctly catches all the cases
we've seen. Do you have any objections to that approach?
Yours,
--
Paul ~{:-)
pj@illuminatedcomputing.com
Hi,
On 2026-09-11 10:56:40 -0700, Paul A Jungwirth wrote:
On Fri, Sep 11, 2026 at 10:30 AM Andres Freund <andres@anarazel.de> wrote:
On 2026-09-11 12:49:54 -0400, Andres Freund wrote:
I wonder if there may be additional issues with DELETE ... FOR PORTION OF, due
to not having a ctid chain to follow.Yep. There's lost updates even with full-key locks, once DELETE FPO enters the
picture.See the AI generated spec file (although I really needed to force both Opus 5
and Fable 5.1 to get to it, they both swore up and down that this isn't a real
issue at first).The problem is that with UPDATE FPO different backends serialize on the
surviving row, allowing only one backend to acquire the FOR UPDATE lock on
that row, with the other transaction waiting for the second transaction to
either abort, or to lock the subsequent row.But with DELETE FPO, there's no such serialization, once the first transaction
commits all concurrent FOR UPDATEs complete, *without* needing a row lock.So I think either FPO needs a fair bit more work (e.g. using the speculative
insert infrastructure from ON CONFLICT and/or perhaps some careful scanning
with a dirty snapshot), or the feature ought to just refuse to be used with
READ COMMITTED. I'm a bit sceptical that the latter is acceptable. And the
former seems very clearly out of scope for 19.Thanks for diving into the concurrency issues. I think it is the most
serious issue here. I was a little surprised that it was originally
considered acceptable, actually, and I would be happy to change it.
FWIW, personally I don't think it is acceptable. I'm quite baffled that it was
committed with such a glaring hole.
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.
Personally I don't find that it's a convincing feature with that
limitation.
For ON CONFLICT Peter (and, to a lesser degree I) spent a *lot* of time
getting the concurrency behaviour somewhat right. I think this feature really
ought to have working concurrency behaviour, not a cheap copout. For better
or worse, READ COMMITTED is extremely widely used, with one reason for that
being not needing to retry, and making FPO not really work that way doesn't
seem convincing to me.
Do you have any objections to that approach?
I think it this should be properly fixed instead.
That'll likely require a protocol of scanning for relevant rows with something
like ExecCheckIndexConstraints() (modified to be able to search for multiple
conflicts), making sure those are locked, with a visibility check for higher
isolation levels. I suspect it'll be hard to get this right without requiring
a WITHOUT OVERLAPS index, but I'm not sure.
Greetings,
Andres Freund
On 11.09.26 20:14, Andres Freund wrote:
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.Personally I don't find that it's a convincing feature with that
limitation.
I think we can explore different solutions, but probably not in the week
before the (hopefully) last beta?
On Sat, Sep 12, 2026 at 9:18 AM Peter Eisentraut <peter@eisentraut.org> wrote:
On 11.09.26 20:14, Andres Freund wrote:
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.Personally I don't find that it's a convincing feature with that
limitation.I think we can explore different solutions, but probably not in the week
before the (hopefully) last beta?
[RMT hat]
I don't quite understand the conclusion here. What are you proposing to do?
- Melanie
Hi,
On 2026-09-12 15:18:28 +0200, Peter Eisentraut wrote:
On 11.09.26 20:14, Andres Freund wrote:
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.Personally I don't find that it's a convincing feature with that
limitation.I think we can explore different solutions, but probably not in the week
before the (hopefully) last beta?
I'm not sure what course you're proposing here? To me it doesn't seem this is
ready for prime-time. I'd not be opposed to incrementally working on it in 20,
although the degree of issues left also makes it seem reasonable to start from
a clean slate.
Greetings,
Andres Freund
On 14.09.26 15:52, Melanie Plageman wrote:
On Sat, Sep 12, 2026 at 9:18 AM Peter Eisentraut <peter@eisentraut.org> wrote:
On 11.09.26 20:14, Andres Freund wrote:
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.Personally I don't find that it's a convincing feature with that
limitation.I think we can explore different solutions, but probably not in the week
before the (hopefully) last beta?[RMT hat]
I don't quite understand the conclusion here. What are you proposing to do?
I was trying to nudge toward reverting without saying it. ;-)
On 14.09.26 16:19, Peter Eisentraut wrote:
On 14.09.26 15:52, Melanie Plageman wrote:
On Sat, Sep 12, 2026 at 9:18 AM Peter Eisentraut
<peter@eisentraut.org> wrote:On 11.09.26 20:14, Andres Freund wrote:
Not having a successful workaround makes it even more of a problem.
Rather than forbidding READ COMMITTED at all, I think we should raise
a serialization failure. Then the user can retry.Personally I don't find that it's a convincing feature with that
limitation.I think we can explore different solutions, but probably not in the week
before the (hopefully) last beta?[RMT hat]
I don't quite understand the conclusion here. What are you proposing
to do?I was trying to nudge toward reverting without saying it. ;-)
I have reverted the feature.