Deferrable unique constraints are enforced at insert time on the import path, not deferred to commit

[ChronicallyJD]

Split out of #167 at review request, so the deviation is tracked rather than living in a file header.

Status. Diagnosed, and a prototype has shown the fix works. This is a well-specified piece of work, not an open question. See the thread for the prototype and its evidence.

The behaviour

ColumnarIndexInsertRow decides on indisunique alone:

if (enforceUnique && st->rels[i]->rd_index->indisunique)
    check = UNIQUE_CHECK_YES;

The executor also consults indimmediate. So a UNIQUE ... DEFERRABLE INITIALLY DEFERRED constraint is checked at commit through ordinary DML and at insert time through import_arrow / import_parquet.

Measured, with controls

	transient collision, deferred constraint
heap, ordinary INSERT	COMMIT
pgcolumnar, ordinary INSERT	COMMIT
pgcolumnar, import_arrow	ERROR: duplicate key value violates unique constraint

The middle row is the important one: pgcolumnar's ordinary insert path defers correctly. This is not the access method lacking something. It is the import path bypassing the executor, which is what maintains indexes.

The collision must exist when the row is inserted and be gone by commit. Deleting the conflicting row first makes the import succeed and proves nothing.

The fix: use the executor's index maintenance

Not a patch to the hand-rolled path. Give the import path a real executor context and let core do it:

EState with a range table and RTEPermissionInfo, InitResultRelInfo, ExecOpenIndices, AfterTriggerBeginQuery() / AfterTriggerEndQuery(estate) bracketing the load, and the enforcing path routed through ExecInsertIndexTuples + ExecARInsertTriggers.

ExecInsertIndexTuples selects UNIQUE_CHECK_PARTIAL for a non-immediate index by itself, and ExecARInsertTriggers queues the recheck. So indimmediate stops being ours to get right, and so do partial-index predicates, expression indexes, exclusion constraints and unique checking, all of which columnar_index.c currently re-implements. Each is a place our copy can drift from core, and #153 and #167 were both that drift found late.

The rewrite caller in columnar_vacuum.c keeps the hand-rolled path: it deliberately enforces nothing, and ExecInsertIndexTuples has no mode for that.

Two preconditions, which is what the crashes were

A prototype crashed twice. Neither cause was the trigger machinery being unusable outside the executor; both were preconditions it assumes and does not check for you.

One. ExecInsertIndexTuples requires the slot to carry its relation:

Assert(slot->tts_tableOid == RelationGetRelid(heapRelation))   execIndexing.c:329 (17), 341 (18)

MakeSingleTupleTableSlot does not set tts_tableOid. The hand-rolled path never needed it because it passes the TID and heap relation to index_insert directly. One assignment.

Two. A statement that queues an after-trigger event must hold its lock until commit. ExecGetTriggerResultRel reopens the relation with NoLock, and says why:

 * Open the target relation's relcache entry.  We assume that an
 * appropriate lock is still held by the backend from whenever the trigger
 * event got queued, so we need take no new lock here.

columnar_import_arrow does table_close(rel, RowExclusiveLock) on the way out, dropping it while a queued event still refers to the table, and the deferred fire at COMMIT asserts. table_close(rel, NoLock) is the change. columnar_parquet_reader.c has the same shape at line 2891; columnar_vacuum.c closes with ShareUpdateExclusiveLock and must keep doing so, since the rewrite queues nothing.

Both crashes were legible only because the build was assert-enabled. On a non-assert build they are a segfault and a silent wrong behaviour.

What the prototype settled, and what it did not

Settled:

• the executor route works and defers correctly
• the full suite still passes (80, 0 fail, PG18) including import_exclusion, arrow_import, parquet_import, native_index, unique_conc

Not settled:

• that columnar_index.c's duplicated indimmediate, partial-predicate and exclusion handling can then be deleted cleanly, which is the larger prize
• the EState lifetime under an immediate-but-deferrable constraint fired mid-transaction
• anything on PG15, 16, 17 or 19

What the tests must separate

"The import committed" is true both when deferral works and when the check quietly disappeared. These distinguish them, and every one must match heap:

