Maintain indexes when importing Arrow and Parquet

[jdatcmd]

Closes #153.

import_arrow and import_parquet insert rows with table_tuple_insert, which writes the row and nothing else: index maintenance belongs to the executor, and there is no executor on this path. The imported rows landed in the table and in no index, so an index scan returned nothing while a sequential scan returned everything, and a unique index accepted the same key twice. Neither raised.

The fix

The rewrite path in columnar_vacuum.c already had this machinery, because it is in the same position: it inserts rows without an executor and maintains indexes itself. Rather than copy it, it moves to src/columnar_index.c and both callers use it.

The one behavioural difference is uniqueness, and it is a parameter rather than a second implementation. A rewrite moves rows that already satisfied every constraint, and the row it replaces is still visible while it does, so checking uniqueness there would conflict with the row being replaced: it passes UNIQUE_CHECK_NO. An import inserts genuinely new rows, so it passes UNIQUE_CHECK_YES and a duplicate raises. Partial indexes keep their predicate test, and an index that is not yet indisready/indisvalid is skipped, since the concurrent build that owns it will cover these rows itself.

A table with no indexes does no extra work: the importers skip the machinery rather than opening an empty index list per row.

Tests, and two ways the first version of them was wrong

arrow_import and parquet_import now import into a plain index, a unique index and a partial index. Both mistakes are worth recording because the first would have shipped green:

enable_seqscan = off does not discourage a custom scan. With it off, the columnar path was still cheapest and the index was never consulted, so both index checks passed against a build with this fix deliberately removed. pgcolumnar.enable_custom_scan = off is what takes the columnar path out of the running, and the suite now also asserts the plan really is an index scan, so a future costing change cannot quietly defeat the check again.

The q helper echoes one line per statement, so a count preceded by SET statements was compared against the string SET.

With those corrected:

	with the fix	with index maintenance stripped
arrow_import	PASS	5 failures
parquet_import	PASS	5 failures

The failures are the right ones: both scans agree: got [0] want [100], a point lookup finds its row: got [0] want [1], importing the same keys twice raises unique_violation: got [succeeded] want [error], and the failed second import left the row count alone: got [6000] want [3000].

Gate

PG18 and PG19, full suite, ALL VERSIONS PASSED, with arrow_import, parquet_import, and the three suites that cover the moved helper (native_rewrite, native_recluster, native_reclaim) all green.

Note on the alternative

#153 offered refusing to import into an indexed table as the cheaper option. I went with maintaining the indexes because refusing is a behaviour change for anyone whose call currently succeeds, and because an access method that leaves indexes disagreeing with the table has no correct state to be in. Import into an indexed table is now slower, which is a real cost and the right one to pay; #155 covers the throughput work separately.

[ChronicallyJD]

The right call on the alternative, and the right reason: an access method that leaves indexes disagreeing with the table has no correct state to be in. Extracting the rewrite path's machinery rather than copying it is also right, and making uniqueness a parameter rather than a second implementation is the part I would have got wrong.

Two findings. The first is the same class of defect as #153 itself, and by the argument this PR makes for itself, in scope.

1. Exclusion constraints are still silently bypassed

ColumnarIndexInsertRow decides on indisunique and calls index_insert. An exclusion constraint is not a unique index — it is enforced by check_exclusion_constraint after the entry goes in, which the executor calls and this does not. So the entry is inserted and the constraint is never evaluated.

Measured on ecb352a, PG18, btree_gist installed:

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

So after this PR an import can still put a table into a state that ordinary DML would have refused, which is the shape of #153 rather than a separate concern. It is narrower — exclusion constraints on a columnar table are rarer than unique ones — but it is the same silent divergence, and it is now the only one left.

check_exclusion_constraint is exported from executor.h, and the state you already build has everything it wants: the index relation, the IndexInfo, the EState and the TID. The shape would be roughly what ExecInsertIndexTuples does — after index_insert, if indexInfo->ii_ExclusionOps != NULL, call it. If you would rather not, refusing to import into a table with an exclusion constraint is defensible and is a one-line check in ColumnarIndexInsertBegin; what I do not think is defensible is leaving it silently unenforced now that unique is enforced, because the asymmetry is invisible to the user.

2. A deferrable unique constraint is enforced immediately

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

The executor also consults indimmediate:

else if (indexRelation->rd_index->indimmediate)
    checkUnique = UNIQUE_CHECK_YES;
else
    checkUnique = UNIQUE_CHECK_PARTIAL;

so UNIQUE ... DEFERRABLE INITIALLY DEFERRED is checked at commit, not at insert. With this PR it raises at import time. Measured, same three rows containing one duplicate key, the duplicate removed before commit:

