Remove batching from the RI fast-path FK check - #2
Open
amitlan wants to merge 3 commits into
Open
Conversation
Owner
Author
|
@copilot review. |
amitlan
force-pushed
the
19-ri-fastpath-revert
branch
from
September 2, 2026 08:40
82a7ff4 to
e4371d6
Compare
ri_FastPathCheck() acquired its scan snapshot before opening the referenced relation. If it then waited for the relation lock in READ COMMITTED mode, a referenced row committed during the wait would not be visible to the old snapshot. The check could consequently report a foreign key violation even though the referenced row existed. The batched path does not have this problem, because it opens the relations before acquiring the snapshot used to check the batch. Consequently, batching currently masks the problem for ordinary DML. Fix the per-row path before removing batching and making that path handle those checks. Take the snapshot after opening the referenced relation and reloading the constraint information. This also agrees with the SPI path, which selects its snapshot after executor startup has acquired the required locks. Add isolation-test coverage for the visibility of a referenced row committed after the referencing transaction has executed an earlier command. A later command can see such a row in READ COMMITTED, but not in REPEATABLE READ or SERIALIZABLE. Discussion: https://postgr.es/m/CA+HiwqEhm+_=bs=2wavAJz-UqC+1KebD31++mapJQQGweE8iQQ@mail.gmail.com Backpatch-through: 19
amitlan
force-pushed
the
19-ri-fastpath-revert
branch
from
September 2, 2026 12:59
e4371d6 to
779cec5
Compare
Commit b7b27eb added batching to the direct-index fast path for foreign key checks introduced by 2da86c1. Instead of probing the referenced index once per row, it accumulated referencing rows and checked them in groups, using SK_SEARCHARRAY for single-column foreign keys. The batching requires state to survive across trigger invocations and to be flushed at the end of each trigger-firing cycle. Follow-up work has had to define how that state interacts with nested trigger firing, subtransactions, deferred constraints, and SET CONSTRAINTS. In particular, SET CONSTRAINTS ... IMMEDIATE invoked from a trigger can re-enter the after-trigger machinery while an outer batch remains active. Failure to handle one of those cases can leave a buffered check unperformed, allowing a transaction to commit a permanent foreign key violation without reporting an error. With PostgreSQL 19 close to release, there is not enough time to gain confidence that all relevant trigger and transaction states have been covered. Remove the batching and its after-trigger callback infrastructure. This also removes the per-batch RI cache and the associated subtransaction cleanup. Restore AfterTriggerFireDeferred() to its form before batching was added. Retain the tests added with the batching commit and its follow-up fixes, because they continue to exercise the underlying RI cases through the per-row path. This preserves regression coverage for those behaviors and keeps test coverage aligned with master, simplifying future backpatching of test cases. Keep the underlying per-row fast path. It performs each check synchronously, retains no state across trigger invocations, and requires no changes to the trigger or subtransaction machinery. Also retain the fast-path metadata invalidation handling and the fixes made to the per-row probe, including support for domain-typed referencing columns, restriction to btree referenced indexes, concurrent index replacement, metadata invalidation, and nullable referenced keys. This removal applies only to REL_19_STABLE. The batched implementation is retained in master for v20 development. Discussion: https://postgr.es/m/
amitlan
force-pushed
the
19-ri-fastpath-revert
branch
from
September 2, 2026 13:45
779cec5 to
324f846
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit b7b27eb added batching on top of the direct-index fast path for foreign key checks introduced by 2da86c1: FK rows are buffered and probed in groups using SK_SEARCHARRAY, rather than probed one at a time. This removes that layer and leaves the per-row fast path in place.
The batching was proposed late in the v19 cycle and its transactional design was completed after feature freeze. The four most recent commits touching it -- 3b70fa6, f3a52a2, d2a710c and 268958a -- are not fixes to settled code; they are the state model itself, establishing how a batch relates to a subtransaction and to a trigger firing cycle. The open crash where SET CONSTRAINTS ... IMMEDIATE issued from a trigger body walks the after-trigger event list re-entrantly while an outer batch is live is a defect in the most recent of them.
The concern is not the number of follow-up fixes but that new transaction and trigger states were still being identified weeks before release, in a code path whose failure mode is a foreign key check that is buffered and never performed. That produces an INSERT that succeeds and a row that violates its constraint permanently, with no error at any point. Testing can show that the states we have enumerated behave correctly; it cannot show the enumeration is complete, and the commit history suggests it is not yet.
Removed: ri_FastPathBatchAdd(), ri_FastPathBatchFlush(), ri_FastPathFlushArray(), ri_FastPathFlushLoop(), ri_FastPathGetEntry(), ri_FastPathEndBatch(), ri_FastPathTeardown(), the RI_FastPathEntry and RI_FastPathKey structures, the fast-path entry cache and its in-flush flag, and AtEOSubXact_RI(), which existed only to drop cache entries belonging to an aborting subtransaction.
Retained: the per-row fast path (ri_FastPathCheck(), ri_FastPathProbeOne(), ri_LockPKTuple(), recheck_matched_pk_tuple()) and every fix to it -- 68ace96 (domain-typed FK columns), 8c0aa08 (btree-only referenced indexes), 18a15b9 and abca128 (fast-path metadata lifetime), and the nullable-referenced-key handling from a05ece5, whose ri_FastPathFlushArray() site goes with the flush function while its recheck_matched_pk_tuple() site remains.
AtEOXact_RI() is retained but reduced to releasing FastPathMeta objects detached by InvalidateConstraintCacheCallBack(); its cache-not-flushed warning has no subject once the cache is gone.
In trigger.c, everything the batching used was introduced for it: at 2da86c1 -- the fast path without batching -- afterTriggers.firing_depth, AfterTriggerIsActive(), AfterTriggerCurrentQueryDepth(), the batch callback list and AfterTriggerBatchCallback do not exist, and none of them exist in v18 either. So all of it goes, including the subtransaction-end restore added by f3a52a2, which restores firing_depth and firing_batch_callbacks and has no subject once neither field exists.
AfterTriggerFireDeferred()'s loop is restored to its pre-batching form. b7b27eb removed its "all fired" break so that events queued by a batch callback would be seen on the next iteration; with no callbacks to run inside the loop, the break comes back.
Batching remains a reasonable optimisation and should be revisited for v20, developed over a full cycle.