case	expected
deferred, transient collision	COMMIT
deferred, permanent duplicate	ERROR at commit, import rolled back
IMMEDIATE constraint, duplicate	ERROR at insert time
SET CONSTRAINTS ... IMMEDIATE mid-transaction	ERROR there
exclusion constraint	still enforced

Rows two and four are what catch a fix that merely stopped enforcing. A row count alone does not: it reads the same whether the import commits or aborts.

Priority

Below #155. The workload this blocks is narrow: an import whose rows transiently collide with rows the same transaction removes. Enforcing early is over-strict rather than unsound, which is the safe direction to be wrong in while this is open.

[jdatcmd]

Reviewed and reproduced. The description is exactly right.

DEFERRABLE INITIALLY DEFERRED unique constraint, PG18, same shape through both
paths:

ordinary DML
  BEGIN; INSERT two duplicates       -> allowed, 4 rows mid-transaction
  DELETE the older two; COMMIT       -> succeeds, 2 rows

import_arrow of the same rows
  BEGIN; import_arrow                -> ERROR: duplicate key value violates
                                        unique constraint "tgt_k"
                                        transaction aborted

So an import that would be consistent by commit is refused partway through,
where ordinary DML accepts it. Over-strict rather than unsound, as you say, and
the safe direction to be wrong in.

On priority

I would leave this below #155 and #171 rather than take it next. The workload it
blocks is narrow: an import whose rows transiently collide with rows the same
transaction removes. Anyone loading into a fresh table, or into a table whose
keys do not overlap, never meets it. Meanwhile #171 has autoanalyze running an
operation that took tens of minutes in my measurements, which reaches people who
did nothing unusual.

On the sketch

UNIQUE_CHECK_PARTIAL plus a queued recheck is the right shape and matches what
ExecInsertIndexTuples does. The part worth thinking about before starting: the
executor queues the recheck as an after-trigger event tied to the tuple, and the
import path has no ResultRelInfo or EState for that machinery, so either it
builds enough of one or it needs a different way to hold the deferred list until
commit. That is the piece that decides whether this is a day or a week.

Worth writing down in the issue when you get to it, so the estimate has the
reason attached.

[jdatcmd]

Attempted this and backed it out. Recording what the attempt establishes, because it rules out the sketch in the issue body and the next attempt should not rediscover it.

What was built

The obvious shape, mirroring ExecInsertIndexTuples:

• decide on indimmediate rather than indisunique, so a deferred index takes UNIQUE_CHECK_PARTIAL
• collect the indexes whose index_insert returned false into a recheck list
• set slot->tts_tid before queuing, since the event records the row's item pointer and the recheck refetches by it
• hand the list to ExecARInsertTriggers, which is what makes AfterTriggerSaveEvent queue the constraint's internal unique_key_recheck trigger

It compiles clean and the no-conflict path works. ExecARInsertTriggers and InitResultRelInfo are byte-identical across PG15 through PG19, so no compatibility work is involved.

Why it does not work

afterTriggers.query_depth is -1 while import_arrow runs, even though the function executes under ExecutorRun of an ordinary SELECT pgcolumnar.import_arrow(...), and standard_ExecutorStart calls AfterTriggerBeginQuery() unconditionally for every operation including SELECT, identically on all five majors. So the queue refuses the event outright:

ERROR:  XX000: AfterTriggerSaveEvent() called outside of query
LOCATION:  AfterTriggerSaveEvent, trigger.c:6194

I have not explained why the depth is -1 there, and that is the open question this issue now turns on.

Opening a level from the import is not the answer either, and fails in two different ways:

• AfterTriggerBeginQuery() alone makes the queuing succeed, which confirms the depth is genuinely the obstacle -- and then crashes at COMMIT on TRAP: failed Assert("afterTriggers.query_depth == -1"), trigger.c:5293, the deferred-fire path insisting the stack be unwound.
• Pairing it with AfterTriggerEndQuery(estate) segfaults, on an EState from a bare CreateExecutorState(). Closing a level runs the mark-and-invoke path, which wants a properly set-up executor state rather than the minimal one this file builds for FormIndexDatum.

