fix: name storage_pkey on both storage_id scans (#1237) - #1241
Conversation
PgColumnarGetSortedInfo and PgColumnarCheckNativeFormatVersion key on
storage_id and both passed InvalidOid, so both swept pgcolumnar.storage
sequentially on the one column storage_pkey is a UNIQUE btree over.
PgColumnarGetSortedInfo planning, via pgcolumnar_sorted_pathkeys
PgColumnarCheckNativeFormatVersion execution, once per relation scanned
Unlike #1210 nothing has to be built and no decision about #1211 is involved:
the index exists and the key is exact.
unfixed count(*) idx=0 seq=1 join idx=0 seq=5 8 passed + 2 failed
fixed count(*) idx=1 seq=0 join idx=4 seq=1 10 passed + 0 failed
The four converted are GetSortedInfo twice at planning and
CheckNativeFormatVersion twice at execution. THE RESIDUE IS
pgcolumnar_written_stripe_row_limit, ONCE PER COLUMNAR RELATION THAT REACHES IT
(#1210, #1211), which keys on relation_oid and has no index to name. One here
because this query's qual is on one side of the join only; @OffgridwithJD
measures two where both sides reach it. `seq` becoming 1 is a property of the
query, not of the change.
Which is why the join arm asserts idx_scan >= 2 rather than seq_scan == 0.
Asserting zero would fail for a defect this change does not fix, and asserting
one would pin a number #1210 is expected to move.
The two shapes are separate arms because they reach different code. A no-qual
count never reaches the row-group-limit lookup, so its only storage access is
the format-version one.
FOUND BY TWO INSTRUMENTS DISAGREEING AND BOTH BEING RIGHT. An elog inside
pgcolumnar_written_stripe_row_limit counted 2 scans for a join; seq_scan on the
catalog counted 4. An elog at one function counts arrivals at that function; a
counter on the relation counts scans from any caller. The gap was the second
reader. A first hypothesis -- that a projection was present in one fixture and
not the other -- was driven with the projection count printed as a premise and
died: four calls with zero projections.
Both harnesses, ten checks each, identical assertion names. The pytest twin got
its own removal proof rather than inheriting the shell one, because the twins'
regimes diverge: it aborts at its first failure where the shell suite runs on,
reporting 8 checks against 10.
Majors from five runs against DEFAULT_CONFIGS: rc=0, 10 passed + 0 failed on
pg15, pg16, pg17, pgsql and pg19.
Census: 1519 -> 1521 by counting, 1606 rows, 85 not never, 1521 + 85 == 1606.
suites_not_covered does not move; catalog_plan_index is already covered.
PgColumnarSetSortedExtent has the same shape on the same key and is NOT included:
it is a write path holding RowExclusiveLock, nothing has measured its cost or
frequency, and the InvalidOid sweep of this file belongs to #1207/#1213.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
APPROVE on bc5ad0c7, 15 of 15 SUCCESS, CLEAN.
Verified in pgcolumnar-audit rather than read off the body:
catalog_plan_index rc=0 10 passed + 0 failed + 0 unrunnable + 0 skipped
storage after count(*) idx_scan=1 seq_scan=0
storage after a two-relation join idx_scan=4 seq_scan=1
harness_selftest PG17 rc=0, PG18 rc=0, 1138 checks, 0 FAILs
docs_style.sh PASSED
ledger 10 checks / 10 rows / 10 pytest asserts, parity rc=0
census 1606 rows, 1521 never, budget agrees
What I attacked
pgcolumnar_index_oid could have traded one scan for another. It does not: it is get_relname_relid(name, pgcolumnar_schema_oid()), a syscache lookup, not a catalog scan. That was the first thing worth checking, because a fix that resolves an index by name on every call can cost more than the scan it removes.
The fallback is fail-safe. systable_beginscan(rel, storIdx, OidIsValid(storIdx), ...) passes the resolved oid and gates indexOK on it, so an unresolvable index degrades to the previous heap scan rather than erroring. Both sites do it the same way.
The count(*) arm is not vacuous. seq_scan == 0 would be trivially satisfiable if nothing touched storage at all, and the premise above it asserts the access happened. Measured here as idx_scan=1, which is the format-version reader -- the one that is reached once per columnar relation scanned at execution and never at planning.
The join arm is measured on idx_scan, and that is the right call
seq_scan=1 on this fixture is not a property of the change. On mine it would be 2: my join's qual reaches the limit lookup on both columnar relations, this one reaches it on one. The invariant across both is the residue is pgcolumnar_written_stripe_row_limit, once per columnar relation that reaches it -- which is #1210 and #1211's territory, not this change's.
So asserting idx_scan >= 2 rather than seq_scan == 0 or seq_scan == 1 is what keeps this arm measuring the change instead of the tree. The comment says so, and it is the distinction I would have wanted argued.
One thing this does not claim, correctly
Whether the execution-side cost matters against actually reading the relation's data. It is unmeasured and the comment says so. The scan is wrong either way -- sequential on an indexed primary key -- and that is enough to fix it without a performance argument attached.
No blockers.
Six mutations over validity_elision, selftest part 190 and native_join_vector_agg. No test changed and no code changed: the arms were attacked and the ledger records what happened. veC the elision predicate forced true 5 arms b190A pgc_build_needs_clean never asks for a clean 5 arms b190B the stamp writes an escaped literal (commandprompt#898) 2 arms b190C pgc_build_needs_clean always demands a clean 2 arms b190D unknown provenance reported as a major, PG? 2 arms jvaB the dim-side uniqueness refusal disabled 2 arms b190A and b190C are opposites and redden disjoint pairs. A guard that never cleans loses the five arms about when a clean is required; one that always cleans loses exactly two, and only those two separate a correct guard from one that is merely fail-closed. Fifteen of the eighteen rows are newly dated. The other three are validity_elision arms already dated under the inverse mutation -- the predicate forced false rather than true -- so they now carry both strings and the later date, rather than the newer observation overwriting the older. Re-derived on the merged tree. All six had run once before commandprompt#1240 and commandprompt#1241 landed and were re-run against ac80762, reproducing their earlier results exactly: same names, not merely the same counts, with a control showing the comparison can separate two unrelated sets. The two C mutations ran with a forced make clean, because PGXS here has no --enable-depend and a stale object after a header edit would leave veC reddening nothing. Census re-derived by counting: 1606 rows, 109 not never, 1497 + 109 == 1606. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MpajdQbkVJ9ey1XyYHcikP
2 participants
Closes #1237. Two readers key on
storage_idand both passedInvalidOid, so both sweptpgcolumnar.storagesequentially on the one columnstorage_pkeyis a UNIQUE btree over.PgColumnarGetSortedInfopgcolumnar_sorted_pathkeysPgColumnarCheckNativeFormatVersioncolumnar_reader.c:644Unlike #1210 nothing has to be built and no decision about #1211 is involved: the index exists and the key is exact.
Red before green, source and binary in step each time
The four converted are
GetSortedInfotwice at planning andCheckNativeFormatVersiontwice at execution.The residue is
pgcolumnar_written_stripe_row_limit, once per columnar relation that reaches it (#1210, #1211), which keys onrelation_oidand has no index to name. One here because this query's qual is on one side of the join only; @OffgridwithJD measures two where both sides reach it.seqbecoming 1 is a property of the query, not of the change.Which is why the join arm asserts
idx_scan >= 2rather thanseq_scan == 0: asserting zero would fail for a defect this change does not fix, and asserting one would pin a number #1210 is expected to move.The two shapes are separate arms because they reach different code. A no-qual count never reaches the row-group-limit lookup, so its only storage access is the format-version one and
seq_scan == 0reads cleanly for that site alone.How it was found: two instruments disagreeing, and both being right
An
eloginsidepgcolumnar_written_stripe_row_limitcounted 2 scans for a join.pg_stat_all_tables.seq_scanon the catalog counted 4. Neither was wrong: an elog at one function counts arrivals at that function, a counter on the relation counts scans from any caller, and the gap was the second reader.A first hypothesis — that a projection was present in one fixture and not the other — was driven with the projection count printed as a premise, and died: four calls with zero projections.
Both harnesses
Ten checks each, identical assertion names. The pytest twin got its own removal proof rather than inheriting the shell one, because the twins' regimes diverge: unfixed it is rc=1 with
storage after count(*) idx_scan=0 seq_scan=1, and it aborts at its first failure where the shell suite runs on, so it reports 8 checks against the shell's 10. The divergence is in the failure output, not in the names.Verification
Census 1519 -> 1521 by counting: 1606 rows, 85 not
never, 1521 + 85 == 1606.suites_not_covereddoes not move.Five instrument failures before the first measurement, one defect behind all five
None about the change, which built clean with zero warnings an hour before the first number.
Each time the run produced no verdict, and each time a grep written for the answer I expected hid the cause.
head -12on the log settled it in one go. Read the tail, then filter once you know what is there.Deliberately not included
PgColumnarSetSortedExtenthas the same shape on the same key. It is a write path holdingRowExclusiveLock, nothing has measured its cost or frequency, and theInvalidOidsweep of this file belongs to #1207/#1213.🤖 Generated with Claude Code
https://claude.ai/code/session_01XiFn3HteTXnGdRiA2xDP2n