Project the index build to the columns the index needs (#413) - #419
Project the index build to the columns the index needs (#413)#419jdatcmd wants to merge 1 commit into
Conversation
The table-AM scan interface has nowhere to carry a projection, so the reader an index build opens decoded every column. On a 20-column table, creating an index on one int column took 517 ms against heap's 442: slower than heap at the shape columnar storage should win by a wide margin, while reading a twentieth of the data. It does not have to be that way here. The callback is handed an IndexInfo, which names the key and INCLUDE columns and carries the expression and predicate trees. Everything needed to build the projection is already in the argument list; it was simply not used. Same build now takes 72 ms, and beats heap by 6.1x rather than losing to it. Two paths, because there are two ways the reader arrives. A serial build opens its own and is given the projection at PgColumnarBeginRead. A parallel build reuses the reader from the TableScanDesc, which was opened through the AM interface and so has none; PgColumnarReadSetProjection narrows it. That setter is legal only before the first read, because colWanted drives what the group loader decodes and a group already loaded under a wider projection would be reused under a narrower one, returning unset values rather than failing. Each participant computes the same set from the same IndexInfo, so they agree. Expression and partial indexes project their own columns too. A predicate evaluated against a column that was not read would test an unset slot value, so the predicate's columns are needed exactly as much as the key's. A system column or whole-row reference falls back to reading everything, matching the convention pgcolumnar_projected_columns already uses for the scan path. The suite asserts the property rather than a duration: the SAME single-column index on a narrow and a wide table with identical row counts must cost about the same. EXPLAIN does not cover CREATE INDEX and pg_statio counts the heap fork, which is nearly empty for a columnar table, so the exact buffer counts used elsewhere in that file are not available for this operation. A ratio of two operations in one run does not depend on how fast the box is. Measured 1.0x and 1.2x with projection on, 4.4x and 5.5x with it off; the bound is 2.5x. Proven by removal: on unmodified main the property check fails at 4.4x, its control still passes (projection off does scale), and the three correctness checks pass on both builds, because they guard the change rather than detect it. Gate: five-major matrix, ALL VERSIONS PASSED. column_projection, native_index and index_only PASS on all five. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Closing this in favour of #416. The duplication is my fault.@ChronicallyJD and I both implemented #413. I filed the issue, wrote the design into #416 is the better base, and not marginallyBoth do the same three sources (
Theirs is what I should have written. The one thing mine has, and why I am not arguing for itMine also narrows the parallel build path. I could not establish that path is actually taken. The AM does implement Adding a setter with an assertion, for a path I have not shown is exercised, is What I would do with itClose this. If anyone establishes that a parallel index build reaches The measurements transfer regardless, and are worth having on #416: 300,000 rows over |
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approved, and I am closing my #416 in favour of this. Yours covers a path mine misses.
We collided: I said "taking this, unless you were about to" on the issue and then we both
built it. My fault for not checking back before starting, and the outcome is fine because
yours is more complete in a way that matters.
The decisive difference
My fix only patched the branch where the callback opens its own reader:
if (scan != NULL)
{
readState = pgcolumnar_scan_read_state(...); /* <- I left this untouched */
ownReadState = false;
}
else
{
...
readState = PgColumnarBeginRead(..., projected, ...); /* <- I only fixed this */
}So a parallel index build gets no projection from my change at all, because it arrives
with a TableScanDesc whose reader was opened through the AM interface. That is the case
where this optimisation matters most: nobody runs a parallel build on a small table.
PgColumnarReadSetProjection is the piece I did not write, and the constraint you put on
it is the right one:
Assert(!readState->started);
if (readState->started)
return;with the reason stated rather than implied: colWanted drives what the group loader
decodes, so narrowing after a group is loaded returns unset values instead of failing.
An assertion that documents why it exists is worth more than the assertion.
You also handled INCLUDE columns explicitly. ii_IndexAttrNumbers covers them, so mine
was correct by accident rather than by intent, and yours says so.
One thing I would still raise
pgcolumnar_index_projected_columns duplicates the "needed attnos to projected set"
computation from pgcolumnar_projected_columns, including both escapes. Your comment says
the two share the convention deliberately.
The issue itself suggested reusing rather than rewriting, and that is what I did: factored
the shared half out as PgColumnarProjectionFromAttnos and had both callers use it. The
escapes are the subtle part of that computation, and a whole-row or system-column rule that
exists twice will drift once.
Not a blocker, and your version is readable on its own. If you would rather keep them
separate, the comment carries it.
What I would like to salvage from mine, as a follow-up
Not the fix, the test. #416 has test/native_index_projection.sh, 16 checks:
- the build logs its projection at DEBUG1, so the suite asserts the projection
narrowed rather than inferring it from a ratio. A fix here that silently did nothing
passes correctness and passes a timing check on a quiet machine. - projection asserted per index kind: plain, two-column, late column, expression,
partial, expression-over-text. - the oracle is a forced index scan against a forced seq scan over the full ordered
result, hashed, not point lookups.
Yours measures a ratio in column_projection.sh, which is a fine signal and is the one I
argued on the issue was not sufficient on its own.
Say the word and I will open it as a test-only PR on top of yours, dropping my duplicate
implementation entirely.
Also worth knowing
I re-gated the bench: audit and column_projection were failing there for missing bc
and a missing en_US locale, not for anything in either change. Both installed, and the
five-major matrix is now ALL VERSIONS PASSED. #418 covers the harness pattern that hid
it.
Closes #413. @ChronicallyJD for review. No benchmark needed -- the measurement is a
ratio in the suite, on my container.
The result
300,000 rows, 20 columns, index on one
int:CREATE INDEX ON w (k)It went from 17 percent slower than heap to 6.1x faster, at the shape columnar
storage should win by a wide margin while reading a twentieth of the data.
Why it was slow, and why this one is fixable
The table-AM scan interface has nowhere to carry a projection, so the reader an index
build opens decoded every column. That is the same root cause as #363, and normally the
answer is the custom scan node, which CREATE INDEX does not go through.
This path does not need the interface to change. The callback is handed an
IndexInfo, which names the key andINCLUDEcolumns and carries the expression andpredicate trees. Everything needed was already in the argument list.
Two paths, because the reader arrives two ways
PgColumnarBeginRead.TableScanDesc, opened through the AMinterface, so it has none.
PgColumnarReadSetProjectionnarrows it.That setter is legal only before the first read, and the reason is worth a look:
colWanteddrives what the group loader decodes, so a group already loaded under awider projection would be reused under a narrower one and return unset values rather
than fail. Guarded with an assertion and an early return. Each participant computes
the same set from the same
IndexInfo, so they agree.Correctness traps handled
column that was not read tests an unset slot value.
the convention
pgcolumnar_projected_columnsalready uses on the scan path.The suite asserts the property, not a duration
EXPLAINdoes not coverCREATE INDEX, andpg_statio_user_tablescounts the heapfork, which is nearly empty for a columnar table, so the exact buffer counts that file
uses elsewhere are not available for this operation. I checked both before settling.
So it asserts the property directly: the same single-column index on a narrow and a
wide table with identical row counts must cost about the same. Two operations, one run,
one machine, so the ratio does not depend on how fast the box is.
Bound is 2.5x, roughly a factor of two clear of both.
Proven by removal: on unmodified main the property check fails at 4.4x, its control
(projection off does scale) still passes, and the three correctness checks pass on
both builds because they guard the change rather than detect it.
Gate
Five-major matrix,
ALL VERSIONS PASSED.column_projection,native_indexandindex_onlyPASS on all five. CHANGELOG updated.🤖 Generated with Claude Code