Inconsistent trigger behavior between two temporal leftovers

Core Problem: Trigger Asymmetry in FOR PORTION OF Leftovers

Background: SQL:2011 Temporal Updates

PostgreSQL's UPDATE ... FOR PORTION OF (new in the temporal tables feature work, pushed by Paul Jungwirth) allows updating only a sub-range of a row's validity period. When the updated portion falls strictly inside an existing row's range, the executor must produce two "leftovers": the portion of the original row before the updated range, and the portion after. These leftovers are physically implemented as INSERTs of the original row with adjusted range bounds, while the updated slice is UPDATEd in place (or represented as its own row).

Because the leftovers are inserts, any BEFORE INSERT ... FOR EACH ROW trigger defined on the table fires for each of them. This is where the asymmetry reported by Sergei Patiakin surfaces.

The Bug

The executor was reusing a single leftoverSlot for both leftover inserts. The sequence was roughly:

1. Copy original row into leftoverSlot.
2. Adjust range bounds for the pre-update leftover.
3. Call ExecInsert() → fires BEFORE INSERT ROW triggers → trigger function may mutate NEW (the slot).
4. For the second leftover, re-use the now-mutated slot, only adjusting the range bounds.

The example in the thread makes this concrete: a revision := revision + 1 BEFORE trigger produces revision = 2 on the first leftover (starting from 1 after the first increment... wait, actually starting from original 0, the first leftover becomes 1 after trigger, the "updated" slice becomes 1, but then the slot carrying the first leftover's post-trigger value of 1 is reused, incremented again on insert → 2 for first leftover's stored value, and then incremented once more → 3 for second leftover). The second leftover inherits modifications the trigger made to the first leftover, breaking the intuitive invariant that each leftover should be a fresh derivative of the original row.

Why This Matters Architecturally

This is not just a cosmetic issue. FOR PORTION OF semantics are defined as "conceptually, split the row into three pieces and apply the update to the middle piece." Users writing audit/revision/history triggers reasonably expect each leftover to be an independent INSERT of the original row. Cross-contamination via a shared slot breaks the mental model and makes trigger semantics path-dependent on the internal iteration order of the executor — essentially leaking an implementation detail into user-visible behavior.

It also interacts with the still-unsettled broader question raised in the referenced thread [1]: should INSERT triggers even fire for FPO leftovers at all, or should they be treated as internal reorganizations? Sergei explicitly sidesteps that larger design debate and argues only for symmetry: whatever the behavior, it should be the same for both leftovers.

The Fix

The patch re-copies the original (pre-trigger) tuple into leftoverSlot before each leftover's ExecInsert() call, rather than relying on the slot retaining its original contents. Paul Jungwirth independently arrived at essentially the same fix and noted he tried to factor out more shared code between the "first pass" and "subsequent pass" leftover handling, but the presence of a tuple conversion map (for partitioned tables, where the leaf partition's rowtype may differ from the root) made unification awkward. The tuple mapping branch via execute_attr_map_slot() has to be handled differently from the no-mapping branch, hence the minor duplication.

Tangent: execute_attr_map_slot() Return Value

Peter Eisentraut raised a meta-question: existing call sites are inconsistent about whether they capture the return value of execute_attr_map_slot(). Sergei confirmed the function always returns its out_slot argument, making assignments like out_slot = execute_attr_map_slot(map, in, out_slot) pure no-ops. Sergei defends the API as conventional (compare strcpy, ExecCopySlot, realpath) — returning the destination buffer for call chaining is idiomatic C. This was left as an observation rather than a blocker; cleanup of existing no-op assignments was floated but not pursued in this patch.

Comment Precision

Zsolt Parragi caught that a new code comment referred to "BEFORE INSERT triggers" while the rest of the patch (commit message, tests, other comments) was careful to say "BEFORE ROW INSERT triggers" — the distinction matters because STATEMENT-level triggers don't see per-row NEW and can't cause this bug. Paul produced an updated patch fixing the wording.

Design Decisions and Tradeoffs

1. Re-copy vs. defensive copy up-front: The fix chose to re-materialize the original tuple into the slot before each leftover insert. An alternative would be to allocate a fresh slot per leftover, but this would churn more memory. Re-copying into an existing slot is cheaper and minimal.

2. Not addressing the "should triggers fire at all" question: The patch is deliberately narrow. It accepts current semantics (BEFORE INSERT ROW triggers fire for leftovers) and only fixes the cross-contamination. This keeps the change backpatchable-friendly and avoids relitigating the larger temporal-semantics debate.

3. Code duplication accepted: Paul's attempt to unify the first-leftover and second-leftover code paths failed cleanly because of the attribute-map (partition routing) branch. Both reviewers accepted the minor duplication rather than forcing a clumsy unification.

Participant Weight

• Paul A Jungwirth is the principal author of the temporal tables / FOR PORTION OF feature; his acceptance that this is a genuine bug and his concurrent independent fix lend strong authority. He also drove the commitfest entry.
• Peter Eisentraut is a senior committer; his question about execute_attr_map_slot() signals that even if it's not blocking, the API's inconsistent usage is a latent documentation debt worth flagging.
• Sergei Patiakin (EDB) is the reporter and patch author; his framing around symmetry rather than correctness of firing is a diplomatic way to isolate a fixable bug from a contested design question.
• Zsolt Parragi (Percona) provided careful review on comment precision — a small but quality-improving contribution.

Takeaways

The bug exemplifies a class of issues that arise when user-visible semantics depend on slot lifetime management in the executor. Any time a single TupleTableSlot is reused across multiple logical operations that can invoke user code (triggers, expressions), the executor must either (a) ensure the slot is re-initialized between iterations, or (b) document and accept the mutation. FPO leftovers made the wrong choice silently, and the fix enforces option (a).