Skip to content

fix: do not divide parallel custom-scan I/O by worker count, with the leader arm covered (#1065, rebased + coverage) - #1094

Merged
jdatcmd merged 5 commits into
commandprompt:mainfrom
OffgridwithJD:review/1065-cover-the-leader-arm
Sep 17, 2026
Merged

jdatcmd merged 5 commits into
commandprompt:mainfrom
OffgridwithJD:review/1065-cover-the-leader-arm

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

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

The C is legitimate and correct

pgcolumnar_parallel_divisor reproduces core's get_parallel_divisor (costsize.c) faithfully, and separating I/O from CPU is the right fix — core's seqscan divides CPU and leaves the disk run whole. The path->rows change is correct and belongs with it. Nothing to argue with in the code.

The defect was in the tests, and it hid the main branch of the copied heuristic

Both suites set parallel_leader_participation = off, and that GUC defaults ON. With it off the divisor is exactly the worker count, so this arm —

if (parallel_leader_participation) { leader_contribution = ...; parallel_divisor += ...; }

— the entire reason the function is not a one-line division, never executed in either harness. The copied heuristic was covered only in the configuration nobody runs and untested in the one everybody does. Same shape as setting-a-guc-is-not-engaging-the-path: the setting is present, so the coverage looks real, and the branch is dark.

What I added

Both suites now measure the same property a second time with the leader participating, and prove the branch ran rather than asserting it did. The row estimate is the observable: the partial path divides rel->rows by the same divisor, so leader-on and leader-off cannot agree.

Measured on PG17, two workers, 40,000 rows:

rows:   leader off = 20000     leader on = 16667      (40000/2 against 40000/2.4)
ratio:  leader off = 1.070     leader on = 1.083      (bound is 1.35)

If the leader-participation branch changes the divisor ever reports same, the branch did not run and every assertion after it is about the wrong divisor.

The suites stay independent: the shell arm reads the plan text, the pytest twin walks FORMAT JSON, neither invokes the other, and the names match so compare_to_bash grades them one-for-one.

Verified

parallel_scan_cost.sh          13/13 on PG15/16/17/18/19
test_parallel_scan_cost.py     13/13 against a real cluster on PG17
compare_to_bash                128/128 one-for-one
shellcheck -S error            clean
ledger 1243 rows uniform 15;16;17;18;19, census 1235, GATE rc=0

Ledger rows re-derived from those runs, not by editing field 4. Their rows read 15;16;17;18 while every other row reads 15;16;17;18;19, which the release gate refuses even though CI is green (ci.yml:503).

Merge order

Takes TESTS.md section 44, correct while main is at 43. All five take 44; the numbering is gated, so the second to land renumbers and re-derives its census — the conflict offers two numbers and neither is right (#996).

Suggested order: #1092 (their #1063) first, then #1094/#1095, then this, then #1093.

Original: #1065. Author: @linuxhikerpm.

jdatcmd and others added 4 commits September 17, 2026 00:05
Core seqscan divides CPU across workers and leaves disk I/O whole. The
partial columnar path divided the entire run by workers, so an
I/O-dominated scan was quoted at half its serial cost with two workers.

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>
…t#1065 review)

The C is right: pgcolumnar_parallel_divisor reproduces core's get_parallel_divisor
faithfully, and separating I/O from CPU is the correct fix. The defect is in the
tests.

BOTH SUITES SET parallel_leader_participation OFF, AND THAT GUC DEFAULTS ON. With
it off the divisor is exactly the worker count, so the

    if (parallel_leader_participation) { leader_contribution ... }

arm -- the entire reason the function is not a one-line division -- never
executed in either harness. The copied heuristic was covered only in the
configuration nobody runs, and untested in the one everybody does.

Both suites now measure the same property a second time with the leader
participating, and prove the branch ran rather than asserting it did. The row
estimate is the observable: the partial path divides rel->rows by the same
divisor, so leader-on and leader-off cannot agree. Measured on PG17, two
workers, 40,000 rows:

    rows: leader off=20000   leader on=16667      (40000/2 against 40000/2.4)
    ratio: leader off=1.070  leader on=1.083      (bound is 1.35)

If "the leader-participation branch changes the divisor" ever reports "same",
the branch did not run and every assertion after it is about the wrong divisor.

The suites remain independent: the shell arm reads the plan text, the pytest twin
walks FORMAT JSON, and neither invokes the other. Assertion names match so
compare_to_bash grades them one-for-one -- 128/128.

Verified: shell 13/13 and pytest twin 13/13 on PG17, shell 13/13 on all five
majors. Ledger rows re-derived from those runs rather than by editing the majors
field: 1243 rows, census 1235, gate rc=0.

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.

The divisor is a faithful copy and I checked it against core rather than from memory. Diffed against get_parallel_divisor in src/backend/optimizer/path/costsize.c of a real PostgreSQL source tree: same initialisation, same parallel_leader_participation guard, same 1.0 - (0.3 * parallel_workers), same > 0 test, same accumulation. Logic-identical.

Separating I/O from CPU is right, because core's seqscan divides CPU and leaves the disk run whole.

The coverage fix closes a real gap. On #1065 both harnesses set parallel_leader_participation off and nothing set it back, so the if (parallel_leader_participation) arm at columnar_customscan.c:2531 never executed. The GUC defaults ON, so the only covered configuration was the one nobody runs.

The new arm is a genuine differential rather than a GUC flip: check "the leader-participation branch changes the divisor" asserts rows_off != rows_on, so it fails if the branch is inert. That is the part that makes the coverage real.

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

…-leader-arm

# Conflicts:
#	CHANGELOG.md
#	test/check_ledger.tsv
#	test/check_ledger_budget.txt
#	test/pytest/TESTS.md
#	test/pytest/expected_tests.txt
#	test/pytest/test_compare_to_bash.py
@jdatcmd
jdatcmd merged commit b7eb875 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