Skip to content

test: row_tid refuses a row number below 1, and without the floor row_tid(0) crashes the backend (#1341) - #1391

Merged
jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-row-tid-floor
Oct 15, 2026
Merged

jdatcmd merged 2 commits into
commandprompt:mainfrom
OffgridwithJD:test/1341-row-tid-floor

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Part of #1341. pgcolumnar.row_tid refuses a row number below 1 (22003, "Row numbers start at 1"), and nothing tested it. This adds the arm in both twins.

The floor guards a crash, not just a wrong value

With the floor removed (if (false && rowNumber < 1)), row_tid(0) crashes the backend (PG17). The server goes into recovery, and in the first draft, where the arm sat in the middle of the rollup suite, the checks after it failed too, one of them a premise: row. So the arm now runs:

  • last in rollup.sh, and
  • as a pytest test of its own, last in test_rollup.py (test_row_tid_refuses_a_row_number_below_one).

That way a crash takes nothing else down. Under the mutant, the shell is 82 passed + 1 failed (only this arm), and pytest fails only this test (got '?|?|?'). The teardown error is the crash itself.

Evidence

  • Clean: rollup 83/83, and test_rollup.py 2 passed, on each of PG15–19.
  • Red first (PG17, floor removed): shell got [||] want [22003|22003|ok]; pytest got '?|?|?' want '22003|22003|ok'.

Bookkeeping

  • Ledger: one row, dated by that mutation. 2460 + 1 = 2461; never 1893; dated 568. Ceiling 244, unchanged. The gate is clean against origin/main.
  • cluster_tests 539 → 540 (collection +1 against main). guard_tests is 429/429. A TESTS.md row is added.

Not needed: the other half of this #1341 item

The partition_drop_plan NULL cutoff (22023) in both overloads is already asserted at partition_lifecycle.sh:320 and in its twin. It landed with #1350, after the audit was written.

🤖 Generated with Claude Code

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

Reproduced at d16b9dd on PG18:

run shell rollup test_rollup.py
this PR 83/83 2 passed
floor removed (if (false && rowNumber < 1), src/columnar_storage.c:685) 82 + 1 failed: `got [

Only this arm turns red, in both twins, so placing it last does what the PR says. The extra TESTS.md text for test_rollup_phase_one ("each group's TID range holds exactly its rows") describes a check that already exists at test_rollup.py:271. It fixes an omission.

Approving. This PR needs a rebase: main is now at f4a7591, after #1388 (ledger 2464 / 1895 / 569, cluster_tests 541) and #1390. Once it's rebased and CI is green, I'll check the patch against d16b9dd and merge.

🤖 Generated with Claude Code

@OffgridwithJD

OffgridwithJD commented Oct 15, 2026 •

Copy link
Copy Markdown
Collaborator Author

Rebased onto 57f6e0e (#1401), on top of #1403. The new head is 3c88cbc. Only the count files changed, so this needs your re-approval.

  • Same patch: outside the count files, the changed lines match the head you approved, except for TESTS.md section renumbers where main's new sections took the old numbers.
  • The ledger is restated on main's census and agrees with its tsv: 2787 rows, 2095 never, 692 dated, read back from the file. Each changed paragraph's arithmetic starts from main's last numbers.
  • cluster_tests 567, by collection over the cluster files on this tree. guard_tests 433 is unchanged.
  • Runs on the rebased tree at 217f23b, PG17 and PG18: PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG17 pytest: 4 passed in 32.92s; PG17 rollup: 105 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped; PG18 pytest: 4 passed in 30.22s; PG18 rollup: 105 passed + 0 failed + 0 unrunnable + 0 skipped. harness_selftest 1245/1245 and docs_style passed on both. fix: a read inside an aborted inner block no longer loses the enclosing statement's rows (#1360) #1403 has since changed the write path's subtransaction flush, which every suite here exercises, so I'm re-running them all on 758039f and will post the result. CI runs PG17 and PG18 on this head.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Re-run finished on the tree after #1403 (758039f or later). Everything is green, so the head above stands:

  • PG17 docs_style: passed
  • PG17 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG17 pytest: 4 passed in 23.30s
  • PG17 rollup: 105 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 docs_style: passed
  • PG18 harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • PG18 pytest: 4 passed in 22.18s
  • PG18 rollup: 105 passed + 0 failed + 0 unrunnable + 0 skipped

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-row-tid-floor branch from 3c88cbc to 89762ba Compare October 15, 2026 17:06
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto 7da848d (#1432, #1409). The new head is 89762ba; only the count files changed, so please re-approve this head.

🤖 Generated with Claude Code

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

Re-approved at 89762ba. Compared with d16b9dd, the head I approved, file by file: the changed lines are the same, apart from the count files (check_ledger_budget.txt, expected_tests.txt) and, in #1378/#1379/#1380/#1383, the TESTS.md section number, now 115.

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-row-tid-floor branch from 89762ba to 857ef8a Compare October 15, 2026 17:52
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto 1f7b5da (#1411, #1412, #1417, #1418). The new head is 857ef8a. Only the count files changed, plus TESTS.md section numbers where main took 115, so please re-approve this head.

🤖 Generated with Claude Code

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

Re-approved at 857ef8a, the rebase onto 1f7b5da. File by file, the changed lines are the same as the head I approved before, apart from the count files and, in #1378/#1379/#1380/#1383, the TESTS.md section number.

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

The PG18 re-run on this head (857ef8a, on 1f7b5da with #1412's index-build change) is all green:

  • docs_style: passed
  • harness_selftest: 1245 passed + 0 failed + 0 unrunnable + 0 skipped
  • pytest: 4 passed in 17.89s
  • rollup: 105 passed + 0 failed + 0 unrunnable + 0 skipped

🤖 Generated with Claude Code

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto 37ea557 (#1414, #1425). The new head is 56edb7d. Only the count files changed, so please re-approve this head.

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-row-tid-floor branch from 857ef8a to 56edb7d Compare October 15, 2026 18:28
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto f2bd5f1 (#1427). The new head is 95d5c70. Only the count files changed, so please re-approve this head.

🤖 Generated with Claude Code

@OffgridwithJD
OffgridwithJD force-pushed the test/1341-row-tid-floor branch from 56edb7d to 95d5c70 Compare October 15, 2026 18:38

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

Re-approved at 95d5c70, the rebase onto f2bd5f1. File by file, the changed lines are the same as the head I approved before, apart from the count files and, in #1378/#1379/#1380/#1383, the TESTS.md section number.

🤖 Generated with Claude Code

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