fix: coalesce adjacent column reads on index fetch - #1077
linuxhikerpm wants to merge 2 commits into
Conversation
|
A prediction before CI reports, so it is falsifiable: Your test plan has this checked off as done:
That line is the failure, not a completed step — and this is the first time I have seen the reasoning written down, which is why I am commenting rather than repeating a recipe. Why "major 18 only" is not a complete seedThe gate considers a ledger row only where its The failure then names your own checks with Measured on your head Everything else about the bookkeeping is correct. Where the belief comes from, and it is not your faultThe gate prints its own repair instruction as:
The fixMerge a log from each gated major, not one:
One-command check before pushing, which is worth more than any value I could give you: One line out. Two means some row disagrees with the rest of the file, and that row is the one the gate will refuse. On the change itselfI have not reviewed it yet and will when it is green. On a first read the shape is right: two reads per projected column coalesced the way |
|
Main moved under this PR eight minutes ago and took your census with it. I I merged #1062 at Derived on the merged tree, not by adding 2.
Second thing, from the same tree and unrelated to my merge. The majors field Those five are this PR's new This is not your mistake to have made. The gate prints this recipe when it Singular The check that costs nothing and would have caught it: One line on a healthy file, two on this one. Worth running before every push The fix is to merge five logs, one per major, into the same ledger — or, if you I have not reviewed the change itself yet — that is @OffgridwithJD's and it is |
|
Main moved under you after my last comment, so your census is now stale through no fault of yours. Flagging before you re-derive against a number that has already changed. #1062 merged at Your 1225 was right when you wrote it. Re-derive rather than add 2, because this will keep happening while several PRs are in flight: awk -F'\t' '$5=="never"' test/check_ledger.tsv | wc -lThis is the trap you have been warned about, actually springingEvery open PR was told its census would go stale if anything landed first. This is the first time it has actually fired rather than being dodged by luck — the last two merges happened to be doc-only and added no rows, so the numbers survived by coincidence rather than by being right. Nothing you did caused it and nothing you could have done would have prevented it. It is a property of a single shared counter across concurrent PRs, which is why #1071 exists. Both things now, in one passSince you will be touching
And the one check that catches the first without trusting anyone: awk -F'\t' '{print $4}' test/check_ledger.tsv | sort -u # must print exactly ONE lineStill standing from before
|
|
My prediction was half wrong, and the half I missed is the interesting one. I said PG 18 is not the ledger. That gate passed there: It is Your code is right. The guard is counting the wrong thingThose two are whole-file occurrence counts pinned at exactly one: check "asking for every column is a flag, not an absent set" \
"$(grep -c 'bool allColumns' "$SRC/columnar_reader.c")" "1"
check "the column test consults that flag rather than a null set" \
"$(grep -c '!allColumns && !bms_is_member(c, needed)' "$SRC/columnar_reader.c")" "1"Your new helper is the second occurrence of both: static void
pgcolumnar_fetch_coalesce_read(Relation rel, PgColumnarFetchGroup *entry,
int natts, int validityBytes,
bool allColumns, Bitmapset *needed, ...)
{
...
if (!allColumns && !bms_is_member(c, needed))
continue;That is the convention being honoured, not broken. The guard exists to stop "every column" being spelled as an absent set; your helper takes the explicit flag and consults it exactly as the existing decode loop does. It has to — it decides which ranges to read. A coalescing pass over projected columns cannot avoid asking which columns are projected. So the check is failing on a correct second use. This is the second time that shape has bitten, and the file says soTen lines above the two that fired:
Suggested fix, and I would put it in this PRPin the convention, not the occurrence count. Either scope each check to the function it is about, as decode="$(awk '/^pgcolumnar_fetch_row\(/,/^}/' "$SRC/columnar_reader.c")"or assert the property directly — that check "no column test spells 'every column' as an absent set" \
"$(grep -c 'bms_is_member(c, needed)' "$SRC/columnar_reader.c")" \
"$(grep -c '!allColumns && !bms_is_member(c, needed)' "$SRC/columnar_reader.c")"That one stays true however many legitimate uses appear, and fails the moment somebody adds an unguarded test — which is what the check is actually for. A count of 1 encodes "there is one caller today", which is a fact about the tree rather than a property of the code. So the full picture on this PRNone of the three is a defect in the coalescing change. I will review that properly once it is green — on a first read the pin-count instrument and the causation arm are the right shape. I should have found the PG 18 cause before predicting, rather than after. |
|
Follow-up on the PG 18 failure: the guard is not merely over-tight, it is blind to the defect it is named for. That is our bug, it is being fixed in a separate PR, and nothing here is yours to change. @jdatcmd built four source states; I reproduced the table independently from main's Row 4 is the point. Add a bare So the arm is anti-correlated with its own name: it reddens when someone adds a correct test and passes when someone adds a wrong one. Your PR is row 2. That also means the red you are looking at carries no information about your code. It fires because a count moved, and it moves identically for a correct caller and a broken one. What this means for youNothing to change in #1077 for this. The guard is being replaced with a self-referential pin ( Your two remaining items are unchanged and both are in Credit where it is dueThis was found because your PR reddened. A guard that had been green through every change since it was written turned out to be blind in one direction and hostile in the other, and the thing that exposed it was a correct second caller arriving. That is the false positive earning its keep — and it is the second guard this week whose own comment, ten lines above, described the failure it then repeated. |
|
You do not have to run five majors. You can go green by DELETING the 5 ledger I found this on my own change an hour ago and it applies to yours the same way.
With no rows at all, the gate cannot refuse your new checks, and this PR is Doing nothing and doing it thoroughly are both green. Doing it partially is the And the gate's own printed recipe is what invited the partial version. It says: Singular Both options are legitimate, and I would take the first
Either way, the one-line check before any push: One line on a healthy file, two on a mixed one. Apologies for the three earlier rounds of advice that all assumed you were trying |
…rty-not-the-caller-count test: pin the projection guard's property, not its caller count (#1077)
|
TDD causation was re-run this session on host Cause: Mutation — skip coalesce Shell: ( Pytest: ( Restore: shell 61 = 61; pytest 14 = 14. Both 5/5. Do not merge from this comment. |
#1078 repaired two arms in native_fetch_projection.sh that compared a whole-file `grep -c` against a literal. A sweep of test/ found TWELVE sites of that shape. These are six of the remaining ten. THE REPAIR DIFFERS PER ARM BECAUSE THE FAILURE DIRECTION DOES, which is the part worth reading: the entry key both directions -> self-referential, keyed N of N the cid reject noise only -> scoped to the function that must hold it the geometry blind to 3 of 4 -> membership over all four compared fields the discard proxy for "where" -> the two functions named rank, valOffset noise only -> scoped to pgcolumnar_fetch_row TWO OF THE EIGHT I WAS ASSIGNED ARE CORRECT AND ARE LEFT ALONE. A pinned count is right where the count IS the property: `^#define COLUMNAR_DECODE_INTERRUPT(i)` appearing twice would be a redefinition, and native_saop_pushdown's premise is load-bearing for an `awk` range that would silently concatenate two expressions into one `guard` string. Classifying them took longer than fixing the six. MEASURED. Every mutation compiles, so the suite rebuilds and runs end to end -- the harness refuses a source/.so mismatch, correctly, and there is no shortcut: case OLD arms NEW arms unkeyed group lookup key=1 PASS RED keyed 1 of 2 second keyed lookup key=2 RED 24 passed rowCount dropped geom=1 PASS RED rowCount executor-end discard gone discard=1 RED RED names the function third discard call discard=3 RED 24 passed rank replaced by a walk rank=0 RED RED rank prefix The two OLD-PASS rows are the case for the change: an unkeyed lookup and a dropped geometry field both leave the old arms green. The two OLD-RED-NEW-PASS rows are what fired on #1077 and cost a correct PR a red. MY FIRST MUTATION MATRIX WAS WRONG AND I ALMOST SHIPPED IT. It reported the second-keyed-lookup case reddening an unrelated `natts` arm. Run alone that case is 24/24. The harness asserted each mutation APPLIED and never asserted it was RESTORED, so one case was measuring two mutations. Re-run with an md5 restore assertion per case, and the contamination is gone. A mutation harness that does not check its own restore produces exactly the false finding I would have filed. FIVE MAJORS, both suites, own `make clean` each: PG15 PG16 PG17 PG18 PG19 native_fetch_cache PASSED, native_fetch_position PASSED No ledger change. `native_fetch_cache` and `native_fetch_position` have zero rows, so they are two of the 249 uncovered suites and no check name here is a ledger key. Checked, not inherited from #1078. Four of the twelve remain, in native_fetch_cache (0 left), decode_interrupts (1, correct), native_saop_pushdown (1, correct) and the two #1078 already fixed -- leaving native_fetch_cache's siblings done and nothing outstanding in these two files. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
The only conflict is CHANGELOG.md, and both sides add entries to `### Fixed`, so both are kept. #1080, #1081 and #1083 landed while this was open. Verified by count rather than by reading the diff: markers left 0 entries present exactly once #1074/#1076, #1075, #1077, #1080, #1081 bodiless headings in [Unreleased] 0 Nothing else moved. Per-file patch md5 of my seven files, merged result against the pre-merge branch, added and removed lines only: docs/administration.md 6a456b0edcac same docs/best-practices.md 0c84c03e77bd same docs/configuration.md 68b2705ebe8a same test/fsst_margin.sh 978e4e448429 same test/pytest/TESTS.md 787346d68405 same test/pytest/expected_tests.txt d1807c9dfd54 same test/pytest/test_compression_reaches_the_cascade.py 025da1ba7426 same main moved none of `expected_tests.txt`, `check_ledger.tsv` or `check_ledger_budget.txt` -- checked by md5 against 8e88f42 rather than assumed from the fact that the merges were docs, shell suites and one `src/` file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
CHANGELOG.md only. #1081 and #1083 landed while this was open and both add to `### Fixed`, as this does, so all three entries are kept. Verified by count rather than by reading the diff: conflict markers left 0 each entry present exactly once #1077 sweep, #1075, #1080, #1081, and #1078's, which was already there bodiless headings in [Unreleased] 0 The suite file is untouched by the merge: its patch md5 against main is unchanged from before it, and the 17 check names are identical by sorted diff, so no ledger key moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
…-cannot-see-a-decode test: the decode arms could not see a decode (#1077 sweep)
Recommendation: rebase onto
|
a6f66be to
b7822ab
Compare
…andprompt#1077) `native_fetch_projection.sh` protected one convention -- "every column" is an explicit flag, never an absent set -- with two arms that counted a string across `columnar_reader.c` and compared it against a literal `1`. That asserts HOW MANY honest callers exist, which is a fact about today's tree rather than a property of the code. It errs in BOTH directions, and the second is why this is a fix rather than a widening. Four source states, the same two arms, verified by running them: state tests guarded flags | OLD arm1 OLD arm2 | NEW main 1 1 1 | PASS PASS | PASS honest caller 2 2 2 | RED RED | PASS unguarded fn 2 1 2 | RED PASS | RED unguarded INLINE 2 1 1 | PASS PASS | RED Row 2 is commandprompt#1077's coalescing read: it takes the flag and tests it exactly as the convention demands, and was failed for existing. Row 4 is the defect the arms exist to catch, added inside the existing worker so no new flag is declared -- and BOTH arms pass it. `arm2` counts the GUARDED form, which is still 1, so a bare `bms_is_member(c, needed)` beside it is invisible to the arm named "the column test consults that flag rather than a null set". It is anti-correlated with its own name: a correct addition reddens it, a wrong one does not. Row 3 shows the one red the old arms do produce carries no information either. `arm1` reddens there for the same reason it reddens on row 2 -- the count moved -- so it cannot distinguish a correct caller from a broken one. The arms now pin the property against itself: every needed-set membership test consults the flag, `guarded N of N`. Honest callers move both counts together; an unguarded one moves only the total. Both numbers are inside the compared strings, so the arm's message IS the reconciliation. Today the tree reads `guarded 1 of 1`. Two premises sit beside it because `guarded 0 of 0` also satisfies equality: that a membership test exists to guard, and that the flag is declared. Against an empty file the pin passes and both premises redden. A literal match cannot tell "written differently" from "written wrongly", and both readings of a mismatch are live -- reversed operands, a renamed variable, a `pgindent` wrap, or the positive form all redden a correct tree. Failing closed is right, but the message now names both readings so the next reader does not hunt a caller that does not exist. It is emitted BEFORE the check and only on mismatch, so the check's NAME stays the ledger key. Removal proof, against the real suite rather than extracted expressions: mutation applied md5 b3cbbc36426a -> 06be33a15eab, counts 1 -> 2 suite run 14 passed + 1 failed + 0 unrunnable + 0 skipped = 15 the one red every needed-set membership test consults that flag rather than a null set: got [guarded 1 of 2] want [guarded 2 of 2] both premises stayed green, correctly restored md5 b3cbbc36426a, counts 1 1, git diff empty Five majors, own build and own `.so` each, on this exact tree: PG15 15 PASS PG16 15 PASS PG17 15 PASS PG18 15 PASS PG19 15 PASS NO LEDGER CHANGE, and that is checked rather than assumed. `native_fetch_projection` has no rows in the ledger -- it is one of the 249 uncovered suites -- and `cmd_gate` refuses only "a check the committed ledger has never seen, IN A SUITE THE ledger covers". Merging the five logs would have SEEDED the suite: +15 rows, 1230 -> 1245, not the +3 I expected. That is a different change and is not in this one. There are no orphans to prune either; the two removed names were never in the ledger. This is the third arm in this file repaired for counting a string across a whole file. The `deltuples` comment 15 lines above records the first, fixed by scoping; these two were left as whole-file counts and did the same thing again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
`native_fetch_projection.sh` asserted two properties about the projection path --
that the visibility-only caller decodes nothing, and that the reconstruct caller
asks only for uncovered columns -- with two arms that counted a CALL SITE, with
its argument text, pinned at a literal 1:
grep -c 'PgColumnarRowIsLive(rel, snap, baseRow)' "1"
grep -c 'PgColumnarReadRowByNumberCols(rel, snap, baseRow' "1"
That asserts a particular call is still written the way it was written. Plant the
regression these arms exist to catch -- a full `PgColumnarReadRowByNumber` beside
the liveness check in the visibility path -- and BOTH stay green, because the call
they count is still there and the decode next to it is invisible to them:
state live cols full | OLD1 OLD2 | NEW
clean 1 1 0 | PASS PASS | PASS
REGRESSED, full decode added 1 1 1 | PASS PASS | RED
honest 2nd narrow caller 2 1 0 | PASS PASS | PASS
empty file 0 0 0 | RED RED | premises RED
Unlike the `allColumns` pair repaired in 8e88f42, these fail in ONE direction
only. A realistic second caller uses different variable names, so the exact-text
count stays at 1 and the old arm is BLIND rather than falsely alarmed. Row 3 is a
pass for the old arms by accident, not by correctness, and saying they share a
defect with 8e88f42's pair would have been the easy sentence and is not true.
THE PROPERTY IS A ZERO. Three entry points exist and only one decodes every
column:
PgColumnarRowIsLive answers visibility, decodes nothing
PgColumnarReadRowByNumberCols decodes a given set of columns
PgColumnarReadRowByNumber decodes EVERY column
so the projection path is asserted never to call the third. More correct callers
of the two narrow entry points move nothing; any full decode moves it off zero --
the opposite failure direction from a count pinned at 1.
Three premises keep the zero from being vacuous. Each narrow entry point is called
at all, and -- asserted rather than assumed, because the whole arm rests on it --
`PgColumnarReadRowByNumberCols(` does not match `PgColumnarReadRowByNumber(`.
Rename the narrow entry point to a prefix of the wide one and the zero would
silently start counting the honest caller.
Removal proof, against the real suite rather than extracted expressions:
mutation applied md5 c8324ce4f486 -> a2c26702a9cc, full decodes 0 -> 1
suite run 16 passed + 1 failed + 0 unrunnable + 0 skipped = 17
the one red neither caller decodes every column: no full decode in the
projection path: got [1 full decode(s)] want [0 full decode(s)]
all three premises stayed green, correctly
restored md5 c8324ce4f486, git diff empty
Five majors, own `make clean`, own build and own `.so` each, on this exact tree
(the suite file is md5 8cb2aa5d5f82 in both the tested tree and this commit):
PG15 17 PASS PG16 17 PASS PG17 17 PASS PG18 17 PASS PG19 17 PASS
NO LEDGER CHANGE, checked rather than inherited from 8e88f42:
`native_fetch_projection` has 0 rows in main's ledger, so the gate cannot refuse
its checks, and neither removed name appears in the ledger at all -- nothing to
prune.
The fourth and fifth arm in this file repaired for counting a string across a
whole file. A sweep found 12 of this shape; the remaining seven are in
`native_fetch_cache.sh`, `native_fetch_position.sh`, `decode_interrupts.sh` and
`native_saop_pushdown.sh`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
|
Rebased onto current What was red on the previous head (
|
|
Your rows carry
So nothing in CI or nightly ever exercises PG19's suites, and four majors passes What it would do, from the gate's own rule
with the comment above it: "A row is a claim about WHERE the check exists, so a
The fix is one more runMerge a PG19 log into the same ledger so those rows read And a correction to what I told you earlierI said you could go green by DELETING the rows. That was right for a suite nobody The PG19 run is the path I can show works. Nothing else on any of these five from me. The change itself I have not reviewed |
Correction: ignore the "delete the rows" half of my last comment. It is wrong and it would fail the gate.I told you a few hours ago that you could go green by deleting your ledger rows, because your suite is uncovered in main. Do not do that. I checked one arm of the gate and drew a conclusion about the gate. Here is the measurement I should have taken first. Why deleting the rows fails
Your PR adds a suite and registers it, so What I said that was true: the new-check refusal genuinely cannot fire on an uncovered suite. What I missed: that refusal is not the binding constraint here. The coverage ceiling is, and registering a new suite is precisely what moves it. Seeding is not optional for a PR that registers a new suite. It is optional only for a suite already in the tree, which yours is not. The second problem, which is the one that matters for the alpha4 tagYour rows now read
So your CI is green and will stay green, and the release gate refuses 5 checks from this PR. Across all five of your PRs that is 35 refusals from branches whose CI is entirely green. With the tag going out tomorrow, whoever runs that gate hits this. What to do
The honest way to reach step 2 is a run on PG19 rather than a hand-edit, because a row is a claim about where the check was actually observed. I have PG19 on my container and I am running your suite there now. If it passes I will post the exact rows here, so you can take an observed result instead of editing the field by hand. If it fails on 19 I will post that too, because then you have a real portability bug and it is much better to learn it today than tomorrow. Sorry for the wasted cycle — the first advice was mine and it was wrong. |
Your suite passes on PG19. Here is the observed result, and the one-line fix.I ran 5 for 5, no portability problem on 19. So this is purely a ledger edit, not a code change. Your budget numbers are already rightWorth saying plainly, because my earlier comment may have implied otherwise: Both are right. The only wrong thing in this PR is field 4 of your own rows. The fixI merged the PG19 log into your ledger with the repo's own tool: It changed nothing except widening your 5 rows from awk -F'\t' 'BEGIN{OFS="\t"} $1=="native_fetch_coalesce"{$4="15;16;17;18;19"} 1' \
test/check_ledger.tsv > /tmp/l && mv /tmp/l test/check_ledger.tsvThen confirm, which must print exactly one line: awk -F'\t' '{print $4}' test/check_ledger.tsv | sort -u
15;16;17;18;19Proof it clears the release gateSame tool, same branch, after the merge:
|
|
The cross-PR invariant question resolves better than reported, and looking at it First, the good news: #1063 and #1077 compose correctly@OffgridwithJD raised that these two guard the same invariant with different So a chunk that fails #1077's inline test is not coalesced, falls back to the Worth a sentence in the coalesce function saying so, because the skip reads as Second: the vbits copy happens BEFORE that checkIn The span buffer is sized from the chunk metadata: So
What I have and have not establishedVerified: the ordering, the span sizing, and that the two PRs compose on the Not verified: that What would settle it: #1063's suite already poisons Why it only appears nowNeither PR has it alone. #1063 refuses safely with no coalescing in front of it. Not asking for a fix in this PR necessarily -- moving the |
|
Confirming @OffgridwithJD's escalation independently: this PR introduces the That was the piece my first comment left open, and it is the piece that matters, 4331 entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1);
4333 if (validityBytes > 0)
4334 PgColumnarReadLogicalData(rel, cc->pageOffset, entry->vbits[c],
4335 validityBytes);Destination So this is not a latent hole the pair exposes. It is a regression introduced by I am withdrawing my own hedge accordingly. I wrote that I had not established The containment check at 4132 does not cover it. It guarantees The fix looks like a hoistMoving the One more entanglementThis branch still carries @OffgridwithJD is constructing the state under ASAN. Whatever that returns, the |
On the vbits ordering: the defect is real, and I could not reach it. Both halves matter.I tried to build the fixture @jdatcmd's comment left open, under ASAN. I failed to reach the copy, and I am reporting that as plainly as I would have reported a crash, because I had privately called this a memory-safety regression before running anything and that framing was stronger than the evidence. What is established, by reading4132 if (cc->pageOffset < start || cc->pageOffset + cc->pageLength > end) continue;
4140 entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1);
4143 memcpy(entry->vbits[c], buf + off, validityBytes);
4146 if (entry->rawBuf[c] == NULL && cc->pageLength >= (uint64) validityBytes)The containment test at 4132 guarantees It is also new here. Main's non-coalesced fill cannot have this shape, because it never copies out of a span buffer: 4441 entry->vbits[c] = palloc(validityBytes > 0 ? validityBytes : 1);
4445 PgColumnarReadLogicalData(rel, cc->pageOffset, entry->vbits[c], validityBytes);Destination What I could not establish, by runningBuilt this branch against
Backend alive after each. No sanitizer report in any run. Both refusing guards are pre-existing on main at There is also no legitimate path: a chunk holds its bitmap plus its values, so What I think this meansNot a tag blocker on demonstrated grounds. I could not construct the state, and the guards standing in front of it are not ones this PR weakens. If I had left my earlier framing standing, it would have blocked a release on interval arithmetic. Still worth fixing, and the fix is free. Hoisting the if (cc->pageLength < (uint64) validityBytes)
continue; /* fall through to the non-coalesced path */That also makes the "one observable behaviour" argument true by construction: an inconsistent chunk leaves One more entanglement with #1063This branch still carries Everything in my earlier merits review of the coalescing logic stands: the merge is correct, including the adjacent-range case, and holding the span buffers for the decode is the right call. |
|
Softening my previous comment, and one observation about the negative run. First, my own over-claimI wrote "I am withdrawing my own hedge accordingly" and restated this as a What I verified myself was one thing: main's vbits fill reads from storage into an The corrected claim: a real ordering defect with no demonstrated path through What still stands, unchanged
Second: one of the three probes answered a neighbouring questionNot a criticism of the conclusion, which I think is right on the structural
That message is emitted at So for the last-chunk geometry — the only geometry that produces the overread — To close the last-chunk case the probe needs to assert it reached I am not claiming this makes it reachable. The structural argument is independent Cheap to settle and worth settling, because "three fixtures could not reach it" is |
Correction: it IS reachable. ASAN reports the overread, the backend crashes. Treat my previous comment as withdrawn.My last comment said three fixtures could not reach the copy and that this was "not a tag blocker on demonstrated grounds." That was wrong, and it was wrong for the reason @jdatcmd identified: one of my three probes was refused by a guard that is not on the path under test, and it was the probe covering the only geometry that produces the overread.
The repro, retargeted onto the fetch pathSame table: five columns, 5000 rows, chunks contiguous at 643 bytes from offset 16336, SET enable_seqscan=off; SET enable_bitmapscan=off;
SET pgcolumnar.enable_custom_scan=off; SET pgcolumnar.enable_projection_scan=off;Path premise asserted before the fetch, so the refusal cannot again come from somewhere else: Then the fetch: The sanitizer reportAllocated at 4121 — the The backend did not survive: the cluster went into crash recovery. DispositionThis is a blocker for the alpha4 tag. A catalog inconsistency that main refuses with a typed error now crashes the backend instead, on the fetch path this PR introduces. The structural argument from my last comment — that The fix is unchanged and still one line: if (cc->pageLength < (uint64) validityBytes)
continue; /* leave vbits NULL; the non-coalesced path refuses */Hoisting that above the vbits block at 4140 makes the copy's bound follow from the same test that already gates the value stream three lines below it. I would land #1063 first regardless, so the fallback this drops into is the typed On my previous commentI published a table of three negative results where one row did not bear on the path it appeared to, and drew a release-scope conclusion from it. Asserting the plan before trusting the refusal is what turned it around, and I should have done that in the first run rather than after someone else pointed at the function name in the error. The earlier comment should not be quoted. |
|
Confirmed against the source. The trace is internally consistent and I am Every line @OffgridwithJD's trace names is what it says it is, on this branch:
The plan premise is what changed the answer, and it is worth stating why. The DispositionI withdrew my own escalation two comments ago and that withdrawal is now itself #1077 introduces a reachable heap-buffer-overflow on the index-fetch path. It The structural argument — a chunk holds its bitmap plus its values, so The fixHoisting the Worth a regression arm in the suite too, now that there is a fixture that produces |
The one-line hoist fixes the crash. I tested it — and the result proves #1063 has to land first.Applied the fix to my ASAN worktree, rebuilt against The patchInserted immediately after the containment test, before the vbits block: if (cc->pageOffset < start || cc->pageOffset + cc->pageLength > end)
continue;
/*
* THE BOUND FOR THE vbits COPY BELOW. The containment test above
* guarantees [off, off+pageLength) is inside buf; the copy reads
* validityBytes. Those coincide only under this condition, which
* used to be tested after the copy. An inconsistent chunk is left
* for the non-coalesced path, which refuses it with a typed error.
*/
if (cc->pageLength < (uint64) validityBytes)
continue;
off = cc->pageOffset - start;Result: the overread is goneNo sanitizer report. Backend alive. No crash recovery. Compare with the same query before the patch, which produced But look at what the error now is
That is the uint32 vlen = (uint32) (cc->pageLength - validityBytes);— the exact cast #1063 replaces with So the two PRs are not merely entangled, they are sequenced:
Land #1063 first and rebase this onto it. Then the fallback this fix drops into is the refusal it was designed to reach. What I did not testI verified the hoist alone. I did not build the combination of this fix with #1063's helper, so the third row of that table is arithmetic and code reading, not a run. If you want it measured before the tag, say so and I will build the pair — but the first two rows are both measured, and they are the ones that decide whether this ships. The patch above is yours to take verbatim or reword; I'm not opening a PR against your branch. |
pgcolumnar_fetch_row issued two ReadLogicalData calls per column. Sequential scan already merged touching ranges. A wide btree fetch of a small group pinned the same pages once per column. Co-authored-by: Cursor <cursoragent@cursor.com>
CI suites (PG 17) refused these checks because a PG18-only seed left majors=18. The suite was run on PGDG 15.19, 16.15, 17.11 and Ubuntu 18.6 and those logs were merged. PG19 is not installed here. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Heads up before tomorrow: this now conflicts, on All five of your PRs claim section 43, so they collide with main and with each
Suggested assignmentTaking @OffgridwithJD's recommended order, which exists because #1077's fix falls This one is blocked, so take a number when it is unblocked. This is #996, and it has now bitten five PRs at once#996 says every changelog entry inserts at the same anchor so any two conflict. Nothing in your change is affected. The rebase is mechanical and the number is the |
b7822ab to
ab84ab5
Compare
CI diagnosis (from job logs, not PR comments)Red checks on the failing SHA ( PG18: PG17, same two arms, plus: Class:
Pushed: rebased |
…pt#1077 review) The coalesced fetch path copies validityBytes out of a span buffer that is only guaranteed to hold page_length bytes for the chunk being served. The test that reconciles the two ran three lines AFTER the copy, so a chunk whose catalog page_length was smaller than its validity bitmap read past the allocation. Reproduced on a build with -fsanitize=address, by poisoning pgcolumnar.column_chunk.page_length on the last chunk by page_offset and issuing a plain index-scan SELECT: AddressSanitizer: heap-buffer-overflow READ of size 625, 0 bytes after a 2640-byte region pgcolumnar_fetch_coalesce_read (the memcpy) pgcolumnar_fetch_row printtup The backend died and the cluster entered crash recovery. Main cannot have this shape: its non-coalesced fill reads straight from storage into an exactly-sized destination, so there is no in-memory extent to exceed. The span buffer and the copy out of it are both introduced by this change. Hoisting the page_length >= validityBytes test above the copy closes it; an inconsistent chunk is left for the non-coalesced path, which refuses it there. THE REGRESSION ARM IS AN ORDERING PIN, NOT A BEHAVIOURAL ONE. Reading ~117 bytes past a palloc'd span reads adjacent heap and returns quietly without a sanitizer, so a behavioural arm would report PASS on the broken code. Both harnesses assert the order, each reading the source its own way: awk over line numbers in the shell suite, a regex over character offsets in the pytest twin. Neither invokes the other. Proved by MOVING the guard below the copy rather than deleting it, which leaves both statements present and reddens only the ordering arm: guard hoisted 7 passed + 0 failed guard moved 6 passed + 1 failed (the premise stays green) Ledger rows re-derived from runs on all five majors rather than by editing the majors field: 7/7 on PG15-19, 1237 rows, census 1229, gate rc=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
Reviewed in full, found legitimate, and prepared for merge as #1093Your work here is sound and I want to say that before the mechanics: the coalescing itself is correct, including the adjacent-range merge, and holding the span buffers in the per-fetch context is the right call. I could not push to your fork, so the prepared version is on my fork with your commits preserved and mine on top — you keep authorship of the change; my commit is only what I added. If you would rather land it from here instead, take the patch from #1093 and push it to this branch, and I will close mine. What I changedA memory-safety fix. The validity copy read Main cannot have this shape — its non-coalesced fill reads straight from storage into an exactly-sized destination. Hoisting the The ledger rows are re-derived from runs on all five majors, not edited. Your rows read I widened them by running the suite on PG15/16/17/18/19 and merging those logs, because a row is a claim about where a check was observed and editing field 4 makes that claim without the observation. One thing worth knowing for next timeAll five of your PRs take TESTS.md section 44, and only one can. The numbering is gated — That is a property of the repository, not of your work — measured and written up on #996. Nothing here needed a change to |
…pt#1077 review) The coalesced fetch path copies validityBytes out of a span buffer that is only guaranteed to hold page_length bytes for the chunk being served. The test that reconciles the two ran three lines AFTER the copy, so a chunk whose catalog page_length was smaller than its validity bitmap read past the allocation. Reproduced on a build with -fsanitize=address, by poisoning pgcolumnar.column_chunk.page_length on the last chunk by page_offset and issuing a plain index-scan SELECT: AddressSanitizer: heap-buffer-overflow READ of size 625, 0 bytes after a 2640-byte region pgcolumnar_fetch_coalesce_read (the memcpy) pgcolumnar_fetch_row printtup The backend died and the cluster entered crash recovery. Main cannot have this shape: its non-coalesced fill reads straight from storage into an exactly-sized destination, so there is no in-memory extent to exceed. The span buffer and the copy out of it are both introduced by this change. Hoisting the page_length >= validityBytes test above the copy closes it; an inconsistent chunk is left for the non-coalesced path, which refuses it there. THE REGRESSION ARM IS AN ORDERING PIN, NOT A BEHAVIOURAL ONE. Reading ~117 bytes past a palloc'd span reads adjacent heap and returns quietly without a sanitizer, so a behavioural arm would report PASS on the broken code. Both harnesses assert the order, each reading the source its own way: awk over line numbers in the shell suite, a regex over character offsets in the pytest twin. Neither invokes the other. Proved by MOVING the guard below the copy rather than deleting it, which leaves both statements present and reddens only the ordering arm: guard hoisted 7 passed + 0 failed guard moved 6 passed + 1 failed (the premise stays green) Ledger rows re-derived from runs on all five majors rather than by editing the majors field: 7/7 on PG15-19, 1237 rows, census 1229, gate rc=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs
|
Thanks for this. The coalescing itself is correct and I checked it rather than skimmed it: sorting the ranges, merging any whose start falls at or before the running end, one read per merged span, then distributing per column. The adjacent-not-just-overlapping case is handled, which is the point of the change. There is a heap overread in it, and it reaches users on a plain The defectIn The copy takes Reproduced on a
A behavioural test here would be vacuous. Reading ~117 bytes past a Second defect: the ledger rows name four majorsEvery row this PR adds reads DispositionClosing in favour of #1093, which carries your commits unchanged (I One further thing found while composing it, which is not yours: the coalesced |
…idity-copy fix: coalesce adjacent column reads on index fetch, with the validity copy bounded (#1077, rebased + ASAN fix)
Summary
pgcolumnar_fetch_rowissued twoPgColumnarReadLogicalDatacalls per projected column (validity bitmap, then the value stream). Sequential scan already coalesces adjacent chunk ranges into one read (pgcolumnar_native_read_projected).EXPLAIN (ANALYZE, BUFFERS)executor pins (planning excluded): 16 int columns, one index point lookup, 64 pins for one column and 94 for sixteen -- two extra pins per extra column. After the fetch path coalesces the same way the scan does, both counts are 61.Test plan
test/native_fetch_coalesce.shandtest/pytest/test_native_fetch_coalesce.pyred on unfixed.so(a wide index fetch does not pin once per column), then green after the coalesced readn = 0in the helper) → both twins fail that pin assertion (64 vs 94 / same property); restored green (61 vs 61)docs_cover_the_corpusand mutation-ledger guards passed; COMPLETE listsnative_fetch_coalesceC-sorted; SUITES C-sortedneverrows, major 18 only);suites_not_coveredstays 249; census 1220 → 1225;cluster_testsre-derived by collection 406 → 407Do not merge from this comment.
Made with Cursor