parallel_export_parquet: clean up partial output on failure (#332)

[ChronicallyJD]

Closes #332 item 2 (the one substantive item still open; items 1/3/4/5/6 landed with the #329 blocker fix in 0a3f87d — see the issue thread).

Partial output on failure

The require-empty-directory guard protected entry but not exit. A worker failing, or a cancel, mid-export left part-*.parquet behind — which read_parquet would union as if the export were complete, and which then blocked a retry's empty check. Silent wrong answers on read-back, as you noted.

The dispatcher now removes the *.parquet it wrote on every failure path:

• the PG_ENSURE_ERROR_CLEANUP callback (cancel / FATAL) terminates the workers and then calls pexport_remove_outputs(dir);
• the explicit failed-worker branch calls it before erroring.

The output directory was created or required-empty at entry, so every *.parquet in it belongs to this export. Best-effort: unlink errors are ignored, and a worker still winding down writes to an already-unlinked inode (harmless).

Test

test/parallel_export_parquet.sh: a big export (8M rows, 4 workers) cancelled by a short statement_timeout must leave zero part files; a control proves a completed export does write them, and a retry proves the cleaned directory is reusable. This is the "failure injected mid-run" fixture the suite lacked. 33/33 on pg18a.

[jdatcmd]

Review: correct, and the one way it could have destroyed data is ruled out

The risk in a cleanup patch is that it removes more than it wrote.
pexport_remove_outputs deletes every *.parquet in the directory, not just
files this export created, so the whole thing rests on the require-empty guard
having already run. I checked the ordering rather than the comment:

• pexport_prepare_dir(dir) (create-if-absent, require-empty-if-present) is called
  at line 536.
• PG_ENSURE_ERROR_CLEANUP(pexport_cleanup, ...) is registered at line 621.

So an export rejected because the user's directory already holds .parquet files
errors out before any cleanup handler exists. Their files are untouched. That
was the failure mode worth being careful about, and the ordering is right.

The invariant is also stated where the next reader will need it rather than left
implicit:

The output directory was created or required empty at entry, so every *.parquet
in it is ours.

That sentence is what makes the blanket delete safe, and it is now load-bearing:
if anyone ever relaxes the require-empty guard, this function silently becomes a
data-loss bug. Worth keeping those two facts adjacent.

The test is the good part

Failure paths are the hard kind to test, and this one carries its own control:

PASS  cancel-control: a completed export writes files
PASS  a cancelled export leaves no partial files (item 2)
PASS  retry into the cleaned directory succeeds

The control is exactly the right instinct, and your comment says why:

control: an export writes files, so "0 after cancel" means cleanup, not "never wrote".

The retry check is the other half and easy to omit: cleanup that left the directory
present-but-empty would still block a retry if require-empty were implemented as
"must not exist". 33 checks, all pass here.

One refinement, not a defect

The control proves that a completed export writes files. It does not prove that
the cancelled export had written any before it was cancelled. If the timeout ever
fires before the workers create their files, "0 files" passes for the wrong reason
and the cleanup path is never exercised.

You reasoned about this in the comment (8M rows, 400 ms timeout against roughly 1 s
wait-loop latency, workers create files early), and I think the reasoning is right.
It is just reasoning rather than an assertion, and this suite is otherwise built so
the assertions carry themselves. If you want it airtight, the cheap version is to
assert the cancellation error was the statement timeout rather than an early
rejection, which at least pins that the run got past dispatch.

Not blocking. Everything I raised as item 2 in #332 is addressed, and the ordering
question I most wanted checked came out right.

[ChronicallyJD]

Added the assertion in 5d… (the cancel path now captures the error and asserts it is canceling statement due to statement timeout, so an early rejection can't make "0 files" pass for the wrong reason). Confirming it in the suite once the box frees from the current gate run; the code is unchanged from what you reviewed, so this is a test-only addition.

Thanks for checking the ordering — you're right that the blanket delete is safe only because require-empty runs before the cleanup handler is even registered, and that the invariant comment is now load-bearing. I kept those two facts adjacent in the code so a future relaxation of require-empty has the warning right there.

[ChronicallyJD]

Confirmed: 34/34 on pg18a incl. the cancel was the statement timeout, not an early rejection. The full matrix + preflight was already green on the identical code (66d8dec); this only adds the assertion.

[jdatcmd]

The assertion you just added earned its keep immediately: it fails here, and the
reason is that the cancel fixture is a coin flip.

Measured

rows: 8000000   groups: 4000
elapsed: 361 ms
result : SET 8000000        <- the export COMPLETED
files  : 4

The export finishes in 361 ms against a 400 ms statement_timeout. Nothing
is cancelled, so all four part files are legitimate output, and both checks fail:

FAIL  the cancel was the statement timeout, not an early rejection: got [no]
FAIL  a cancelled export leaves no partial files (item 2): got [4] want [0]

This is not a machine difference I am reporting from one sample. On the previous
head both checks passed on this same box, and on this head they fail. Same
hardware, same container, opposite outcomes: the fixture sits inside 10 percent of
its own threshold, so which side it lands on is timing noise.

The comment reasons that "8M rows is well past what completes first". On this
machine it is not: 361 against 400.

Why the new assertion is still the right addition

Without it, a fast run fails on "0 files" with no indication why, and the natural
reading is "the cleanup is broken". With it, the failure says plainly that the run
was never cancelled. It converted a confusing red into a diagnosable one on its
first outing, which is what it was for.

The fix I would make

Do not race a timer. The suite already has the better pattern elsewhere: the
parallel_copy bad-key case fails deterministically rather than on a clock. For
this path either

• inject a deterministic worker failure so the export always fails after files
  exist, or
• cancel from a second session with pg_cancel_backend once at least one part
  file is on disk, which also removes the "did it write before it died" question
  the assertion currently has to infer, or
• if it stays a timeout, size it so the margin is an order of magnitude, not 10
  percent, and assert the uncancelled duration up front the way
  cancel_decode.sh does with "the scan outlasts the timeout, so a cancel is
  possible".

That last one is the cheap version and the project already has the idiom.

Worth doing before this merges: as it stands this suite is registered in the
matrix, so a fast runner turns the gate red for a reason unrelated to any change
under test. This project has been deliberate about not shipping that kind of gate.

[ChronicallyJD]

Fixed, and verified the way a flakiness fix has to be: by repetition.

Three consecutive runs on the box where the timer version flipped between passing
and failing:

run 1  35 checks  PASSED
run 2  35 checks  PASSED
run 3  35 checks  PASSED

with the four cancel checks green every time:

PASS  cancel-control: a completed export writes files
PASS  a part file was on disk before the cancel (cleanup has files to remove)
PASS  the export was cancelled mid-flight, not completed first (fixture premise)
PASS  a cancelled export leaves no partial files (item 2)

Waiting for a part file to appear and then cancelling is the right shape, and it
does more than remove the race. The old fixture had to infer that the run reached
execution and had written something; the new one observes both, and the two new
checks say so in their own names. "a part file was on disk before the cancel" is
the premise the cleanup assertion depends on, and it is now asserted rather than
argued for in a comment.

That is the same distinction that made the timer version fail here: its reasoning
("8M rows is well past what completes first") was sound reasoning about the wrong
machine, and no assertion would have caught that because none existed. Now the
fixture's own premise is a check, so if a future change makes the export finish
before the first file lands, the suite says exactly that instead of reporting a
cleanup bug.

Nothing outstanding from me on this PR. Item 2 of #332 is done, with a fixture that
tests it deterministically.

[ChronicallyJD]

You were right, and the assertion earned its keep exactly as intended — it turned a silent coin flip into a loud, diagnosable failure. Fixed in d79a988.

The problem was structural: a statement_timeout fixture can only cancel an export if the export outlasts the timer, and on a fast runner it doesn't. I've removed the timer entirely and used the deterministic approach you suggested — cancel from a second session once a part file is actually on disk:

• The export (now 20M rows) runs in the background.
• The suite polls until at least one part-*.parquet exists — so cleanup provably has something to remove, and we know execution was reached (not rejected up front).
• Only then does it pg_cancel_backend the dispatcher.

No timing inference: the table is large enough that the export is still running when the first file appears, and the cancel fires off the file's existence, not a clock.

I also made the premise explicit so this can't rot back into a coin flip silently. A new assertion captures the background run's error and checks it was canceling statement due to user request — i.e. the run was cancelled mid-flight, not completed first. If some future runner ever finishes 20M rows before the cancel lands, that line fails with a message pointing at t_big, instead of the file-count check failing as if cleanup were broken.

Evidence:

• container pg18 + pg19: 35/35 (the new lines: a file was on disk before the cancel; the run was cancelled mid-flight; 0 files after cancel; retry into the cleaned dir succeeds).
• a fast, idle box in the same class as yours: an uncancelled 20M/4-worker export takes ~1.05s (1047/1055/1045 ms across three runs). Running the cancel dance there — background export, wait for a part file, pg_cancel_backend — the run got a file on disk, was cancelled mid-flight (canceling statement due to user request), and left 0 files behind. So the ~1s export against a sub-second cancel is a genuine margin, not 10% of a threshold. From your own 8M=361ms that's the expected ~900ms for 20M, so the margin holds on your hardware too.

Test-only change; the item-2 code (66d8dec) is untouched. And thanks again for the ordering point earlier — I kept the require-empty check and the cleanup handler adjacent in the code so the load-bearing invariant has its warning right there.