Skip to content

fix: refuse a uint32-truncated column chunk on index fetch (#1063, rebased + ledger) - #1092

Merged
jdatcmd merged 4 commits into
commandprompt:mainfrom
OffgridwithJD:review/1063-ledger-names-pg19
Sep 17, 2026
Merged

jdatcmd merged 4 commits into
commandprompt:mainfrom
OffgridwithJD:review/1063-ledger-names-pg19

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

This is @linuxhikerpm's #1063, rebased onto cfdb393 with its ledger rows re-derived. Their commits are preserved; mine is the last one. Opened from my fork because I cannot push to theirs.

Land this one first of the five. #1077's fix falls through to the uint32 cast that this change repairs, so the other order leaves a window where an index fetch answers with a raw allocator error instead of the typed XX001.

What I reviewed and found legitimate

The C is complete and I verified it rather than reading it: columnar_reader.c:2721 and :4369 are the only two places in the tree that cast (uint32) (pageLength - validityBytes), and both are fixed. No third site exists. Nothing to argue with.

I also attacked the guard through its own seam with a different catalog column and it still fires, so it is broader than the PR claims:

UPDATE pgcolumnar.row_group SET row_count = row_count + 4294967296 ...
  -> XX001: columnar chunk for column 1 has a validity bitmap longer than the chunk

What I changed

Only the ledger. Their rows read 15;16;17;18 while every other row in the file reads 15;16;17;18;19, so covered_majors includes 19 and the release gate refuses this suite's six checks — while CI stays green, because ci.yml:503 runs 17+18 per PR and only the local five-major matrix adds 19.

The rows are re-derived from runs, not edited. A row is a claim about where a check was observed; widening field 4 by hand makes that claim without the observation.

native_chunk_length_bound on PG15/16/17/18/19:  6/6 each, rc=0
ledger 1236 rows, uniform 15;16;17;18;19, census 1228
GATE rc=0   registered=254 covered=5 not-covered=249 ceiling=249

check_ledger_budget.txt's suites_not_covered needed no change; the census is re-derived from the merged ledger.

Merge order and the section number

This branch takes TESTS.md section 44, which is correct while main is at 43. All five of these PRs take 44 — the numbering is gated (test_the_contents_list_is_numbered_in_order requires 1..N with no gap), so the number is settled by merge order and the second to land renumbers.

After this merges, each remaining PR needs its section incremented and its census re-derived — not resolved. The conflict offers two numbers and neither is right; see #996 for the measurement.

Open, not blocking

The validityBytes < 0 clamp is the one asymmetry: an impossible value is silently accepted where the over-long bitmap refuses, and it escapes as a generic XX000 from smgr rather than the typed XX001. Reachable through the same seam. Diagnosis quality, not a hole.

Original: #1063. Author: @linuxhikerpm.

jdatcmd and others added 4 commits September 17, 2026 00:05
page_length is uint64; both decode entry points cast the value stream to
uint32. Adding 2^32 left the low bits unchanged, so a btree fetch returned
the row. Sequential scan already refused via containment.

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>
… edit

The rows read 15;16;17;18 while every other row in the ledger reads
15;16;17;18;19. covered_majors is the union over all rows, so it includes 19, and
pgc_ledger.py refuses a known check seen on a major its own row does not name.
ci.yml:503: the per-PR gate runs 17+18, nightly runs 15-18, and the local
five-major matrix adding PG19 remains the release gate. So CI was green and the
release gate would have refused this suite's checks.

The rows are re-derived by running the suite on all five majors and merging those
logs, not by editing field 4. A row is a claim about where a check was observed,
and widening it by hand makes that claim without the observation.

check_ledger_budget.txt needed no change to suites_not_covered; the census is
re-derived from the merged ledger.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

@jdatcmd jdatcmd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving. I verified the claims rather than reading them.

The two cast sites are the only two. (uint32) (pageLength - validityBytes) appears at exactly two places in columnar_reader.c on main, and this fixes both. The helper refuses in both directions, underflow and >UINT32_MAX, and ERRCODE_DATA_CORRUPTED is the right SQLSTATE.

The ledger correction is load-bearing and I proved it. I ran pgc_ledger.py gate with the same PG19 observation against two ledgers differing only in the majors field:

@linuxhikerpm's rows (15;16;17;18)   -> "1 check(s) the ledger has never seen"  rc=1
this PR's rows (15;16;17;18;19)      -> "census stated 1228, ledger holds 1228"  rc=0

One variable, opposite results. Their #1063 could not have passed the release gate and CI could not have told them, because ci.yml:503 runs 17 and 18 only.

Commit preservation checked: every commit from #1063 is an ancestor of this branch.

Merge first, and note that #1093 needs the deferral guard it now carries, or this PR's own arm regresses to XX000.

@jdatcmd
jdatcmd merged commit fb6ad6a into commandprompt:main Sep 17, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants