FOR PORTION OF does not recompute GENERATED STORED columns that depend on the range column

First seen: 2026-04-07 08:42:42+00:00 · Messages: 15 · Participants: 4

Latest Update

2026-05-07 · opus 4.7

New in this round: v8 crash, v9 fix, and a minor comment nit

Peter Eisentraut surfaces a regression

Peter reported that the patch (v8) crashes the server under the injection_points/isolation test. This is the first time Peter appears in the thread, and his report reveals that the v7→v8 iteration introduced a NULL-deref that none of the committer-side review caught.

Root cause (diagnosed by Jian)

Jian's diff pinpoints the bug in ExecGetUpdatedCols (src/backend/executor/execUtils.c):

RTEPermissionInfo *perminfo = GetResultRTEPermissionInfo(relinfo, estate);
Bitmapset *updatedCols = perminfo->updatedCols;   // ← deref before NULL check

if (perminfo == NULL)
    ...

The patch dereferences perminfo on the line immediately before the existing if (perminfo == NULL) guard. GetResultRTEPermissionInfo legitimately returns NULL for result relations that have no RTEPermissionInfo (e.g., some internally-generated result rels exercised by the injection_points isolation scenarios). The pre-existing code handled this; the refactor to cache perminfo->updatedCols into a local broke it.

This is a direct consequence of the v2/v5 design choice to centralize the fix in ExecGetUpdatedCols — the centralization itself is still sound, but the NULL-handling contract of that function needed to be preserved, and wasn't.

v9 (Paul) — fix ordering

Paul confirmed he hadn't built with injection_points enabled and reproduced the crash once he did. v9 reorders the NULL check to precede the perminfo->updatedCols access. No other substantive changes.

Minor: comment accuracy (Jian)

Jian flagged the comment in the lazy ExecInitForPortionOf site that says "partition child being hit for the first time". He notes this is incomplete because table inheritance (classical inheritance, not partitioning) hits the same code path, and suggests "child table" as a more accurate term. This is cosmetic but worth noting because it hints that the lazy-init path is exercised by regular inheritance as well — something the v3–v5 partitioning-correctness work focused on but didn't explicitly call out for the inheritance case.

Assessment

No design shift. The centralize-in-ExecGetUpdatedCols approach survives. The v8 crash is a straightforward ordering bug in the refactor, fixed in v9. The remaining open item is purely a comment wording nit.

History (1 prior analysis)
2026-05-06 · opus 4.7

FOR PORTION OF and updatedCols: A Bug Exposing a Conceptual Overload

The Core Problem

PostgreSQL 18's FOR PORTION OF feature (temporal UPDATE/DELETE, committed by Paul Jungwirth) narrows a range-typed column to a sub-interval, inserting "leftover" rows for the portions outside the targeted sub-range. Satya Narlapuram reported that when a GENERATED ALWAYS AS ... STORED column references the range column, the stored value becomes stale on the updated row:

UPDATE t FOR PORTION OF valid_at FROM 30 TO 70 SET val = 99;
-- row [30,70) still has range_len = 99 (inherited from original [1,100)),
-- not the correct 40.

The leftover rows [1,30) and [70,100) come out correct because they are synthesized via CMD_INSERT, and insert paths unconditionally recompute all generated columns. The directly updated row goes through the UPDATE path, which only recomputes generated columns whose dependencies intersect perminfo->updatedCols. Virtual generated columns are unaffected because they are re-evaluated at read time from the stored range value.

Why the range column is absent from updatedCols

transformForPortionOfClause() deliberately omits the range column from RTEPermissionInfo.updatedCols. The rationale is a user-facing permissions decision: a user with UPDATE privilege on val but not on valid_at is still allowed to execute UPDATE ... FOR PORTION OF valid_at, because the user is not semantically updating valid_at — the system is narrowing it as a side effect of the temporal operation.

The bug is that updatedCols is used for more than permissions checking. Over time it has accumulated roles:

  1. Permissions enforcement (ExecCheckPermissions).
  2. Choosing which GENERATED STORED columns to recompute (ExecInitStoredGenerated / ExecComputeStoredGenerated).
  3. Firing UPDATE OF column triggers.
  4. HOT / index-update optimizations via ExecGetExtraUpdatedCols → index_unchanged_by_update.

Because the range column is physically being rewritten on disk, every consumer in (2)–(4) needs to know about it; only (1) wants it hidden. The fix therefore sits at the boundary of a conceptual overload that predates FOR PORTION OF.

Evolution of the Fix

v1 (Satya): add in ExecInitGenerated

