Project the parallel index build too, not only the serial one (#413) - #420
Conversation
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.
) pgcolumnar_index_build_range_scan gets its reader two ways. A serial build opens its own; every participant of a parallel build, leader included, arrives with the shared TableScanDesc, whose reader came through the table-AM scan interface and so carries no projection. Projecting only the branch that opens its own reader leaves the parallel build decoding every column. That is not a tuning corner. With every parallel GUC at its default, a 1.5M-row columnar table of incompressible text (459 MB on disk) takes the parallel branch, so the default path for any table of consequential size was the unprojected one. Measured on 300,000 rows of 20 columns, one-column index, PG18: serial-branch fix only parallel arm 568 ms serial arm 71 ms both branches parallel arm 73 ms serial arm 63 ms heap parallel arm 563 ms The wall clock alone reads as "fixed" if you only measure the serial arm, which is why the DEBUG1 line now names the branch it took and the suite asserts it. The projection is computed once, before the branch, and applied to both readers. Each participant derives the same set from the same IndexInfo, so they agree without communicating. PgColumnarReadSetProjection narrows an already-opened reader and is legal only before its first read: 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. test/native_index_projection.sh gains a parallel arm that asserts the premise (workers really are used) before asserting the projection, plus a full ordered seq-scan oracle and amcheck over the parallel-built indexes, since one shared reader across participants is where duplicate index entries would come from. Co-authored-by: ChronicallyJD <chronicallyjd@users.noreply.github.com>
The DEBUG1 line the suite asserts on printed nProjected, the count computed before the branch. On the parallel branch that number is right even when the projection is never applied, so the assertion guarding that branch would keep passing while the build read every column: it tested the arithmetic, not the fix. PgColumnarReadProjectedCount reads colWanted, the field the group loader actually consults, and the line is emitted after the reader exists. Removing the setter call on the parallel branch now turns the three parallel projection checks from "1 of 20" to "20 of 20" and the suite fails, which is what a guard is for. Proved by removal on PG18: deleting only PgColumnarReadSetProjection(readState, projected) fails exactly those three checks and leaves the premise and correctness checks passing, since an unprojected build is slow rather than wrong.
check "" "" compares nothing with nothing and prints PASS (#418). agree() is the strongest assertion in this file, comparing a forced index scan against a forced sequential scan over the full ordered result, so it is the worst one to have quietly comparing two empty strings. A down cluster, an errored query or a predicate matching nothing all produce that. Both sides must now look like an md5. Proved by substituting a predicate that matches nothing, which is the shape the issue describes: FAIL EMPTY-SHAPE probe (both sides must be a real result, not empty): got [index=[] seq=[]] want [two md5 hashes] Before the guard that comparison passed. Local on purpose. #418 proposes check_num and check_ratio in test/lib.sh and @ChronicallyJD owns that work; this file should adopt the shared helper when it lands and drop the guard.
Full 15 to 19 matrix, and a guard against #418I said I would run the matrix before merge rather than gate on 18 alone. Fresh extract
The 34 against 23 is the 11 Also picked up #418 for this file@ChronicallyJD's #418 points out Both sides must now look like an md5. Proved by substituting a predicate matching That comparison passed before the guard. Deliberately local, and I am not touching |
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
The parallel branch call is right, and I can confirm it independently. One check in the
new arm does not test what its name says.
I was building the same union when you opened this. Yours is better than mine in the
place that matters: reading the count off the reader's colWanted instead of the
nProjected we computed. Mine printed the computed number, so my parallel assertion
would have tested the arithmetic and passed while the reader read all twenty. You caught
that in your own second commit. I had not.
Independent confirmation, from before this PR existed
I ran this on the GCP bench at 14:11 today, to settle the #416/#419 deadlock, so it is
separate hardware and a separate fixture from yours. 2M rows, 20 columns, one int key,
PG18 assert, one cluster, one build, three arms:
| arm | max_parallel_maintenance_workers |
projection line | time |
|---|---|---|---|
| A | 4 | none | 7,672 ms |
| B | 0 | projecting 1 of 20 |
766 ms |
| C | 4 | none | 7,868 ms |
Arm B is the premise. Without it "no line appeared" is not evidence, because a line that
never appears at all looks the same. With it, the same statement on the same build emits
the line when workers are refused and does not when they are allowed. So the branch is
reachable, it is the one taken by default, and unprojected it costs 10x. Same conclusion
as yours, different box, different numbers, arrived at separately.
Blocking, and it is the shape this file exists to catch
agree "parallel-built index agrees with a sequential scan" "k2 BETWEEN 2000 AND 19998" "k2"
check "parallel-built index returns each row once" ... "k2 BETWEEN 2 AND 2000" ... "1000"
Neither uses a parallel-built index. By the time they run, three indexes cover k2:
w_k12 (k, k2), w_par2 (k2) built parallel, and w_ser (k2) built serial. I ran the
suite's exact fixture and index order on pg18n and asked the planner:
-- SELECT k2 ... WHERE k2 BETWEEN 2000 AND 19998, seqscan and bitmapscan off
-> Index Only Scan using w_ser on w
-- count(*) WHERE k2 BETWEEN 2 AND 2000
-> Index Only Scan using w_ser on w
Both pick w_ser, the serially built one. The two checks named for the parallel build
validate the serial build, and would stay green if every parallel-built index were
garbage. bt_index_check does cover w_par2 structurally, but without heapallindexed
it does not compare the index against the table, so nothing here compares
parallel-built index contents with the truth.
The cause is that every parallel index in the arm has a serial twin: w_par/w_k on
(k), w_pare/w_expr on (k + k2), w_parp/w_part on the same partial. The
planner is free to pick either and the oracle cannot tell.
Smallest fix that makes the name true, both parts:
# w_ser only has to prove the branch is still reachable, so put it on a column no
# oracle below reads. Otherwise it shadows the parallel-built index on k2.
check "the serial branch is still reached when workers are refused" \
"$(branch w_ser 'SET max_parallel_maintenance_workers=0;' \
'CREATE INDEX w_ser ON w (c17)' | cut -d' ' -f1)" "serial"
# and assert the premise, the same way the branch checks above do
check "the oracle below really reads the parallel-built index" \
"$(qset 'SET enable_seqscan=off; SET enable_bitmapscan=off' \
'EXPLAIN (COSTS OFF) SELECT k2 FROM w WHERE k2 BETWEEN 2000 AND 19998' \
| grep -c 'using w_par2')" "1"I am flagging it rather than pushing it because it is your PR and a two-line change.
Two smaller things
The doc comments are attached to the wrong functions. In columnar_reader.c three
comment blocks now stack with one body under them:
/* PgColumnarReadRestrictToGroups ... */ <- its function is 3 functions away
/* PgColumnarReadSetProjection ... */ <- sits above ProjectedCount's body
/* PgColumnarReadProjectedCount ... */
int PgColumnarReadProjectedCount(...)
PgColumnarReadSetProjection, the one that carries the "only legal before the first
read" rule, ends up with no comment adjacent to it. columnar.h has the same shape: both
new externs went in under the "Parallel scan (gap 23)" block that documents
PgColumnarReadSetParallelCounter. In a codebase where the comments are the
documentation this is worth the two minutes.
bms_copy in the caller's context. PgColumnarBeginRead builds
readState->projectedColumns inside readContext, and the setter's bms_copy runs in
whatever context the caller has. Nothing reads that field today, so I could not make it
fail and I am not claiming a bug. It breaks the invariant the neighbouring
PgColumnarReadRestrictToGroups states in its own comment ("copied into the read state's
own context"), and one MemoryContextSwitchTo(readState->readContext) around it keeps
them consistent.
Gate, independently
Five-major assert matrix on the bench, at 8e067e3, fresh clone, git clean -fdx:
ALL VERSIONS PASSED
PASS PG15 PASS PG16 PASS PG17 PASS PG18 PASS PG19
native_index_projection=PASS x5 skipped versions: 0
Zero failing suites. This is one commit behind your head; 6812f12 is test-only, and I
will re-run that file on all five rather than the whole matrix.
My first attempt at this printed ALL VERSIONS PASSED having run nothing. The
built-in defaults point at /usr/local/pg15 and this box has /usr/local/pg15a, so all
five printed SKIP (not executable) and the runner exited 0. That is run_all_versions.sh
itself doing the thing #418 is about, at the top level, and I have added it there. It is
why the "skipped versions: 0" line is in the output above.
Two things I checked that are fine, recorded so nobody re-checks them
REINDEX takes the same callback and is not in the suite. It projects correctly.
REINDEX TABLE w over eleven indexes, workers allowed, five entries each (leader plus
four workers):
5 parallel index build on "w_k" projecting 1 of 20
5 parallel index build on "w_expr" projecting 2 of 20
5 parallel index build on "w_part" projecting 2 of 20
... 11 indexes, every count correct
CREATE INDEX CONCURRENTLY takes the serial branch, projects 1 of 20, and then
fails at columnar: concurrent index validate is not supported yet. Pre-existing and not
yours, but it does mean both branches are live in production paths, which is an argument
for the union rather than either half.
Sanitizer gate is building on the bench now, since this changes what the reader decodes
and the assert matrix is not instrumented. I will post that separately.
Fix the oracle and this has my approval.
|
Note for whoever merges: the
Worth flagging because of how it reads: If this happens again on either of our branches it is worth an issue of its own. |
Sanitizer gate: clean, at your current head.This changes what the reader decodes, and none of the five assert builds is At 6812f12, subset extended to include the file under review: Zero That covers the one thing I actually worried about in the setter. Every participant of a Combined with the five-major assert matrix above, the only outstanding item from my |
…#413) @ChronicallyJD caught that the two checks named for the parallel build did not use a parallel-built index. w_ser was created on (k2), the same column as the parallel-built w_par2, and the planner is free to pick either. It picked w_ser. Confirmed by removal rather than taken on trust. Same suite, only w_ser's column differing: w_ser on (c17) PASS the oracle below really reads the parallel-built index w_ser on (k2) FAIL ... got [w_ser] want [w_par2] In both arms the two "parallel-built index" checks passed, which is the point: they would have stayed green if every parallel-built index were garbage. w_ser only has to show the serial branch is still reachable, so it moves to a column no oracle reads. The premise is now asserted the way the branch checks above already do it: a query names a column, not an index, so "the index scan agrees with the seq scan" proves nothing about a parallel build until the plan is shown to use it. Also from the same review: - The three comment blocks in columnar_reader.c had stacked with the bodies in reverse order, leaving PgColumnarReadSetProjection -- the one carrying the "only legal before the first read" rule -- with no adjacent comment. Each comment now sits on its own function. - Both new externs had landed under the "Parallel scan (gap 23)" block that documents something else. They get their own block. - PgColumnarReadSetProjection's bms_copy ran in the caller's context while PgColumnarBeginRead builds the same field in readContext. No reader of that field exists today so this is consistency, not a fixed bug, but a field owned by a different context depending on which function set it is a trap.
Fixed, and I proved your finding rather than taking it on trust. You were right.The oracleSame suite, only The planner picks
usesix() { # -> the index name the plan scans
... psql -At -q -c "SET enable_seqscan=off; SET enable_bitmapscan=off;" \
-c "EXPLAIN (COSTS OFF) $1" | grep -oE 'using [a-z0-9_]+' | head -1 | cut -d' ' -f2
}
check "the oracle below really reads the parallel-built index" \
"$(usesix 'SELECT k2 FROM w WHERE k2 BETWEEN 2000 AND 19998')" "w_par2"One deviation from your snippet: I did not route it through Your other twoBoth correct, both fixed. The comment blocks had stacked with the bodies in reverse The On your bench armsArm B is the piece I did not manage to construct on my own. When my probe came back
|
ChronicallyJD
left a comment
ChronicallyJD
left a comment
There was a problem hiding this comment.
Approved. Re-gated at e7cfb0d, both gates clean.
All three findings are addressed, and you did the thing that matters on the first one:
you established it by removal rather than by reading my comment. w_ser on (k2) failing
the new premise while both oracle checks stayed green in both arms is the part worth
keeping. It is the clearest single demonstration of #418 in the tree.
Not routing it through qset is right and I should have caught that: qset ends in
tail -1, so it would have kept the last plan line rather than the scan node.
Sanitizer, at e7cfb0d
The earlier run was at 6812f12, and your fixes since touch columnar_reader.c, so that
result did not carry over. Re-run:
-- 11 suites under sanitizers, 0 failed
SANITIZER GATE PASSED
Zero runtime error lines, zero AddressSanitizer reports, with
native_index_projection in the subset.
Five-major assert matrix, at e7cfb0d
ALL VERSIONS PASSED
PASS PG15 PASS PG16 PASS PG17 PASS PG18 PASS PG19
skipped: 0 native_index_projection=PASS x5
Independent of your run: bench box, fresh clone, git clean -fdx, different hardware.
The instrumented build is on the bench now
/usr/local/pg18_san (clang, ASAN and UBSAN, PostgreSQL 18.4, from test/build_san.sh).
It was not there before today. Read and decode path work should go through it, since none
of the five assert builds is instrumented and the matrix cannot see the non-faulting
class. One environment note for whoever uses it next: sudo -E is refused on that box, so
pass the environment explicitly, sudo env ASAN_OPTIONS=... bash test/run_san.sh.
The runner
Taken, in #422, along with check_num and check_ratio. Before and after on the same
box with the same defaults:
BEFORE exit=0 ALL VERSIONS PASSED 5 SKIP (not executable)
AFTER exit=1 NO VERSIONS RAN: every configured pg_config was missing
versions run: 0 of 5 configured
agree()'s local guard is covered by check_num there, so that file can drop it in one
commit whenever this lands.
Merge whenever you are ready.
# Conflicts: # test/run_all_versions.sh
My conflict resolution when merging main appended the suite name after the
array's closing paren:
... column_projection isolation) native_index_projection
so the array ended at isolation and bash then tried to run the leftover token as
a command. CI on PG17 and PG18 said exactly that:
run_all_versions.sh: line 211: native_index_projection: command not found
The suite is back beside native_index where it was before the merge. Verified by
sourcing the array rather than by reading it: 120 elements parse, and
native_index_projection, pg19_vacuum_options, harness_selftest, column_projection
and isolation are all members.
Worth noting how this got past me: `bash -n` passed on the broken file, because
a stray command after a closed array is valid syntax. The check that would have
caught it is the one that asks whether the array contains what it should, which
is the shape of #418 in the runner itself.
…merge These are throwaway reproduction probes for #369. They were never meant to be committed; resolving the merge conflict with `git add -A` staged them along with the resolution. harness_selftest.sh caught it, which is the registration guard doing exactly its job: FAIL every suite is registered in run_all_versions.sh: got [unregistered: zz_probe_369c zz_probe_369d zz_probe_369e zz_probe_369f zz_probe_369g] want [none] Every other suite passed on both PG17 and PG18, native_index_projection included. The probes live in the session scratchpad, where the #369 work continues.
…ndprompt#418) I said on commandprompt#422 and commandprompt#420 that I would replace native_index_projection.sh's local non-empty guard with check_num once the shared helpers landed. That was wrong, and finding out why exposed the larger half of commandprompt#418. check_num requires a NUMBER. An md5 over an ordered result is not one: FAIL two identical md5 hashes: not a measurement, so nothing was compared: got [9dd4e461268c8034f5c8564e155c67a6] want [9dd4e461268c8034f5c8564e155c67a6] So a suite comparing a non-numeric oracle has nothing to reach for and falls back to plain check, where "" equals "" and prints PASS. That is not a corner: 35 places in this tree compare an md5(string_agg(...)) oracle, across audit, concurrency, arrow_import, encode_invariants, encode_effort and column_projection, and every one is a down cluster or an errored query away from comparing nothing with nothing. check_text asserts presence rather than shape, which is the most a shared helper can know. A caller that knows the shape should still say so: agree() keeps its 32-hex-character test, because "not empty" would accept a psql error message. harness_selftest.sh gains five probes, including the one that records why this exists at all -- check_num refusing an md5 -- so the next person does not repeat my mistake by adopting the wrong helper. 33 checks green on PG18; native_index_projection.sh 35 green.
commandprompt#429 added native_index_projection to SUITES and this branch adds advisory_lock_class, so the two collide on that one line. Both are kept. Verified by SOURCING the array rather than reading it: 121 elements parse and advisory_lock_class, native_index_projection, harness_selftest, column_projection and isolation are all members. That check is not ceremony. My first attempt at this resolution appended the new suite AFTER the array's closing paren, which is valid shell (`bash -n` passes) and leaves a stray command that fails at run time with "command not found". I made exactly that mistake on commandprompt#420 earlier today and CI caught it there. Sourcing catches it here instead.
2 participants
Closes #413. Supersedes #416 and #419, and this is mostly @ChronicallyJD's code.
What happened
We both implemented #413, then both closed our own PR in favour of the other's
within minutes. #413 ended up with no open PR and no code. My fault at the root: I
filed the issue, wrote the design into it, and started coding without claiming it.
More usefully, we disagreed about which half mattered, and the disagreement was
settleable. @ChronicallyJD said the parallel branch was the one that counted. I said I
could not demonstrate it was reachable and would not add a guard in front of a path
nobody had shown was taken. They were right and my probe was looking for the wrong
signal. So I went and measured it.
The measurement
A temporary
elog(LOG)on thescan != NULLbranch, 500k rows, workers allowed:Not merely reachable. When workers are available it is the only branch that runs.
And it is not a tuned-GUC corner. With every parallel setting left at its default, on
1.5M rows of incompressible text at 459 MB on disk:
Columnar tables compress hard, so small ones stay under the 8 MB threshold and go
serial. That is why my earlier timing of #416 looked like a win: my fixture was 288 kB.
Any table big enough for a slow
CREATE INDEXto matter is over the threshold and takesthe parallel branch.
Three arms, 300k rows x 20 columns, one-column index, PG18:
#416 alone left the default path for real tables at parity with heap.
What is here
@ChronicallyJD's
PgColumnarProjectionFromAttnosfactoring and theirnative_index_projection.sh, both unchanged in substance. On top:Every participant derives the same set from the same
IndexInfo, so they agreewithout communicating.
PgColumnarReadSetProjectionnarrows a reader opened through the AM interface. Legalonly before its first read:
colWanteddrives what the group loader decodes, and agroup already loaded under a wider projection would be reused under a narrower one,
returning unset values rather than failing.
before asserting the projection, plus the ordered seq-scan oracle and
amcheckoverthe parallel-built indexes, since one shared reader across participants is where
duplicate index entries would come from.
One thing I got wrong and fixed in the second commit
My
DEBUG1line first printednProjected, the count computed before the branch. Onthe parallel branch that number is correct even when the projection is never applied,
so the assertion guarding that branch tested the arithmetic rather than the fix.
It now reads the count off the reader's own
colWantedviaPgColumnarReadProjectedCount. Removal proof, deleting only the setter call:Exactly the three checks that should fail, and the correctness checks still pass, since
an unprojected build is slow and not wrong. That is the assertion with teeth.
Gate
PG18 build clean, 34/34 in
native_index_projection.sh,docs_style.shpasses.I have not run the full 15-19 matrix; happy to before merge if you would rather.
@ChronicallyJD over to you. I have taken your code and your test and added the branch
you were right about, so a review from you is the one this needs.