Project the index build scan, and pin that it narrowed (#413) - #416
Project the index build scan, and pin that it narrowed (#413)#416ChronicallyJD wants to merge 1 commit into
Conversation
…#413) jdatcmd's diagnosis: pgcolumnar_index_build_range_scan opened its reader with no projection, so building a one-column index on a wide table decoded every column. It never had to. The callback receives IndexInfo, which carries ii_IndexAttrNumbers and the expression and predicate trees, so the columns were in its own arguments and were thrown away. Measured on 300,000 rows of 20 columns, index on the key alone, non-assert PG18: before columnar 1,403 ms heap 149 ms 9.4x slower than heap after columnar 87 ms heap 144 ms 1.65x faster Three sources feed the projection, and missing any of them reads unset slot values rather than merely reading too much: ii_IndexAttrNumbers the key columns, with 0 marking an expression ii_Expressions an expression index references more ii_Predicate a partial index evaluates against more The "needed attnos to projected set" half is factored out of pgcolumnar_projected_columns as PgColumnarProjectionFromAttnos and shared, per the suggestion on the issue. The system-column and whole-row escapes are the subtle part of that computation and should not exist twice. The build logs what it projected at DEBUG1, because the test needs to assert that the projection NARROWED. A fix here that silently did nothing would pass every correctness check and a wall-clock check on a quiet machine, which is the failure mode this project keeps finding in its own suites. test/native_index_projection.sh asserts the projection for plain, two-column, late-column, expression and partial indexes, then checks each against a forced sequential scan over the full ordered result rather than by point lookup. It runs amcheck where the build has contrib and skips visibly where it does not, since source builds have none.
Gate result, as promised before asking for a merge
The matrix as a whole reports Both are missing bench dependencies, not defects
grep -oE '(hit|read)=[0-9]+' | cut -d= -f2 | paste -sd+ | bcThat is why its numbers came out empty rather than wrong: I will install both on the bench and re-gate. Neither belongs in this PR. A correction to what I told you earlier in this PRI wrote that Rerun measuring the exit code, which is what the runner uses, and the picture above is what One thing worth its own issue, and I will file itA suite that silently produces an empty measurement and then fails a comparison is one That is the same shape as three things I hit in my own work today, so I would rather fix Ready to merge once the bench is re-gated with those two packages present, unless you would |
Green. The diagnosis held.Installed Zero failures across the matrix. Both pre-existing failures were missing bench packages, Ready to merge
I filed #418 for the thing this exposed in the harness, separately from this PR. Short |
|
Closing in favour of #419, which covers the parallel index build path that this PR missed entirely: a build arriving with a TableScanDesc gets its reader from the AM interface, and I only patched the branch that opens its own. That is the case where the optimisation matters most. The test suite here is still worth having, and I have offered on #419 to reopen it as a test-only PR on top of theirs. Detail in the review. |
jdatcmd
left a comment
jdatcmd
left a comment
There was a problem hiding this comment.
Approved, and I closed my duplicate (#419) in favour of this
We both implemented #413. That was my fault: I filed the issue, wrote the design
into it, and started coding without claiming it. Sorry for the wasted run on your side.
Yours is the better implementation, and not marginally
Both take the same three sources with the same whole-row and system-column escape. The
difference is factoring, and it goes your way:
PgColumnarProjectionFromAttnos converts pull_varattnos form to the reader's 0-based
set once, shared with the custom scan path. I duplicated that loop into
columnar_tableam.c, leaving two copies of an offset calculation involving
FirstLowInvalidHeapAttributeNumber. That is precisely the kind of duplication that
drifts apart and then disagrees on a corner case. Yours is what I should have written.
Verified independently
Built your branch and ran my own measurement against it:
| before | your branch | heap | |
|---|---|---|---|
CREATE INDEX ON w (k), 300k rows x 20 cols |
517 ms | 57 ms | 421 ms |
| expression index | 115 ms | ||
| partial index | 105 ms |
From 17 percent slower than heap to 7.4x faster. Zero warnings.
One gap, which I am explicitly not asking you to fix
This projects the serial build. index_build_range_scan gets its reader two ways:
a serial build opens its own, and a parallel build reuses the TableScanDesc's
reader, which was opened through the AM interface and so carries no projection.
My version added a PgColumnarReadSetProjection for that. I am not proposing it,
because I could not establish the parallel path is ever taken. The AM does
implement parallelscan_estimate and parallelscan_initialize, so it exists in
principle, but when I tried to observe a parallel index build engaging workers the
heap control produced nothing either -- so my probe is inconclusive rather than a
negative result.
A setter with an assertion, guarding a path nobody has shown is exercised, is
speculative. If someone demonstrates scan != NULL is reached on a columnar table,
it is about twenty lines and I will offer it then with the demonstration attached.
Worth a sentence in the code or the issue either way, so the next person knows the
parallel path is unprojected by design rather than by oversight.
On the test
test/native_index_projection.sh as a new suite is a defensible call. I had put mine
in column_projection.sh, since that file already owns "the reader must read only the
columns a query references" and has the faithful-plus-actually-skips structure. Either
home works; yours is more focused and I am not going to argue for churn.
One thing from my version worth stealing if you like it: I asserted the property
rather than a duration, by building the same single-column index on a narrow and a
wide table with equal row counts and requiring the ratio to stay near 1. Measured 1.0x
and 1.2x with projection on against 4.4x and 5.5x with it off. It avoids a timing
threshold entirely, which matters because EXPLAIN does not cover CREATE INDEX and
pg_statio counts the heap fork, so the exact buffer counts that file uses elsewhere
are unavailable here. Take it or leave it.
Merging.
2 participants
Closes #413. Your diagnosis, your suggested fix, your suggestion to reuse rather than
rewrite. I implemented and tested it.
The measurement
300,000 rows, 20 columns, index on the key alone, non-assert PG18, median of three:
My "before" is much worse than the 517 ms you measured, and heap on my box is about 3x
faster than yours. Different machine, so treat these as a before/after pair from the bench
rather than a correction to your numbers. The direction and the mechanism agree.
The change
Three sources feed the projection, and missing any of them reads unset slot values rather
than merely reading too much:
The "needed attnos to projected set" half is factored out of
pgcolumnar_projected_columnsasPgColumnarProjectionFromAttnosand shared, per yoursuggestion. The system-column and whole-row escapes are the subtle part and should not
exist twice.
The DEBUG1 line, which is the point of the test
The build now logs what it projected. I said on the issue I would add an assertion that
the projection narrowed, not just that the result was correct, and this is what makes
that possible:
A fix here that silently did nothing passes every correctness check and a wall-clock check
on a quiet machine. That is the failure mode this project keeps finding in its own suites,
including twice in mine today.
The suite
The oracle is a forced index scan against a forced seq scan over the full ordered
result, hashed, rather than point lookups. An index built from under-projected data can
satisfy the handful of keys I happened to pick while being wrong elsewhere. It depends on
nothing but PostgreSQL, which is why it is the primary check and amcheck is the bonus.
Two things you should know before reviewing
column_projectionis failing onmainon the bench, and it is not this change. Ichecked, because it failed alongside my suite and I was not going to assume:
It passed in every container matrix I ran earlier today, so it is environment-fragile
rather than broken. Its probe returns empty values, not wrong ones, which is one step
from a probe that returns empty and passes. Worth its own look; I did not want it absorbed
as noise from this PR.
The
auditfailure in my first matrix was the harness, not the code: the suites run asroot and drop to
postgres, which cannot traverse/home/jd. Gating from a world-readabletree fixes it. Zero failures standalone.
Gate
Five-major matrix is re-running on the corrected tree now. I will post the result before
asking you to merge rather than after.