Skip to content

fix: reject truncated parallel export paths - #863

Merged
jdatcmd merged 3 commits into
mainfrom
audit/parallel-export-path-length
Sep 2, 2026
Merged

fix: reject truncated parallel export paths#863
jdatcmd merged 3 commits into
mainfrom
audit/parallel-export-path-length

Conversation

@OffgridwithJD

Copy link
Copy Markdown
Collaborator

Summary

  • reject parallel-export destinations whose generated part path cannot fit in MAXPGPATH
  • validate before creating the destination, so a failed call leaves no misleading output
  • cover the prior silent success that wrote part-0000.parqu and still stamped _SUCCESS

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

  • test/parallel_export_parquet.sh /usr/bin/pg_config (PostgreSQL 18.6, Ubuntu 26.04): 45 passed, 0 failed
  • red arm on origin/main: success return plus _SUCCESS and truncated part-0000.parqu reproduced

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Adversarial review at c01cc98. This is the strongest of the four, and the
test is the best one you have written today.

Red arm, run here

Main's src/ plus only this PR's test file:

exit=1   PASS=43  FAIL=2
  FAIL  reject a destination whose generated part path would truncate: got [ok] want [error]
  FAIL  long destination is rejected before it is created: got [created] want [absent]

Both arms load-bearing, and the second one is the reason this test is better than
the others: it does not merely assert that something failed, it asserts the
second property the fix claims — that the destination is not created. A fix
that rejected after pexport_prepare_dir would pass arm 1 and fail arm 2.

And you grep for destination is too long rather than accepting any error. That
is the SQLSTATE point I raised on #860, #861 and #862, solved a different and
equally good way. Do this in the others.

I went looking for siblings and found none. Stating that, because a reviewer will wonder

columnar_parallel_export.c builds the same path shape at three more places
that do not check snprintf's return:

:306  snprintf(fp, sizeof(fp), "%s/part-%04d.parquet", dir, i)          cleanup
:537  snprintf(fp, sizeof(fp), "%s/part-%04d.parquet", hdr->dirpath, i) worker
:760  snprintf(slots[i].filepath, ..., "%s/part-%04d.parquet", dir, i)  dispatch
:411  snprintf(fp, sizeof(fp), "%s/_SUCCESS", dir)                      marker

None of them is a defect after this change, and I checked rather than
assumed:

  • hdr->dirpath is strlcpy(hdr->dirpath, dir, sizeof(hdr->dirpath)) at :740,
    so it is the validated dir.
  • _SUCCESS is 9 bytes against the part suffix's 23 at INT_MAX, so the longer
    construction dominates it.

Your entry-point check therefore covers every one of them. That is the right
design — validate once where the value enters — and it is worth one sentence in
the code saying so, because right now the precondition is implicit. A reader at
:537 has no way to know why that snprintf needs no check, and the next person
to add a caller that bypasses the entry point will not either.

INT_MAX is conservative, deliberately or not

The probe reserves ten digits for the part index. The real index is bounded by
PEXPORT_MAX_WORKERS and the max_parallel_workers budget, so part-0031 is
nearer the truth than part-2147483647 — about six bytes of over-strictness. It
errs toward refusing paths that would have worked, which is the right direction,
and using the type's maximum rather than a runtime cap means the check cannot go
stale when the cap changes. If that was deliberate, say so in the comment; if it
was not, it is still the choice I would make.

Missing, and it is the fourth time today

No CHANGELOG.md, no docs. This makes a previously-succeeding call error.
docs/sql-reference.md documents parallel_export_parquet and says nothing
about a destination length limit, so there is no sentence a user could read to
predict this.

Across #860, #861, #862 and #863 the same two gaps repeat: no CHANGELOG or docs
on a behaviour change
, and deny arms that assert failure rather than the
reason
. #863 fixes the second one. Worth making both habits rather than
per-PR review findings.

Not approving — same account, and that reads as self-approval whoever typed it.
jdatcmd's call.

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

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"; done

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

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Both of jdatcmd's findings reproduce here, and I missed both after looking
directly at the evidence for one of them.
Recording that plainly, because how I
missed it is more useful than the confirmation.

Finding 1 confirmed — and line 330 was in my own sweep output

guard probes            "/part-2147483647.parquet"          24 bytes
line 330 builds         "%s/%s" from dir + a directory entry
line 326 admits          *.parquet  AND  *.parquet.tmp.*
longest such entry      "/part-0000.parquet.tmp.1234567"    30 bytes

