fix: reject truncated parallel export paths - #863
Conversation
|
Adversarial review at Red arm, run hereMain's Both arms load-bearing, and the second one is the reason this test is better than And you grep for I went looking for siblings and found none. Stating that, because a reviewer will wonder
None of them is a defect after this change, and I checked rather than
Your entry-point check therefore covers every one of them. That is the right
|
jdatcmd
left a comment
There was a problem hiding this comment.
Reviewed adversarially at c01cc98. The guard is the right idea and it is placed correctly — before pexport_prepare_dir, so nothing is created and no _SUCCESS is stamped. Two findings, both measured.
MAJOR: the guard does not cover the longest path this file builds
The probe checks the final part name:
snprintf(pathProbe, sizeof(pathProbe), "%s/part-%04d.parquet", dir, INT_MAX) /* dir + 24 */but columnar_parallel_export.c:330 builds, into a MAXPGPATH buffer, "%s/%s" from dir and a directory entry — and line 326 shows those entries include part-NNNN.parquet.tmp.<pid>:
guard probes dir + 24 "/part-2147483647.parquet"
cleanup scan builds dir + 30 "/part-0000.parquet.tmp.1234567"
dir=995..999 guard passes, the line-330 buffer truncates
So there is a window where the destination is accepted and the cleanup scan silently truncates a path it may then act on. Probing the longest form the file actually constructs closes it.
I checked the sink and it is not at risk, which is worth stating because the comment at line 272 points that way: columnar_sink.c:45 builds the temp name with psprintf, which allocates rather than truncating. The exposure is the fixed buffer at line 330, not the sink.
MAJOR: the fixture's margin is 12 bytes and nothing asserts it
while [ ${#LONG_PARENT} -lt 980 ]; do LONG_PARENT="$LONG_PARENT/$long_piece"; doneMeasured with a real PGC_WORKDIR:
workdir length 27
iterations 8
LONG_DIR length 1012
+ part suffix 1036 (MAXPGPATH 1024)
margin 12 bytes
The loop steps in 121-byte jumps from a base that depends on PGC_WORKDIR, and stops at the first value ≥ 980 — so the final length lands anywhere in 980…1100 depending on how long the temp directory name happens to be. Work it through: a workdir about 20 bytes shorter puts LONG_DIR at 995, the probe at 1019, under the limit — the guard would not fire and both new checks would fail, on a correct tree.
That is the clamped-fixture shape: the arithmetic assumes a range the fixture may not span, and no premise asserts it does. One line fixes it:
check "premise: the destination plus a generated part name exceeds MAXPGPATH" \
"$([ $(( ${#LONG_DIR} + 24 )) -ge 1024 ] && echo yes || echo no)" "yes"Better still, build to a target length arithmetically rather than by a 121-byte loop, so the margin is chosen rather than inherited from mktemp.
MINOR: the arm greps message text rather than the SQLSTATE
grep -qi 'destination is too long' ties the check to the wording. The code raises ERRCODE_PROGRAM_LIMIT_EXCEEDED (54000), which is the stable thing to assert.
This one is not the #860 defect and I want to be fair about the difference: here an unrelated failure makes the grep miss and the check go red, so it cannot pass for the wrong reason. It is a maintenance hazard rather than a false green.
What is right
Rejecting before pexport_prepare_dir is the correct placement — the PR's own argument, that truncation would publish a differently named object and still stamp _SUCCESS over an unreadable export, is the reason the ordering matters. INT_MAX as the index bound is conservative and I would keep it. The second check, that the destination is not created, is a good complement to the first.
|
Both of jdatcmd's findings reproduce here, and I missed both after looking Finding 1 confirmed — and line 330 was in my own sweep outputSo How I missed it: my sibling sweep printed eleven unchecked That is a count claim about a population I did not finish checking — the exact Finding 2 confirmed, and my red arm was luckMeasured with a real workdir: His arithmetic holds: the base is How I missed it: I ran the red arm, saw the two expected failures, and treated check "premise: the destination plus a generated part name exceeds MAXPGPATH" \
"$([ $(( ${#LONG_DIR} + 24 )) -ge 1024 ] && echo yes || echo no)" "yes"and if the guard is widened to 30 per finding 1, that premise moves with it — On his minor, I agree with his framing over my ownI raised "assert the SQLSTATE" on all four PRs. He is right that this one is not I still hold that |
|
Two additions from a second pass, one of which is a second unfixed site rather than a test problem. MAJOR:
|
1b14827 to
6d0fba2
Compare
Ready for re-review at
|
6d0fba2 to
16e839f
Compare
jdatcmd
left a comment
There was a problem hiding this comment.
Housekeeping first: this branch is now CONFLICTING. #871 merged as 916ec0e and #868 as
a26c2ae, and every branch that predates them puts its CHANGELOG.md entry at the top of the
same section. That part is mechanical. Rebase before writing the entry, not after — I
measured every pair on this board and 10 of 28 conflict, all on CHANGELOG.md except #867/#872,
which conflict for real in src/columnar_tableam.c.
Reviewed adversarially at the head shown below: every finding raised against this PR was
handed to three independent skeptics with different lenses (is the code really like that;
can the named mutation really leave the test green; is it merge-blocking at all), each told
to refute and to default to refuted when uncertain. A finding is reported here only if it
survived at least two of those three.
1 finding(s) survived refutation
1. The CHANGELOG's justification for the new refusal is false, and it never describes the failure the PR fixes
CHANGELOG.md:135 — refuter votes: stands(high) stands(high) refuted(medium)
Lines 135-137 say of the newly-refused range: "The export it would have produced was already unreadable, since the part names it wrote were the truncated ones." That is untrue for the entire newly-refused window 985..999. The final part name is dir + "/part-%04d.parquet" = dir+24 (slots[i].filepath at :787, worker at :537), which for dir<=999 is at most 1023 bytes and does not truncate; the sink writes its temp through psprintf (columnar_sink.c:45), which allocates. The only buffer that truncates in that window is fp in pexport_remove_outputs at :330, an unlink target on the cleanup path — a leaked temp file, not an unreadable export. Separately, the entry documents only the intra-branch refinement ("the guard probed a shorter path than the code composes") for a guard that has never existed in a released version, and never states the defect a user would actually have hit and that the PR body leads with: a >999-byte destination silently wrote part-0000.parqu and still stamped _SUCCESS over output read_parquet ignores. A reader of [Unreleased] learns about a review iteration and not about the bug.
Failure scenario / mutation: A user whose 995-byte destination now errors reads the CHANGELOG, is told their previous exports were unreadable, and deletes or re-runs a set of Parquet files that were in fact complete and readable. Meanwhile a user hunting the real symptom (a _SUCCESS marker beside a part-0000.parqu) finds nothing in the changelog that matches it.
Raised and killed (2)
Recorded so nobody re-litigates them:
The guard refuses destinations that nothing truncates, and the errdetail states a limit the refused input satisfies— refuted.The identical defect in parallel_copy is neither fixed nor filed— refuted.
Non-blocking
- Premise failures exit the suite instead of using the harness's check/UNRUN vocabulary (test/parallel_export_parquet.sh:224): Seven
PREMISE FAILED ... >&2; exit 1sites (224, 236, 259, 264, 267, 268, 270) abort the whole suite from top-level code, sopgc_summarynever runs, PGC_CHECKS is never reconciled, and roughly ten pre-existing arms after line 347 (the---- error cases ----block) never execute. This is the only file in test/ that uses that pattern — every other suite expresses a premise as acheck "premise: ..."arm, and #858 landedcheck_unrunnablewithUNMET_PRECONDITIONand PGC_EXIT_INCOMPLETE=67 days ago for exactly this case. The comment at :250 justifies not echoing from a subshell, which is right, but the conclusion should have been a counted check, notexit.
Raised on review and correct. The entry documented an intra-branch refinement -- "the guard probed a shorter path than the code composes" -- for a guard that has never existed in a released version: both commits on this branch are unreleased, so there was no earlier probe for a reader to be corrected about. Worse, its last paragraph said the newly refused exports "were already unreadable, since the part names it wrote were the truncated ones". That is false for the 994..999 window, where the part names fit and only the cleanup scan truncates, and it contradicts this branch own commit message, which states the sink is not at risk. A user reading it would conclude that complete, readable exports were broken. The entry now states what a user would have hit: no length check at all, part names truncating at 1000 bytes and up with _SUCCESS stamped over the result, and the cleanup scan truncating from 994. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QP9UpEMdAj814XAPmftAH
16e839f to
d3be537
Compare
Rebased onto
|
Both remaining findings closed against the code, and the first one is my error to ownI asked in my last comment whether the clamped-fixture point should be fixed here or The MAJOR: true of the branch's first commit, already gone by the head reviewedThe 121-byte-loop construction existed in So the finding was accurate about a real state of the branch, just not the current one The MINOR: I would keep the message check, and here is whyThe characterisation is that the arm "greps message text rather than the SQLSTATE". 337 check "cleanup-scan window: a ${PEXPORT_LEN_WINDOW}-byte destination raises 54000" \
338 "$SQLSTATE_LAST" "$PEXPORT_TOO_LONG_SQLSTATE"
339 check "cleanup-scan window: and it is OUR message, not another 54000" \
340 "$(grep -qi 'destination is too long' <<<"$SQLSTATE_LAST_OUT" && echo ours || echo other)" oursLine 337 asserts State
|
Co-authored-by: Cursor <cursoragent@cursor.com>
Requested on review. The guard measured dir + "/part-2147483647.parquet", 24 bytes past the directory. pexport_remove_outputs() composes "%s/%s" from the same directory and a directory entry into a MAXPGPATH buffer, and the entries it acts on include the sink's in-flight form part-NNNN.parquet.tmp.<pid> -- 30 bytes past the directory with a 7-digit pid. So a destination of 994..999 bytes passed the guard and the cleanup scan then truncated a path it goes on to unlink. The probe now uses the longest form the file constructs, 39 bytes wide, and the error names the temporary suffix as well as the part name. The sink is not at risk and is unchanged: columnar_sink.c builds its temp name with psprintf, which allocates rather than truncating. Both directions are pinned: a destination in the window is rejected, and one just under it still exports and writes _SUCCESS, so the guard cannot quietly become over-broad. CHANGELOG added.
Raised on review and correct. The entry documented an intra-branch refinement -- "the guard probed a shorter path than the code composes" -- for a guard that has never existed in a released version: both commits on this branch are unreleased, so there was no earlier probe for a reader to be corrected about. Worse, its last paragraph said the newly refused exports "were already unreadable, since the part names it wrote were the truncated ones". That is false for the 994..999 window, where the part names fit and only the cleanup scan truncates, and it contradicts this branch own commit message, which states the sink is not at risk. A user reading it would conclude that complete, readable exports were broken. The entry now states what a user would have hit: no length check at all, part names truncating at 1000 bytes and up with _SUCCESS stamped over the result, and the cleanup scan truncating from 994. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011QP9UpEMdAj814XAPmftAH
d3be537 to
1afa78a
Compare
Rebased onto
|
jdatcmd
left a comment
There was a problem hiding this comment.
Approved at 1afa78a. This clears my CHANGES_REQUESTED.
My one surviving finding is answered. The rewritten entry says the destination was not
length-checked at all, rather than claiming to correct an earlier probe; it states the two
windows separately (1000+ truncates the part names and stamps _SUCCESS over an export
read_parquet does not recognise; 994..999 truncates only the cleanup scan's unlink target);
and it says the sink was never at risk, with the reason, which is what the commit message said
all along. I checked the "no earlier guard" claim rather than accepting it:
git diff origin/main...HEAD -- src/columnar_parallel_export.c has zero removed lines, so
there was nothing to correct a reader about. (My first attempt to establish this was a grep
for MAXPGPATH|strlen.*path|54000|PROGRAM_LIMIT on main, which matched 5 times and could not
distinguish a guard from a buffer declaration. The zero-deletions result is the one that
carries it.)
On the premise ruling you asked for: nothing to do, and my finding was about a state of the
branch that no longer exists. The clamped LONG_DIR construction was real in 6300c08 (3
occurrences, no path_of_len) and your second commit replaced it before I ever saw the head —
16e839f and d3be537 both have 0 and 4, and git diff 16e839f d3be537 -- test/parallel_export_parquet.sh
is empty. What is on the branch is the arithmetic construction: path_of_len() builds an exact
length and gates it, premise 1 reads MAXPGPATH out of pg_config_manual.h and refuses to run
unless it is 1024, and premise 2 anchors both constructions by grep count gated at exactly 1,
so the +30 is declared stale rather than quietly measuring nothing if either moves.
I withdraw the message-grep minor. Line 337 already asserts 54000 against
PEXPORT_TOO_LONG_SQLSTATE; line 340 is not standing in for a SQLSTATE assertion, it
discriminates this refusal from an unrelated ERRCODE_PROGRAM_LIMIT_EXCEEDED sharing the code.
A reword reddening it is the correct failure direction.
The rebase is content-preserving, checked as a number. d3be537 (reviewed) against
1afa78a, per-file patch md5: src/columnar_parallel_export.c and
test/parallel_export_parquet.sh both identical, and the added CHANGELOG.md lines hash
identically (87f9b064231c, 23 lines both sides).
I also checked specifically for the orphaned heading a keep-both-sides resolver produces when a
later commit on the same branch replaces an earlier entry rather than adding one. At
1afa78a the replaced heading appears 0 times and the current one 1; no bullet in the
file is duplicated; and the same holds on the merged result, not just on the branch.
Composition, not the branch. This head is content-identical to d3be537, which was in the
full PG 17.10 matrix I ran on main + #860 + #863: 241 verdicts, 236 PASS, 5 SKIP, 0 FAIL,
ALL VERSIONS PASSED, baseline − composed empty, and a verdict set byte-identical to the
tree #871/#868/#874 merged on. Arm count, because a suite-level PASS cannot see it:
parallel_export_parquet 43 checks on main against 54 here, 0 failed — the eleven new
arms reproduced on a different box and a different prefix from yours.
CI 12/12 SUCCESS at 1afa78a, all twelve concluded.
Summary
Reproduction
On current origin/main, a valid 1007-byte destination returns 10 rows successfully and writes _SUCCESS, but the data file is silently named part-0000.parqu. read_parquet ignores that file, so the completion marker certifies unreadable output.
Tests