heap, deferred unique:      commits, 2 rows
columnar, import_arrow:     ERROR: duplicate key value violates unique constraint "c_k"
                            0 rows

The heap transaction is legal SQL and this rejects it.

I want to be straight that the honest fix here is harder than for the first finding. UNIQUE_CHECK_PARTIAL is only half of it: when index_insert returns false the executor queues a deferred recheck, and doing that outside the executor is real work. Simply passing UNIQUE_CHECK_PARTIAL and ignoring the result would be worse than what you have now, because the duplicate would then never be caught at all. So the proportionate answer is probably to detect a non-immediate unique index in ColumnarIndexInsertBegin and raise a clear error there — "cannot import into a table with a deferrable unique constraint" — rather than to raise a confusing one mid-import. Documenting it is a defensible third option; leaving it to surface as a spurious duplicate-key error is the one I would avoid.

Verified correct

• Expression indexes work. FormIndexDatum needs econtext->ecxt_scantuple, and it is set before the loop. This is the piece hand-rolled index maintenance usually gets wrong, and it is right here.
• Partial indexes. The predicate is prepared once in Begin rather than per row, and rows outside it are skipped rather than inserted with a null key.
• indisready/indisvalid skipping, with the correct reasoning: a concurrent build owns the rows it covers.
• Memory. ResetPerTupleExprContext runs after index_insert rather than before, so expression results are still live when the entry is formed. The slot lives outside the per-tuple context, so the reset does not pull the values out from under it.
• UNIQUE_CHECK_NO for the rewrite is right for the reason given: the row being replaced is still visible.
• The no-index case genuinely skips the machinery rather than opening an empty list per row.

On the two test mistakes you recorded

The enable_seqscan = off one is worth more than a note in a PR body, because it generalises: for this AM, the switch that takes the fast path out of the running is pgcolumnar.enable_custom_scan, not enable_seqscan. Any future test that means to force an index scan has the same trap waiting, and the failure mode is the worst kind — the check passes against a build with the fix removed. Asserting the plan shape as well as the result is the right belt-and-braces and I would suggest it as the house rule for index-path tests.

I hit the sibling of your second one on my own suites this week: q echoes a line per statement, so anything with a leading SET needs tail -1. Two of us tripping on it in a week suggests it deserves a line in lib.sh's header.

Verdict

The fix is right, the extraction is right, and the tests discriminate — you proved that by removal rather than asserting it, which is the standard this project has held me to twice.

Not blocking, but I would like the exclusion-constraint case addressed before this closes #153, because #153 is about imports leaving the table disagreeing with its indexes, and after this PR that is still true for one kind of index. Enforcing it, or refusing to import, would both close it; silence would not.

[ChronicallyJD]

Re-verified on an assert-enabled build, because my review above was done on the packaged non-assert server and that is exactly the gap that made #159 unmergeable. Nothing changed in the PR since; this is me re-testing my own review.

No new findings. The index-maintenance code is clean under assertions:

arrow_import      rc=0, 0 failures
parquet_import    rc=0, 0 failures
native_rewrite    rc=0, 0 failures
native_recluster  rc=0, 0 failures
native_reclaim    rc=0, 0 failures

That is worth stating positively: hand-rolled index maintenance builds an EState, a slot and per-tuple contexts by hand, which is the kind of code assertions usually have something to say about, and they do not.

Both findings from my review reproduce on the assert build, so neither was an artifact of the non-assert server:

Exclusion constraints, still silently bypassed:

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

Deferrable unique, still enforced immediately:

heap, DEFERRABLE INITIALLY DEFERRED:   commits, 2 rows
columnar, import_arrow:                ERROR: duplicate key value violates unique constraint

My position is unchanged and I would rather restate it plainly than leave it implied: the fix is right and I would merge it, but the exclusion-constraint case should be closed before this closes #153, since the issue is about imports leaving the table disagreeing with its indexes and after this that is still true for one kind of index. Enforcing it via check_exclusion_constraint, or refusing to import into a table that has one, both close it. Silence does not, and the asymmetry with unique — now enforced — is invisible to the user.

The deferrable case I would be content to see documented rather than fixed, for the reason in my review: UNIQUE_CHECK_PARTIAL without the deferred recheck would be worse than what you have.

[ChronicallyJD]

Recording a verdict, since my earlier review went in as a comment and so registered no decision — that was my mistake, and if you were waiting on a sign-off, this is it.

Approving. The fix is right, the extraction of the rewrite path's machinery is right, uniqueness as a parameter rather than a second implementation is the part I would have got wrong, and the tests discriminate — proven by removal rather than asserted.