So dir lengths where dir+24 < 1024 <= dir+30 pass the guard and truncate at
330. Real.

How I missed it: my sibling sweep printed eleven unchecked snprintf sites,
six in this file — 306, 311, 330, 335, 411, 537, 760. I then verified 306,
411, 537 and 760, found each dominated by the new entry check, and wrote "none
of them is a defect after this change."
I verified four of six and reported a
conclusion about all of them. 330 is one of the two I did not open, and it is the
one that is not dominated.

That is a count claim about a population I did not finish checking — the exact
shape I have flagged in other people's work three times today. The sweep was
right; my coverage of it was not, and nothing in what I wrote said which sites I
had actually read.

Finding 2 confirmed, and my red arm was luck

Measured with a real workdir:

PGC_WORKDIR        /tmp/pgcolumnar-test.rEbKVN   (27 bytes)
loop iterations    8       (121-byte steps from a 37-byte base)
LONG_DIR           1012
+ part suffix      1036    vs MAXPGPATH 1024
margin             12 bytes
guard fires?       YES — here

His arithmetic holds: the base is PGC_WORKDIR + /long_path, the loop adds 121
per step and stops at the first length ≥ 980, so the final value lands wherever
that lattice happens to cross. A shorter mktemp name shifts LONG_DIR below
1000, dir+24 drops under 1024, the guard does not fire, and both new checks
fail on a correct tree.

How I missed it: I ran the red arm, saw the two expected failures, and treated
that as evidence the fixture was sound. A red arm that depends on an unasserted
margin proves the fixture worked this time. That is the clamped-fixture shape,
and the premise he suggests is the fix:

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 —
which is an argument for computing the suffix length once and using the same
constant in the code and the fixture.

On his minor, I agree with his framing over my own

I raised "assert the SQLSTATE" on all four PRs. He is right that this one is not
the #860 defect: here an unrelated failure makes the grep miss and the check go
red, so it cannot pass for the wrong reason. Maintenance hazard, not false
green. That distinction is worth keeping — I had been applying the rule without
it.

I still hold that 54000 is the more stable assertion, and that greping the
message and the SQLSTATE costs nothing.

@jdatcmd

jdatcmd commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Two additions from a second pass, one of which is a second unfixed site rather than a test problem.

MAJOR: parallel_copy has the identical defect, unfixed

src/columnar_parallel_copy.c:1630 validates and splits the file at the full user path, then strlcpy()s it into hdr->filename[MAXPGPATH], and the loaders open the truncated name. A destination over 1023 bytes therefore behaves exactly as parallel export did before this PR: accepted, silently truncated, work done against a path nobody asked for.

The fix here is right and the sibling entry point has the same hole. Worth closing in the same change while the reasoning is fresh, or filing so it does not wait for someone to hit it.

MAJOR: the guard is now over-strict at the top of the range

Probing with INT_MAX reserves 24 bytes for /part-2147483647.parquet, but the real index is bounded by the worker or partition count and is written %04d. Destinations roughly 1000-1005 bytes long are now rejected even though their generated part path fits in MAXPGPATH and exports correctly on main.

That is a behaviour change the PR body does not mention: it says "reject truncated parallel export paths", and it also rejects some that would not truncate. Either bound the probe by the actual maximum index, or say in the message and the body that the limit is conservative by design.

Both of these sit either side of my earlier point that the probe under-covers the cleanup path at line 330 (dir + "/" + d_name, where d_name can be part-NNNN.parquet.tmp.<pid>, 30 bytes). Taken together: the guard is too strict at the top of the range and too loose for the longest name the file actually builds, which is one probe expression away from being right in both directions.

@OffgridwithJD
OffgridwithJD force-pushed the audit/parallel-export-path-length branch from 1b14827 to 6d0fba2 Compare September 2, 2026 02:20
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Ready for re-review at 6d0fba2 — 12/12 green, rebased onto main

Your ask is addressed and the branch has been rebased onto 53224e4, because #870
landed and made this PR conflict:

49cea2b  fix: reject truncated parallel export paths        (original)
6d0fba2  fix: probe the longest path this file builds       (your review)
   ↑ both after a rebase; the CHANGELOG was the only conflict, one hunk, both entries kept

The rebase is content-preserving and verified as such: the patch outside
CHANGELOG.md is byte-identical to the pre-rebase patch, and the 19 CHANGELOG
lines the branch adds are the same 19.

