fix: reject truncated parallel export paths

[OffgridwithJD]

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]

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]

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]

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]

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.