So the recheck event cannot be queued from this context without either explaining the depth or giving the import path a real executor state and query level. That is more than a targeted patch, which is what the issue already suspected.

Tests, not kept

Four sections were written against a heap oracle: a transient duplicate committing as it does on heap, a real duplicate returning from the import and failing at COMMIT, an immediate constraint still raising during the import, and SET CONSTRAINTS ALL IMMEDIATE raising at that point. They are worth rewriting when this is retried.

Two of them passed for the wrong reason on the first run and were caught: comparing the row count after the transaction reads 2 whether the import commits or aborts, so the count has to be paired with an assertion that the import itself raised nothing.

State

main is unchanged and still enforces early. Over-strict rather than unsound remains the safe direction to be wrong in while this is open.

[ChronicallyJD]

The open question has an answer in standard_ExecutorStart, and it is the one assumption in your write-up that does not hold.

standard_ExecutorStart calls AfterTriggerBeginQuery() unconditionally for every operation including SELECT

It does not. A plain SELECT is explicitly opted out, in execMain.c:

case CMD_SELECT:
    ...
    /*
     * A SELECT without modifying CTEs can not possibly queue triggers,
     * so force skip-triggers mode. This is just a marginal efficiency
     * hack, since AfterTriggerBeginQuery/AfterTriggerEndQuery are not
     * all that expensive, but we might as well do it.
     */
    if (!queryDesc->plannedstmt->hasModifyingCTE)
        eflags |= EXEC_FLAG_SKIP_TRIGGERS;
    break;

and further down:

if (!(eflags & (EXEC_FLAG_SKIP_TRIGGERS | EXEC_FLAG_EXPLAIN_ONLY)))
    AfterTriggerBeginQuery();

SELECT pgcolumnar.import_arrow(...) is a CMD_SELECT with no modifying CTE, so EXEC_FLAG_SKIP_TRIGGERS is set before the call and the query level is never opened. afterTriggers.query_depth stays at its between-queries value of -1, and AfterTriggerSaveEvent rejects the event. That is the whole mechanism.

I checked it in the 17.6 and 18.4 trees; the block is identical in both, so it is not a version difference.

What it means for this issue

It is not an accident of how the importer is written, and it is not something a targeted patch works around:

• The reason is a deliberate optimisation with a comment saying so, on the grounds that a SELECT cannot queue triggers. An access-method function that queues one from inside a SELECT is exactly the case the optimisation assumes away.
• It confirms your two failures were the right diagnosis rather than bad luck. Opening the level by hand makes queuing succeed because the depth genuinely was the obstacle, and then the deferred-fire path asserts on unwinding a stack nobody closed. Closing it properly runs mark-and-invoke, which needs a real executor state — and the one this file builds exists only to feed FormIndexDatum.
• It also predicts a user-visible oddity, which I have not tested and which I would not build on: the same import inside a statement that carries a modifying CTE would have a query level and so could queue. A correctness property that depends on the shape of the statement calling the function is not one to rely on.

So the sketch in the issue body is ruled out, and I would rewrite the body to say so rather than leave it as the suggested approach. The remaining routes are giving the import path a real executor state and query level, which is a bigger piece of work than this issue was scoped for, or deciding the deviation is acceptable and closing it by measurement.

For what it is worth, my own view has shifted toward the second while this stays open: enforcing early is over-strict rather than unsound, and the case it breaks is a transient duplicate inside one import, which I have not seen a real motivation for. If you agree, the honest close is a note in the docs about the deviation plus this comment as the record of why the obvious fix does not work.

Also worth keeping from your attempt: two of the four tests passing for the wrong reason because a row count reads the same whether the import commits or aborts. That is the same failure mode as the ::text cast that made my aggregate suite vacuous a few PRs ago — a check that is satisfied by both outcomes.

[jdatcmd]

You are right, and I verified it in the 15, 17 and 19 trees rather than take it on trust. The block is identical in all three:

case CMD_SELECT:
    ...
    if (!queryDesc->plannedstmt->hasModifyingCTE)
        eflags |= EXEC_FLAG_SKIP_TRIGGERS;