Since that review I re-verified the whole thing on an assert-enabled build, because my original testing used the packaged non-assert server and that is exactly the gap that made my #159 unmergeable. No new findings: arrow_import, parquet_import, native_rewrite, native_recluster and native_reclaim all clean under assertions. Worth saying positively — hand-rolled index maintenance builds an EState, a slot and per-tuple contexts by hand, which is the kind of code assertions usually have something to say about.

The two findings from my review both reproduce on the assert build, so neither was an artifact:

• Exclusion constraints are still silently bypassed. import_arrow inserts two rows that violate one, no error, where an ordinary INSERT of the same violation raises. This does not block the merge, but I would not have it close #153: that issue is about imports leaving the table disagreeing with its indexes, and after this that is still true for one kind of index — now the only one, which makes the asymmetry invisible to a user. check_exclusion_constraint is exported and the state you already build has everything it wants; refusing to import into such a table is an equally good answer.
• A deferrable unique constraint is enforced immediately where heap defers to commit. Content to see this documented rather than fixed, for the reason in my review: UNIQUE_CHECK_PARTIAL without the deferred recheck would be worse than what you have.

Neither needs a re-review from me.

[ChronicallyJD]

Third pass, going after the one thing I had taken on trust: the condition deciding which indexes get maintained. It is wrong, but measurement downgraded it from what I first thought, and I would rather show that than report the version I started with.

The skip condition does not match the executor's

if (!irel->rd_index->indisready || !irel->rd_index->indisvalid)
    continue;

with the reasoning that "an index that is not ready or not valid is one a concurrent build has not finished with. That build is responsible for the rows it covers."

The executor's rule is indisready alone. BuildIndexInfo in catalog/index.c passes indexStruct->indisready as the isready argument to makeIndexInfo, and ExecInsertIndexTuples skips on !indexInfo->ii_ReadyForInserts and never consults indisvalid.

That difference is the whole point of indisready. CREATE INDEX CONCURRENTLY sets it before its second scan precisely so that concurrent writers maintain the index while the build runs — the builder is explicitly not responsible for rows written after it started. So for indisready && !indisvalid, the comment's reasoning is backwards: skipping is what loses rows, not what avoids double-inserting.

What I expected to find, and did not

I expected an import during a CIC to leave the finished index missing rows, and built the race to show it: a blocking transaction to stall CIC in the window, an import fired inside it, then a comparison of index-scan against seq-scan counts.

The window is real and I hit it —

flags at import time: true/false     (indisready/indisvalid)
imported: 500

— but the index never becomes valid, because CREATE INDEX CONCURRENTLY is not supported on this access method at all:

CIC exit=1  ERROR:  columnar: concurrent index validate is not supported yet
flags after CIC: true/false

columnar_index_validate_scan is COLUMNAR_UNSUPPORTED, on main, untouched by this PR. So the state my finding needs cannot be produced by the path that would normally produce it. Counts agreed (500 against 500) because the invalid index is not usable and the planner falls back to a seq scan.

So this is latent, not live. I am not going to dress it up as the serious bug I thought I had.

Why I would still change it

• It is one condition, || !irel->rd_index->indisvalid, and removing it makes the AM agree with the executor rather than diverge from it for a reason that does not hold.
• The window is reachable, just not usefully: a failed CIC leaves the index indisready = true, indisvalid = false until someone drops or reindexes it, and in that state ordinary INSERT maintains it while an import would not. Nothing reads it, so nothing breaks today.
• It becomes live the moment columnar_index_validate_scan is implemented, and by then the reasoning in the comment will read as considered rather than as an assumption nobody rechecked.
• The comment should say indisready is the executor's test and why, since the next person will otherwise reach for the same intuition I did.

The two findings from my earlier reviews, unchanged

Both reproduce on the assert build:

• Exclusion constraints are silently bypassed. import_arrow inserts two rows that violate one; an ordinary INSERT of the same violation raises. This is the one I would want closed before this closes import_arrow and import_parquet do not maintain indexes: index scans silently return nothing, UNIQUE is not enforced #153, since that issue is about imports leaving the table disagreeing with its indexes, and after this that remains true for one kind — now the only one, which makes it invisible.
• A deferrable unique constraint is enforced immediately where heap defers to commit. Content to see documented rather than fixed.

Verdict

Approval stands. The fix is right and I would merge it. Of the three, exclusion constraints is the one worth doing before #153 closes; the indisvalid condition is a one-line change I would take now while it is cheap and latent rather than later when it is not.

For the record on my own process: I came to this expecting to report an index silently missing rows after a concurrent build, and the reason I am not is that I built the race instead of reasoning about it. The source reading was correct and the conclusion I drew from it was too strong.