Satya's initial patch added the range attno to the local updatedCols only inside ExecInitGenerated. This fixes generated columns but leaves triggers and index_unchanged_by_update wrong. It is also scoped too narrowly to address the broader inheritance / trigger issues Jian was tracking in parallel threads.

v2 (Jian): centralize in ExecGetUpdatedCols

Jian Universality argued for putting the adjustment in ExecGetUpdatedCols itself, caching the result on estate->es_query_cxt (mirroring how ExecGetExtraUpdatedCols caches ri_extraUpdatedCols). Reasons:

  • Several callers use ExecGetUpdatedCols without ExecGetExtraUpdatedCols, so the fix needs to live in the former.
  • Permission checking (ExecCheckPermissions in InitPlan) runs before ExecGetUpdatedCols is first invoked, so the permission bypass for the range column is preserved.

Jian also raised an architectural concern: ri_forPortionOf is lazily initialized deep in ExecForPortionOfLeftovers (inside ExecUpdateEpilogue). That is "too late" for other code paths that want to consult fp_rangeAttno earlier. He proposed initializing ForPortionOfState right after the ExecLookupResultRelByOid call, and noted redundancy between ForPortionOfExpr (plan node) and ForPortionOfState (exec state).

Additionally, Jian flagged fp_leftoverstypcache as apparently unused (dead code that could move to ExecInitModifyTable), and pointed out a stale comment claiming "other types may have more" leftovers when only anyrange/anymultirange are supported.

v3–v5 (Paul): partitioning correctness

Paul Jungwirth — the FOR PORTION OF author and thus the domain authority here — accepted Jian's centralization approach but caught a partitioning bug: ri_forPortionOf->fp_rangeAttno on a leaf partition has already been mapped from parent to child attnos. Adding the unmapped parent attno to updatedCols and then running the tuple-conversion mapping again would double-map.

The v5 fix adds the range attno to updatedCols after the parent→child mapping is applied, and therefore explicitly avoids mutating perminfo->updatedCols (which holds parent attnos and must not carry child-specific entries). Paul added a partitioned-table regression test with a GENERATED STORED column that reproduces the mis-mapping.

Paul expressed discomfort with mutating perminfo at all — it optimizes away repeated bitmapset allocations for multi-row updates but is surprising as a side effect on a cached catalog-derived structure.

v5→v7: segfault scare and polish

Jian reported a segfault in v5; after a clean rebuild it disappeared (stale build artifacts). The remaining review was cosmetic: remove an accidental #include "nodes/print.h", refactor a regression test to compute range_len by hand for easier diffing, and rebase past d3bba04154.

Secondary Issues Raised and Their Resolutions

Satya also reported two related scenarios:

  1. BEFORE INSERT trigger returning NULL kills leftovers. A DELETE FOR PORTION OF against a table with a BEFORE INSERT trigger that returns NULL silently loses both leftover rows — the deletion succeeds but no leftover is inserted, violating the user's evident intent.

  2. Same issue on UPDATE: the leftovers vanish, and only the narrowed row survives.

Both Jian and Paul classified this as expected behavior, citing the documented warning that FOR PORTION OF fires INSERT triggers on leftovers and a trigger returning NULL means "do nothing". The ExecInsert path returns NULL on ExecBRInsertTriggers failure, which is the standard trigger contract. Changing this would require special-casing leftover inserts, which the thread declined to do.

Deeper Design Question Raised by Paul

Paul raised the key architectural question explicitly: should updatedCols be split into two bitmapsets — one for permissions, one for "physically modified columns"? Today the patch is essentially a workaround: updatedCols keeps its permission semantics, and each consumer that cares about physical modification is patched to add the range attno. A cleaner refactor would invert the defaults: make updatedCols mean "physically modified", and move the permission carve-out into ExecCheckPermissions.

This refactor was not undertaken — the bug needs a backpatchable fix first — but it is the right long-term direction and was left as an open question.

Implications

  • Correctness on a committed feature. FOR PORTION OF shipped with this latent bug; any user combining temporal UPDATE with stored generated columns, UPDATE OF col triggers, or HOT-sensitive indexes on the range column would see wrong results or missed optimizations.
  • index_unchanged_by_update impact. Without the fix, B-tree bottom-up deletion heuristics would incorrectly treat the range column as unchanged on a FOR PORTION OF update, potentially triggering suboptimal deduplication behavior on indexes that include the range column.
  • Partition-aware bitmapset handling. The partitioning half of the fix is a reminder that any code touching tuple-routed updatedCols must be mindful of whether attnos are in parent or child coordinate space.