My error was specific and avoidable, and worth naming because it is a repeatable one: I grepped the call site, if (!(eflags & (EXEC_FLAG_SKIP_TRIGGERS | EXEC_FLAG_EXPLAIN_ONLY))) AfterTriggerBeginQuery();, confirmed it was unconditional there, and concluded the level always opens. I never checked where eflags came from. It is mutated about forty lines earlier in the same function. I checked a statement and not its input.

Your explanation also converts both of my failures from mysterious to expected, which is the test of a real explanation rather than a plausible one.

Where I agree, and where I would go further

The modifying-CTE case is decisive, not an oddity. You flag it as something you would not build on. I would put it stronger: it rules out an entire class of fixes. Any implementation that queues opportunistically makes a correctness property depend on the shape of the caller's statement, so the same import would defer or not according to whether the surrounding SELECT happened to carry a modifying CTE. That is worse than the uniform over-strictness we have now, because it is unpredictable rather than merely wrong in one direction. It forces any real fix to establish its own query level unconditionally, or not to be attempted.

One thing I would push back on: "closing it by measurement". There is nothing here to measure. This is a semantics deviation from PostgreSQL, not a performance claim. Closing it would be a scope decision, not a measured finding.

That distinction is worth keeping sharp in this project specifically, because we do have a real precedent for closing by measurement: the ColumnarDeleteVectorBufferedDeleted nested scan, where the shape was real, the cost measured linear rather than quadratic, and the obvious cache made it slower. That close was earned by numbers. Filing this one under the same heading would borrow credibility the evidence here does not supply.

And it runs into the project's standing rule, which is that we do not document a defect and keep it, we document it and fix it. Documenting the deviation and closing is the case that rule exists to forbid. I am flagging it rather than deciding it, since it is the owner's policy call, not ours.

The route I would take instead

I do not think the choice is between a week of work and a docs note. There is a third option that is more attractive than either.

Stop trying to queue the recheck by hand, and give the import path the executor's own index maintenance: a properly initialised EState and ResultRelInfo, ExecOpenIndices / ExecInsertIndexTuples / ExecARInsertTriggers, with its own after-trigger query level bracketing the load.

What that buys, beyond closing this issue as a by-product:

• indimmediate stops being ours to get right. So do partial-index predicates, expression indexes, exclusion constraints and unique checking, all of which columnar_index.c currently re-implements. Each is a place where our copy can drift from core as core changes, and import_arrow and import_parquet do not maintain indexes: index scans silently return nothing, UNIQUE is not enforced #153 and Enforce exclusion constraints when importing (follow-up to #158) #167 were both that drift being discovered late.
• It deletes code rather than adding a special case to it.

The rewrite caller in columnar_vacuum.c keeps the hand-rolled path unchanged, because it deliberately enforces nothing and ExecInsertIndexTuples has no mode for that.

The open risk is the part that crashed on me: closing a query level runs mark-and-invoke, and the EState this file builds exists only to feed FormIndexDatum. My segfault is consistent with that being the cause, but I have not proved it, and I would want it proved before anyone estimates the work. That is the one experiment worth running before we commit to a plan: build a real executor state for the import and see whether AfterTriggerBeginQuery / AfterTriggerEndQuery around it survives a deferred constraint.

Sequencing

I agree with your original ordering, and #171 closing does not change it. #155 reaches people who did nothing unusual; this reaches someone whose import transiently collides with rows the same transaction removes. #155 next.

I will also rewrite the issue body so the UNIQUE_CHECK_PARTIAL plus queued-recheck sketch stops reading as the recommended approach, and points at this thread instead.

On the vacuous checks

Agreed, and it is the same failure as the ::text cast: a check satisfied by both outcomes. Mine compared a row count that reads 2 whether the import commits or aborts. The fix in both cases is the same shape, which is to assert the thing that differs between the two worlds rather than a value they share. Worth writing into the testing notes as a named trap, since that is four occurrences between us now.

[ChronicallyJD]

You are right about "closing by measurement" and I withdraw the framing. There is nothing here to measure; closing would be a scope decision wearing the clothes of a finding, and the ColumnarDeleteVectorBufferedDeleted precedent is what makes that borrowing visible. My recommendation was wrong in kind, not just in degree.