Your finding, restated as I fixed it. 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 part-NNNN.parquet.tmp.<pid> — 30 bytes past the directory with a
7-digit pid. A destination of 994 to 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 itself 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, so the guard cannot quietly become over-broad: a
destination inside the window is rejected, and one just under it still exports and
writes _SUCCESS. I added the CHANGELOG entry myself rather than ship the docs gap
I keep flagging on other PRs.

Gates. Actions was down from 20:54Z to about 01:00Z, which is why this PR
displayed a tick earned by an older commit. This head is 12 of 12 green.
Locally at 6d0fba2: parallel_export_parquet 54/54 on pg18a and pg19a,
docs_style clean.

MERGEABLE and green, still a draft.

@OffgridwithJD
OffgridwithJD marked this pull request as ready for review September 2, 2026 02:32
@OffgridwithJD
OffgridwithJD force-pushed the audit/parallel-export-path-length branch from 6d0fba2 to 16e839f Compare September 2, 2026 13:54

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

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 1 sites (224, 236, 259, 264, 267, 268, 270) abort the whole suite from top-level code, so pgc_summary never 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 a check "premise: ..." arm, and #858 landed check_unrunnable with UNMET_PRECONDITION and 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, not exit.

OffgridwithJD pushed a commit that referenced this pull request Sep 2, 2026
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
@OffgridwithJD
OffgridwithJD force-pushed the audit/parallel-export-path-length branch from 16e839f to d3be537 Compare September 2, 2026 15:56
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto a26c2ae at d3be537, and your CHANGELOG finding was right — the entry is rewritten

Your finding stands and I am not going to argue any part of it. The 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, and
worse, it contradicts this branch's own commit message, which says in as many
words that the sink is not at risk because columnar_sink.c builds its temp name
with psprintf. I had the correct mechanism three paragraphs earlier in the same
entry and then wrote its opposite in the summary line.

Your second half is the one I would have defended and should not have. I checked
before rewriting: git diff origin/main...HEAD -- src/columnar_parallel_export.c
shows the guard block as entirely additions. There was no length check in
pgcolumnar_parallel_export_parquet at all before this PR, so both commits here are
unreleased and there is no earlier probe for a reader to be corrected about. The
entry documented my own review iteration and a released-version reader would learn
nothing about the defect.

The entry now describes what a user would have hit

Two failures, and I separated them because they have different windows:

  • 1000 bytes and up — the part names themselves truncate. The run writes
    part-0000.parqu, stamps _SUCCESS beside it, and reports success for an export
    read_parquet does not recognise.
  • 994 to 999 bytes — part names fit; the path pexport_remove_outputs composes
    for its cleanup scan does not, so the scan unlinks a truncated path and leaves the
    temporary file behind.

It also states plainly that the sink was never at risk. Commit d3be537, titled
for what it is: describe the defect, not the review iteration.

Gates on the rebased head

parallel_export_parquet   54 passed + 0 failed = 54   on pg18a AND pg19a
docs_style                 9 checks, exit 0

The two majors are distinguishable in my log by check count rather than by
ordering, since I ran them concurrently — 54 is this suite, 33 is #860's.

Your other two, for the record

The MAJOR premise point — that LONG_DIR is built by a 121-byte loop from a
mktemp base, so the final length lands anywhere in 980..1100 and a shorter workdir
would put the probe under the limit and fail both new checks on a correct tree — I
have not addressed in this push. It is right, it is the clamped-fixture shape, and
it deserves the arithmetic construction you suggest rather than the one-line premise
assert. Say whether you want it in this PR or as a follow-up and I will do it either
way; I did not want to bundle a fixture rewrite into a docs correction without
asking.

The SQLSTATE-versus-grep point I agree with and I note you distinguished it
correctly from the #860 defect: here a miss makes the arm red rather than green, so
it is a maintenance hazard and not a false pass. Also happy to take that.

And thank you for the correction on your own #860 review re q() versus
psql_run — you did not have to go back and say that.

@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Both remaining findings closed against the code, and the first one is my error to own

I asked in my last comment whether the clamped-fixture point should be fixed here or
as a follow-up. That question should never have been asked: it was already fixed,
and I answered a review of my own branch from the review's description of it instead
of reading the file.
That is precisely the failure this thread has spent a day
cataloguing, committed by me, about my own code.

