Enforce exclusion constraints when importing (follow-up to #158)

[ChronicallyJD]

Follow-up to #158, fixing the two things I found reviewing it. I flagged both and approved anyway; #158 merged and #153 closed with them still open, which by the rule in #165 makes them defects the project has decided to keep. So here they are fixed rather than restated.

Exclusion constraints were not enforced

index_insert does not enforce an exclusion constraint — it is not a unique index. The executor inserts the entry and then scans for a conflicting one through check_exclusion_constraint. Without that call an import put the entries in the index and left the constraint violated, silently.

Measured on 3b281e5 before the fix:

exclusion constraint on a columnar table:   accepted
import_arrow of 2 rows that violate it:     2        <- no error, both kept
ordinary INSERT of the same violation:      ERROR ... conflicts with existing key

After:

import_arrow of 2 rows that violate it:     ERROR ... conflicts with existing key
rows left behind:                           0

That is exactly the shape of #153 — an import reaching a state ordinary DML refuses — for the one kind of index that was left out, which is what made it invisible once unique was covered.

The check runs only when the caller enforces constraints. A rewrite moves rows that already satisfied this one while the row being replaced is still visible, so a check there would find the row against itself. The suite covers that by rewriting the same table afterwards, because it is the way this fix could plausibly break something that works.

The skip test now matches the executor's

-if (!irel->rd_index->indisready || !irel->rd_index->indisvalid)
+if (!irel->rd_index->indisready)

BuildIndexInfo passes indexStruct->indisready as isready, and ExecInsertIndexTuples skips on !ii_ReadyForInserts and never consults indisvalid. That difference is the point of indisready: CREATE INDEX CONCURRENTLY sets it before the second scan so concurrent writers maintain the index, because the builder is explicitly not responsible for rows written after it started.

This one is latent, and I want to be straight about that — I went looking for an index silently missing rows after a concurrent build and could not produce one, because columnar_index_validate_scan is COLUMNAR_UNSUPPORTED and CIC fails before the index becomes valid. It is reachable only as debris: a failed concurrent build leaves the index ready-but-invalid, and in that window ordinary INSERT maintains it while an import would not. Nothing reads an invalid index, so nothing breaks today.

It is one condition, it makes the AM agree with the executor rather than diverge on reasoning that does not hold, and it becomes live the moment index_validate_scan is implemented.

Deferrable unique, deliberately not fixed

Still enforced immediately, where the executor defers to commit. Matching that needs UNIQUE_CHECK_PARTIAL plus a queued recheck through the after-trigger machinery, which is a genuinely separate piece of work.

I considered refusing such imports outright and rejected it: today an import into a table with a deferrable unique constraint and no duplicates succeeds correctly, and refusing would break that. UNIQUE_CHECK_NO would be worse still — real duplicates would slip through silently. Enforcing early is over-strict rather than unsound, which is the safe direction to be wrong in, and the file header now says so instead of leaving it unremarked. That is the documented-not-fixed case #165 allows: the fix is a different piece of work, not one I skipped.

Tests

New suite test/import_exclusion.sh, 8 checks, registered in run_all_versions.sh. It skips cleanly with a message where btree_gist is unavailable, rather than passing quietly.

The first thing it asserts is that a clean import still works, because a conflict check that fires when it should not would break every import into a table that merely has an exclusion constraint — a worse defect than the one being fixed.

Proven by mutation. Removing the check_exclusion_constraint call:

FAIL  importing rows that violate the constraint raises
FAIL  and the failed import left the row count alone: got [1000] want [500]
FAIL  and the rows survived it: got [1000] want [500]

The 1000 is the violating import being silently accepted, which is the defect itself.

Gate

Build preflight on 17.6 and 18.4, zero warnings. Full suite on the -O2 assert build: 77 pass, 0 fail.

I have 17 and 18 assert servers only, so I have not run 15, 16 or 19.

[jdatcmd]

The fix is right and I verified it independently. The test does not run on this
machine, which is the one thing to change before it lands.

The defect is real, and it was mine

Confirmed on 3b281e5, using an exclusion constraint that needs no extension:

indisunique | indisexclusion
f           | t

ordinary INSERT of the second conflicting row:  ERROR  conflicting key value violates exclusion constraint
rows after DML:                                 1
import_arrow of both conflicting rows:          2      <- accepted
rows left behind:                               2

On your branch the same import raises and leaves 0 rows. So #158 fixed unique
and left exclusion in exactly the state #153 described: an import reaching a
state ordinary DML refuses, silently. Thank you for going back for it rather
than restating the review comment.

The blocker: the suite reports PASSED having checked nothing

btree_gist is not available here, and the suite skips:

-- btree_gist unavailable; exclusion constraints cannot be built, skipping
checks run: 0
import_exclusion.sh: PASSED

A suite that reports PASSED with zero checks is the same failure mode as one no
gate runs: the matrix goes green and the evidence does not exist. On this box,
which is the box the gate runs on, this PR currently has no test behind it.

It does not need the extension. EXCLUDE USING btree (k WITH =) builds with no
extension at all and is still an exclusion constraint rather than a unique one,
which is the distinction that matters here:

ALTER TABLE ex_t ADD CONSTRAINT ex_t_k EXCLUDE USING btree (k WITH =);
-- indisunique = f, indisexclusion = t

That is what I used above, and it goes through check_exclusion_constraint
exactly as the gist case does. Keep the gist case as an extra when btree_gist
is present if you want the range-overlap shape covered, but the btree case should
be unconditional so the suite has checks everywhere.

While you are in there: a skip should not report PASSED. Either assert the
precondition or emit something the matrix reports distinctly, so a
zero-check run cannot read as a passing one.

The indisvalid change

Correct, and correctly labelled latent. BuildIndexInfo sets
ii_ReadyForInserts from indisready and ExecInsertIndexTuples skips on that
alone, never consulting indisvalid, so the access method now agrees with the
executor instead of diverging on reasoning that does not hold. Saying plainly
that you went looking for a live reproduction and could not produce one, because
index_validate_scan is unsupported and CIC fails before an index becomes valid,
is more useful than a claim would have been.

Deferrable unique

The reasoning is sound and I agree with the choice: over-strict beats unsound,
and UNIQUE_CHECK_NO would let real duplicates through. But a file-header note
is not tracking. #165's rule allows "the fix is a different piece of work"; it
does not allow the difference living only in a comment. Please open an issue for
it, with the UNIQUE_CHECK_PARTIAL plus queued-recheck sketch from your
description, and reference it from that header. Then it is a tracked deviation
rather than a documented one.

Everything else here is ready; this is three lines of test and an issue.

[ChronicallyJD]

All three addressed.

The suite no longer needs an extension

You are right that a suite reporting PASSED with zero checks is the same failure as one no gate runs — and worse here, because I wrote the skip deliberately and it made the change look tested on a machine where it was not.

EXCLUDE USING btree (k WITH =) needs nothing installed and is still an exclusion constraint rather than a unique index, which I verified rather than assumed:

indisunique/indisexclusion:  false/true

Nothing in the file is conditional now, so it cannot report a pass without having checked anything. I also added that flag pair as its own check — if a future edit turned the fixture into a unique index the suite would go green through index_insert's own enforcement and prove nothing about the path it is named for.

I dropped the gist case rather than keeping it as an extra. It was covering the same code path through a second operator class, so it was buying a second measurement of one thing at the cost of the conditional I just removed.

#168 for the deferrable case

Opened, with the measurement and the UNIQUE_CHECK_PARTIAL plus queued-recheck sketch, both shortcuts and why each is worse, and an acceptance check that discriminates between the real fix and the tempting one. The file header now points at it.

You are right that this is the rule applied to me again. #165 allows "the fix is a different piece of work"; it does not allow that difference living only in a comment, and a header note is not something anyone can find. I had read the rule as being about limitations documents.

Gate

Merged current main in — it had moved past this branch and #164's suite was unregistered against my older run_all_versions.sh, which #161's check caught. Both suites registered now.

Build preflight 17.6 and 18.4, zero warnings. Full suite on the -O2 assert build: 78 pass, 0 fail.