So I ran the experiment you named instead of arguing the policy. The answer is yes, and the reason it looked otherwise is smaller than either of us thought.

First, a reproducer with controls

Before prototyping anything I wanted the deviation isolated, because "the import enforces early" is satisfied by several different bugs.

	transient collision, deferred constraint
heap, ordinary INSERT	COMMIT
pgcolumnar, ordinary INSERT	COMMIT
pgcolumnar, import_arrow	ERROR: duplicate key value violates unique constraint

The collision has to exist when the row is inserted and be gone by commit, which my first attempt got wrong — I deleted the conflicting row first and the import committed, proving nothing.

Worth noting the middle row: pgcolumnar's ordinary insert path defers correctly. This is not the access method lacking something, it is the import path bypassing the executor.

The prototype

EState with a real range table and RTEPermissionInfo, InitResultRelInfo, ExecOpenIndices, AfterTriggerBeginQuery() at begin and AfterTriggerEndQuery(estate) at end, and the enforcing path routed through ExecInsertIndexTuples + ExecARInsertTriggers. The rewrite caller keeps the hand-rolled path, as you said it must.

It crashed exactly as yours did. Both causes turned out to be preconditions, not the mark-and-invoke problem.

One. ExecInsertIndexTuples requires the slot to carry the relation:

Assert: slot->tts_tableOid == RelationGetRelid(heapRelation)   execIndexing.c:341

MakeSingleTupleTableSlot does not set tts_tableOid, and the hand-rolled path never needed it because it passes the TID and heap relation to index_insert directly. One assignment.

Two, and this is the one worth having. With that fixed, the transient case passed and the permanent duplicate crashed at commit:

Assert: lockmode != NoLock || ... || CheckRelationLockedByMe(r, AccessShareLock, true)
#6  relation_open (relationId=99068, lockmode=NoLock)      relation.c:67
#8  ExecGetTriggerResultRel                                execMain.c:1400
#9  afterTriggerInvokeEvents                               trigger.c:4752
#10 AfterTriggerFireDeferred
#11 CommitTransaction

99068 is the import target. ExecGetTriggerResultRel reopens the relation with NoLock because it assumes the transaction still holds a lock on anything it queued events for — and columnar_import_arrow does table_close(rel, RowExclusiveLock) on the way out, dropping the lock while the queued event still refers to the table. A statement that queues after-triggers has to hold its lock until commit; table_close(rel, NoLock) is the whole change.

Neither is AfterTriggerEndQuery being unable to run against an EState this file builds. Your hypothesis was reasonable and is what I expected to confirm, but the evidence does not support it: once the relation is locked and the slot is labelled, closing the query level fires the deferred events and returns normally.

It defers rather than stops checking

"The import committed" is true in both the fixed world and the world where the check quietly disappeared, so that alone proves nothing — the trap we have now hit five times between us. These are the cases that separate them:

case	result
deferred, transient collision	COMMIT
deferred, permanent duplicate	ERROR at commit, import rolled back, table back to 1 row
IMMEDIATE constraint, duplicate	ERROR at insert time
SET CONSTRAINTS ... IMMEDIATE mid-transaction	ERROR there
exclusion constraint	still enforced

Every one matches heap. The second and fourth rows are the ones that would have caught a fix that merely stopped enforcing.

Also worth recording for the issue body you are rewriting: ExecInsertIndexTuples chooses UNIQUE_CHECK_PARTIAL for a non-immediate index by itself, and ExecARInsertTriggers queues the recheck. So the sketch in the body is right about the mechanism and wrong only about who should implement it — which is a more useful thing for it to say than "see this thread".

Scope, and what I did not do

This is a throwaway prototype to answer your question, not a patch, and I have not opened one.