The MAJOR: true of the branch's first commit, already gone by the head reviewed

                            LONG_DIR   path_of_len   MAXPGPATH premise
6300c08 (branch commit 1)      3            0               0
16e839f (the head reviewed)    0            4               1
d3be537 (current head)         0            4               1

The 121-byte-loop construction existed in 6300c08 and my second commit replaced it.
git diff 16e839f d3be537 -- test/parallel_export_parquet.sh is empty, so the
CHANGELOG rewrite did not touch the fixture: what is there now is what was there when
the review was written. The arithmetic form gates its own output
(PREMISE FAILED: built a ${#p}-byte path, wanted $want), derives 984/985/994 from
MAXPGPATH read out of pg_config_manual.h and refuses to run unless it is 1024, and
anchors both constructions by grep -c gated at exactly 1.

So the finding was accurate about a real state of the branch, just not the current one
— and I could have said so in one command instead of asking for a ruling.

The MINOR: I would keep the message check, and here is why

The characterisation is that the arm "greps message text rather than the SQLSTATE".
It does not replace a SQLSTATE assertion; it sits next to one:

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)" ours

Line 337 asserts 54000. Line 340 exists because 54000 is
ERRCODE_PROGRAM_LIMIT_EXCEEDED and plenty of other things raise it — the arm
distinguishes our refusal from an unrelated limit that happens to share the code. That
is the pair, not a substitute. Your own review already reached the right conclusion
about its risk: a reword makes it go red, so it cannot pass for the wrong reason.
I would rather it reddened on a reword than silently accepted somebody else's 54000,
so I am leaving it and saying so here rather than quietly not doing it.

State

d3be537, MERGEABLE against ce44f11 — measured after #874 landed, not assumed. The
fixture, the SQLSTATE assertions and the CHANGELOG rewrite are all at that head.

OffgridwithJD and others added 3 commits September 2, 2026 16:08
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
@OffgridwithJD
OffgridwithJD force-pushed the audit/parallel-export-path-length branch from d3be537 to 1afa78a Compare September 2, 2026 16:10
@OffgridwithJD

Copy link
Copy Markdown
Collaborator Author

Rebased onto ee0602c at 1afa78a — and the md5 check you asked for caught a real defect in my own rebase

Content preservation, in the form you used on #860 rather than the prose I had been
writing:

src/columnar_parallel_export.c        old=a00837eef572  new=a00837eef572  IDENTICAL
test/parallel_export_parquet.sh       old=e49de93afdd7  new=e49de93afdd7  IDENTICAL
CHANGELOG.md ADDED lines              old=87f9b064231c  new=87f9b064231c  IDENTICAL
conflict markers                      0

It did not read like that on the first attempt

The two source files matched immediately. The CHANGELOG added-lines hash did not
87f9b064231c against ad23e95c56c6, 27 lines against 29. The two extra lines
were the heading of the entry the docs commit had replaced:

212  - A parallel export destination is measured against the longest name the export
213    actually builds, not against the final part name (#863).
214  - A parallel export refuses a destination too long to hold the names it generates

My conflict resolver keeps both sides of a CHANGELOG conflict, which is right
when two branches each add an entry and wrong when one commit on the same branch
replaces another's. This branch now has both shapes in it, so the rebase replayed
the fix commit's original entry, then hit the docs commit's replacement and kept the
orphaned heading alongside the new text. The result was a stray two-line bullet
introducing an entry that then says something different.

That would have shipped. It is not a merge artefact anyone would notice in a green
suite — docs_style passes on it, because a duplicated bullet is well-formed
markdown. The hash is what caught it, which is exactly the argument you made for
using a number instead of a reading, and I would not have caught it with the
"added lines identical, context moved" prose I used on #871.

Gates at 1afa78a

parallel_export_parquet   54 passed + 0 failed = 54   pg18a
parallel_export_parquet   54 passed + 0 failed = 54   pg19a
docs_style                 9 checks, exit 0

Your 54 on a different box and prefix reproduces mine, and the arm count is the thing
a suite-level PASS cannot see — 43 on main against 54 here is the eleven new arms
actually running.

Lease read from GitHub (d3be537), not from a local ref, for the reason that bit me
earlier: a fetch refspec without + silently declines to update a force-pushed
remote-tracking ref, so a local value can be stale without saying so.

Ready when CI is.

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

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.

@jdatcmd
jdatcmd merged commit 381c765 into main Sep 2, 2026
12 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