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