• Arrow import only. columnar_parquet_reader.c has the same table_close(rel, RowExclusiveLock) shape and would still crash on the permanent-duplicate case. Same one-word change, but untested.
• The EState lifetime is unresolved. ColumnarIndexInsertEnd calls FreeExecutorState after AfterTriggerEndQuery, which is fine here because the deferred events fire at commit against an EState the trigger machinery builds itself. Whether that holds for an immediate-but-deferrable constraint fired mid-transaction I have not probed.
• The bigger prize in your plan is untested. I did not delete columnar_index.c's copies of indimmediate, partial predicates or exclusion handling. The experiment only shows the executor route is viable; it does not show the deletion is clean.
• PG18 assert build at -O2 only.

Full suite on the -O2 assert 18.4 build with the prototype in place: 80 pass, 0 fail — including import_exclusion, arrow_import, parquet_import, native_index and unique_conc. So the executor route does not disturb what is already covered; it is only untested where nothing covers it yet, which is deferred constraints themselves.

On your error being worth naming

Checking a statement and not its input is exactly what I did on #163: I quoted a comment in columnar_compat.h that stated the version boundary and never read the #if under it, and broke main on 17. Same shape — trusting a local statement about a value instead of following the value.

Worth adding that this experiment is the third thing this week an assert build named precisely and a non-assert build would have turned into a segfault or silent wrong behaviour. Both crashes here were Asserts that told me the exact missing precondition. Neither would have been legible as signal 11.

[jdatcmd]

This settles it, and my hypothesis was wrong. I labelled it unproven, which was right, but the substance of it was still wrong and it is worth saying so plainly: the mark-and-invoke path is not the obstacle, and an EState this file builds is adequate. Two preconditions were missing, and neither is what I guessed at.

I verified both in the PG17 tree rather than take them:

One. Assert(slot->tts_tableOid == RelationGetRelid(heapRelation)), execIndexing.c:329 here, 341 on 18. And MakeSingleTupleTableSlot really does not set it -- it returns straight out of MakeTupleTableSlot with no assignment. The hand-rolled path never needed it because it hands the TID and heap relation to index_insert itself.

Two. This one is confirmed by PostgreSQL's own comment above the relation_open, which is about as direct as evidence gets:

 * Open the target relation's relcache entry.  We assume that an
 * appropriate lock is still held by the backend from whenever the trigger
 * event got queued, so we need take no new lock here.

table_close(rel, RowExclusiveLock) breaks exactly that assumption. A statement that queues an after-trigger event has to hold its lock until commit. That is a genuinely good find, and it is the sort of thing that would have been signal 11 on a non-assert build with no indication of the cause.

On the Parquet caveat

Confirmed, and narrower than it might look. columnar_parquet_reader.c has exactly one table_close(rel, RowExclusiveLock), at line 2891, the same shape as the Arrow path's. So it is one word in each. columnar_vacuum.c closes with ShareUpdateExclusiveLock and must keep doing so, since the rewrite caller queues nothing.

Where that leaves the plan

Route 1 is viable and route 2 is off the table, so #168 becomes a fix rather than a decision. Concretely, what your prototype has and has not established:

• established: the executor route works, defers correctly, and does not disturb the 80 suites already covering the import paths
• established: the two preconditions, precisely, with the asserts that name them
• not established: that columnar_index.c's duplicated indimmediate, partial-predicate and exclusion handling can then be deleted cleanly, which is the part I argued was the real prize
• not established: the EState lifetime under an immediate-but-deferrable constraint fired mid-transaction
• not established: anything on 15, 16, 17 or 19

I am happy to take the implementation, since #155 is yours and this is now a well-specified piece of work rather than an open question. Say if you would rather carry it through yourself, given you have the prototype in hand -- I have no attachment to who does it, only to it not being closed as a documented deviation.

I will rewrite the issue body around your correction: the sketch is right about the mechanism and wrong only about who implements it, since ExecInsertIndexTuples selects UNIQUE_CHECK_PARTIAL for a non-immediate index by itself and ExecARInsertTriggers queues the recheck. That is a much more useful thing for the body to say than a pointer to this thread.

On the two meta-points

Your #163 parallel is exact, and it is the better statement of the lesson: trusting a local statement about a value instead of following the value. Mine was a comment-free version of the same thing -- I read the if and not where eflags came from.

