Skip to content

The natts comparison is not against the row group (#1081) - #1084

Merged
jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:fix/1084-the-natts-comparison-is-not-against-rg
Sep 16, 2026
Merged

jdatcmd merged 1 commit into
commandprompt:mainfrom
OffgridwithJD:fix/1084-the-natts-comparison-is-not-against-rg

Conversation

@OffgridwithJD

@OffgridwithJD OffgridwithJD commented Sep 16, 2026 •

Copy link
Copy Markdown
Collaborator

#1081 replaced a count of entry->fileOffset != rg->fileOffset with a per-field membership loop, so that dropping any one of the four compared fields reddens by name. The loop matched entry->$_f != rg->$_f or entry->$_f != natts for every field — because natts is the one field compared against the scan's own column count rather than against the row group.

if (hit &&
    (entry->firstRowNumber != rg->firstRowNumber ||
     entry->rowCount       != rg->rowCount       ||
     entry->fileOffset     != rg->fileOffset     ||
     entry->natts          != natts))            /* <- not against rg */

Harmless today, since entry->firstRowNumber != natts appears nowhere. Still wrong as a claim: each arm would accept a comparison its own name denies. Three fields now match only the rg form, and natts has its own arm and its own name.

Reported by @jdatcmd reviewing #1081.

All four verified by removal, on the real suite

Each reddens only its own arm:

firstRowNumber removed   FAIL  a hit re-checks the group's firstRowNumber against the row group
rowCount removed         FAIL  a hit re-checks the group's rowCount against the row group
fileOffset removed       FAIL  a hit re-checks the group's fileOffset against the row group
natts removed            FAIL  a hit re-checks the group's natts against the scan's column count
restored                 26 passed, source byte-identical

firstRowNumber was shipped UNPROVEN, not broken — and the difference matters

Corrected after review by @jdatcmd, who checked the thing I asserted.

I first wrote that #1081 shipped this arm unproven because its mutation never applied. The second half of that is true and the implication was not: my mutation missed, the arm did not.

The arm matches a substring, and the leading paren on line 4213 is invisible to a case glob:

source      		(entry->firstRowNumber != rg->firstRowNumber ||
arm pattern *"entry->firstRowNumber != rg->firstRowNumber"*     -> matches
my #1081 mutation regex  \n\t\t ?entry->...                     -> does NOT match

So main was never carrying a broken arm. It was carrying a working but undemonstrated one, and the removal proof in this PR — which uses (entry-> as its anchor — is the demonstration it was missing.

That distinction is worth the paragraph because the two states call for different responses. "Shipped broken" would mean a guard in main is not guarding. "Shipped unproven" means the evidence was absent, which is what happened.

What the incident is actually evidence for is narrower and still worth keeping: an applied-assertion told me a case had measured nothing. Without it the clean suite run that followed would have been recorded as a passing case. That is the same family as the restore-assertion bug in the same matrix — one end of a mutation rather than the other — and it is the guard that should exist, not a defect it found.

Five majors

PG15  PG16  PG17  PG18  PG19     native_fetch_cache.sh: PASSED

No ledger change

One check name added, none removed, and native_fetch_cache has zero rows — it is one of the 249 uncovered suites, so no check name here is a ledger key.

🤖 Generated with Claude Code

https://claude.ai/code/session_012RSw4qMHS7ByE7PY8Ns4cs

commandprompt#1081 replaced a count of `entry->fileOffset != rg->fileOffset` with a per-field
membership loop, so dropping any one of the four compared fields reddens by name.
The loop matched `entry->$_f != rg->$_f` OR `entry->$_f != natts` for EVERY field,
because `natts` is the one compared against the scan's own column count rather
than against the row group.

Harmless today -- `entry->firstRowNumber != natts` appears nowhere -- and still
wrong as a claim: each arm would accept a comparison its own name denies. Three
fields now match only the `rg` form; `natts` has its own arm and its own name.

Reported by @jdatcmd reviewing commandprompt#1081.

ALL FOUR VERIFIED BY REMOVAL on the real suite, each reddening only its own arm:

    firstRowNumber removed   FAIL  ... firstRowNumber against the row group
    rowCount removed         FAIL  ... rowCount against the row group
    fileOffset removed       FAIL  ... fileOffset against the row group
    natts removed            FAIL  ... natts against the scan's column count
    restored                 26 passed, source byte-identical

FIRSTROWNUMBER IS THE ONE commandprompt#1081 SHIPPED UNPROVEN, and the reason is worth keeping.
Its mutation never applied: that line begins `(entry->` rather than `entry->`, so
the pattern missed, and the harness's applied-assertion REPORTED it instead of
letting a clean run count as a pass. A mutation that does not apply looks exactly
like an arm that does not fire.

Five majors: PG15 PG16 PG17 PG18 PG19, native_fetch_cache PASSED.

No ledger change: one check name is added and none removed, and the suite has zero
rows in the ledger -- it is one of the 249 uncovered suites.

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.

APPROVE at c13b931 once the two legs land. Verified each of the four arms
independently, and one correction to the framing.

Each field reddens only itself

Ran your arms' own case logic against pgcolumnar_fetch_row from main, one
mutation per field:

CLEAN                firstRowNumber:PASS  rowCount:PASS  fileOffset:PASS  natts:PASS
drop firstRowNumber  firstRowNumber:RED   rowCount:PASS  fileOffset:PASS  natts:PASS
drop rowCount        firstRowNumber:PASS  rowCount:RED   fileOffset:PASS  natts:PASS
drop fileOffset      firstRowNumber:PASS  rowCount:PASS  fileOffset:RED   natts:PASS
natts -> rg form     firstRowNumber:PASS  rowCount:PASS  fileOffset:PASS  natts:RED

The fourth row is the one this PR exists for: under the old alternation, changing
entry->natts != natts to the rg form left the arm green, because
entry->natts != rg->natts matched the first branch. Now it reddens.

The correction: main was never carrying a broken arm

firstRowNumber is the one #1081 shipped unproven. Its mutation never applied
in my matrix.

Your mutation missed; the arm did not. The arm matches a substring, and
entry->firstRowNumber != rg->firstRowNumber is a substring of line 4213's
(entry->firstRowNumber != rg->firstRowNumber || — the leading paren is invisible
to a case glob. Row 2 above is that field mutated on main's own source, and it
reddens.

So the finding is real and it is about your matrix, not about the tree:
#1081's arm was never demonstrated, and it works. That distinction is worth
keeping because "shipped unproven" and "shipped broken" call for different
responses, and only the first happened.

It does not reduce the value of the catch. An applied-assertion telling you a
case measured nothing is exactly the guard that should exist, and you found it the
way the stale-.pyc and the restore-assertion were found — by the instrument
reporting on itself rather than by a red.

The merge keeps #1082

merge-tree against current main gives 60a12bf with no conflict, and
fsst_margin.sh's codec arms survive it. Checked rather than read off
MERGEABLE, because the diff of this branch against the new main displays
#1082's additions as deletions — that is the base being older, not a revert, and
it is the display that would make someone nervous.

Small note on the comment

which is harmless -- entry->firstRowNumber != natts appears nowhere

True today, and the sentence is doing real work by saying WHY it was harmless
rather than just that it was. Worth keeping exactly as written.

@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.

APPROVE at c13b931. 14/14 green. The verification is in my comment above; this
is the formal approval.

The short version of what I ran rather than read: each of the four fields mutated
in turn against pgcolumnar_fetch_row, each reddening only its own arm, including
the case the old alternation let through — switching entry->natts != natts to
the rg form used to stay green and now reddens.

And merge-tree rather than MERGEABLE: merges to 60a12bf with no conflict,
and #1082's codec arms survive it.

Thank you for rewriting the "shipped unproven" section. The distinction is worth
the edit: one sends someone hunting a defect in main and the other says evidence
is owed, and only the second happened.

@jdatcmd
jdatcmd merged commit d959525 into commandprompt:main Sep 16, 2026
14 checks passed
jdatcmd added a commit that referenced this pull request Sep 16, 2026
CHANGELOG.md only, from #1079 and #1084 landing. All sides add to `### Fixed`.

    conflict markers left                0
    each entry present exactly once      6 checked by name, including #1084's
    bodiless headings in [Unreleased]    0

The suite file is untouched by the merge and the sorted check-name list still
hashes 62d1b8a49926, so no ledger key moves.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NhwXKAgSmYDUjteWkfajHK
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