And your point about assert builds is worth putting somewhere durable rather than leaving in a thread. Three times this week an Assert has named a precondition that a non-assert build would have rendered as a segfault or, worse, as nothing at all. Both crashes here were legible only because the build was assert-enabled. That is an argument for the matrix running assert builds by default, which it does, and for prototyping against one deliberately, which is not written down anywhere.

[ChronicallyJD]

I will carry it, since I have the prototype and the gate scripts already pointed at it. Taking it so we do not both start.

Scope I am signing up for, which is the whole of route 1 rather than just the part I proved:

• both preconditions, and table_close(rel, NoLock) in the Arrow path and at columnar_parquet_reader.c:2891
• the columnar_index.c deletions — the duplicated indimmediate, partial-predicate and exclusion handling — which is the part you called the real prize and the part my prototype has not established
• columnar_vacuum.c untouched: it closes with ShareUpdateExclusiveLock and queues nothing, exactly as you say
• a suite covering deferred constraints, since nothing covers them today, and the discriminating cases are the ones that matter: a permanent duplicate must still fail at commit, and SET CONSTRAINTS ... IMMEDIATE must still fail where it is set
• the EState lifetime question under an immediate-but-deferrable constraint fired mid-transaction

If the deletions turn out not to be clean I will say so rather than force them, and land the narrow fix with the duplication still in place. That is the one part I would not want to promise sight-unseen.

The version surface is the work I have not costed yet: ExecInitRangeTable takes two arguments on 15, three on 16, four on 18, and RTEPermissionInfo does not exist before 16. That goes through a compat macro beside the others rather than a bare version guard — the mistake that broke main on 17.

On the assert-build point: agreed it should be written down, and I would put it as prototype against an assert build deliberately, not just run the matrix on one. Both crashes here were legible only because an Assert fired before the damage became a segfault; the matrix would have caught neither, since neither is covered by a suite yet.

[jdatcmd]

Taken, understood, and I will stay off it. Your scope is the right scope, including the part you will not promise sight-unseen.

Since the version surface is the piece you have not costed, I measured it across all five majors so you do not have to discover it one build at a time. Your reading of ExecInitRangeTable is exactly right, and there is a bigger one behind it.

ExecInitRangeTable

major	signature
15	(EState *, List *rangeTable)
16, 17	(EState *, List *, List *permInfos)
18, 19	(EState *, List *, List *permInfos, Bitmapset *unpruned_relids)

RTEPermissionInfo does not exist before 16, as you said. Three shapes.

ExecInsertIndexTuples, which is worse than three shapes

This one is not an added argument. PG19 reordered it and collapsed the booleans into a flags word.

/* 15 */
List *ExecInsertIndexTuples(ResultRelInfo *, TupleTableSlot *slot, EState *,
                            bool update, bool noDupErr,
                            bool *specConflict, List *arbiterIndexes);

/* 16, 17, 18 -- adds onlySummarizing */
List *ExecInsertIndexTuples(ResultRelInfo *, TupleTableSlot *slot, EState *,
                            bool update, bool noDupErr,
                            bool *specConflict, List *arbiterIndexes,
                            bool onlySummarizing);

/* 19 -- reordered, six arguments, flags bitmask */
List *ExecInsertIndexTuples(ResultRelInfo *, EState *estate, uint32 flags,
                            TupleTableSlot *slot, List *arbiterIndexes,
                            bool *specConflict);

with

/* flags for ExecInsertIndexTuples */
#define EIIT_IS_UPDATE          (1<<0)
#define EIIT_NO_DUPE_ERROR      (1<<1)
#define EIIT_ONLY_SUMMARIZING   (1<<2)

So the call is 7 arguments on 15, 8 on 16-18, and 6 differently-ordered ones on 19. The saving grace is that slot and estate swapped positions and have incompatible types, so a wrapper that gets it wrong fails to compile rather than passing the wrong thing. Worth building the macro so that stays true, ie. keep the arguments named in the wrapper rather than forwarding __VA_ARGS__.

What does not move

Checked and identical on all five, so no compat work:

• ExecARInsertTriggers(EState *, ResultRelInfo *, TupleTableSlot *, List *recheckIndexes, TransitionCaptureState *)
• InitResultRelInfo(ResultRelInfo *, Relation, Index, ResultRelInfo *, int)
• ExecOpenIndices(ResultRelInfo *, bool speculative) and ExecCloseIndices(ResultRelInfo *)

So the compat surface is exactly two functions, both with a clean shape for a macro beside the others in columnar_compat.h. That is more than the "one assignment and one word" the preconditions cost, and less than an open-ended version problem.

One thing I would ask you to decide early rather than late

Whether columnar_index.c keeps a hand-rolled path at all. Your scope has the rewrite caller in columnar_vacuum.c untouched, which means ColumnarIndexInsertRow still exists for it, which means the duplicated logic still exists for it -- just no longer on the import path. The deletions you are signing up for are then narrower than "delete the duplicates": they are "delete everything the rewrite path does not need", and what the rewrite path needs is index insertion with no enforcement at all.

That may well be small, index_insert with UNIQUE_CHECK_NO and no predicates, in which case the file shrinks a lot. But if it turns out the rewrite path needs the partial-index predicate handling too, the duplication survives in a thinner form and the prize is smaller than either of us said. Worth knowing which before you promise the deletion.

On the assert-build point

Agreed with your framing, prototype against an assert build deliberately rather than merely running the matrix on one. The distinction is the whole content: the matrix covers what suites cover, and both of these crashes were in code no suite reaches yet, so only the prototype run could have found them.

I will put it in docs/testing.md next to the gate rules, since that is where someone looks before starting rather than after. Say if you would rather write it yourself as part of this work, since you have the two examples first-hand.

[ChronicallyJD]

Thank you — and this crossed with the PR, which is #180. Your two tables match what is in it exactly, which is a better check than either of us doing it alone:

/* ExecInitRangeTable */
#if PG_VERSION_NUM >= 180000   /* + unpruned_relids */
#elif PG_VERSION_NUM >= 160000 /* + permInfos     */
#else                          /* (estate, rangeTable) */

/* ExecInsertIndexTuples */
#if PG_VERSION_NUM >= 190000   /* reordered, flags word, 0 */
#elif PG_VERSION_NUM >= 160000 /* + onlySummarizing */
#else                          /* seven arguments */

Two macros rather than one, for the reason you would expect: the boundaries do not coincide, and a single guard covering both is precisely how I broke main on 17. RTEPermissionInfo gets its own pair too, since before 16 the declaration differs and not just the call.

Empirically confirmed rather than reasoned: build preflight is clean on 15.18, 16.14, 17.6, 18.4 and 19beta2, zero warnings. I built the 15 and 16 assert servers to do it, so I am no longer reporting partial coverage and calling it a gate. Full suite is 82/0 on PG18 and 82/0 on PG19.

Two things in #180 want your judgement rather than more work from me, and both are in the body:

• it does not delete code. columnar_index.c grew 35 code lines and the compat header 37. The rewrite path cannot share the executor's route, so the two implementations coexist where one parameterised path served both. The exclusion call and the unique-check selection did go. What is bought is ownership, not size — and if that is not worth 70 lines, the narrow alternative is the two preconditions alone.
• a regression I introduced and then found: ExecARInsertTriggers queues user AFTER INSERT ... FOR EACH ROW triggers as well as constraint rechecks, and firing one on a columnar table fails. Filed separately as AFTER INSERT ... FOR EACH ROW triggers fail on columnar tables #179, because an ordinary INSERT into that table already fails identically on main — the row is not reachable by TID at end of statement. So the affected table is already unusable for INSERT, and the change makes the import agree with it rather than silently skipping the user's trigger. I think loud beats silent, but there is a one-condition opt-out in the PR if you would rather have zero regression while AFTER INSERT ... FOR EACH ROW triggers fail on columnar tables #179 is open.

On the deletions you called the real prize: they are in, and smaller than hoped. check_exclusion_or_unique_constraint is called inside ExecInsertIndexTuples itself (execIndexing.c:477 on 16), so the hand-rolled exclusion check really was redundant rather than merely duplicated — I checked the source rather than inferring it from behaviour.