parallel_copy can refuse a load it has already taken (#403 item 7)

[jdatcmd]

Item 7 of #403. Uses the alpha3 upgrade script #812 opened.

The defect, measured first

load 1 returned 100000, table holds 100000
load 2 returned 100000, table holds 200000

A load that commits, whose acknowledgement the client never receives, is retried and the rows go in twice.

The unit is the whole load, not a "part"

The paper keeps hashes of the last N inserted parts, because there each part commits on its own. parallel_copy is atomic through 2PC — proved, not assumed, with a malformed integer at line 50,001:

rows before = 0
ERROR: worker 1 failed: invalid input syntax for type integer: "not-an-int"
rows after  = 0        prepared transactions left: 0

Parts never commit independently here, so a part hash would deduplicate nothing that is not already all-or-nothing.

My first attempt to prove that failed to fire. I set pgcolumnar.sink_fail_after and the load succeeded anyway, because the injection GUC never reaches the background workers. That would have been a clean-looking pass proving nothing.

Where the check goes, and what it costs

After every loader has PREPAREd and before anything commits. That is the only point at which a repeat can be refused without charging every load for it, and 2PC is what makes it possible: the work can be thrown away after it is done.

So a refused load does its work and discards it. The alternative — hashing before dispatch — costs 42% of the load on a 264 MiB file (1.13 s across four workers against 0.48 s single-threaded SHA-256). As implemented the coordinator hashes while the loaders are already reading the same file:

min-of-8, 5,000,000 rows asserted per rep
dedup off  1.09 s
dedup on   1.08 s

No measurable cost. The arms' ranges overlap almost entirely on this contended box, so the honest claim is that the overhead is not resolvable here — which is the result, given the 42% alternative is resolvable.

The order of the two writes is the safety argument

Data commits first; the fingerprint is recorded after. A crash between them leaves data with no fingerprint, so a retry loads again — the behaviour without this feature, and the safe direction. The reverse would leave a fingerprint with no data and refuse rows that were never stored.

A defect my own removal proof found

Running the mutation, the record insert hit duplicate key value violates unique constraint after the data had committed — reporting failure for a load that succeeded. Two concurrent loads of one file reach that state with no mutation at all: both check before either records.

So the index is not unique. The lookup is an existence test, a duplicate record is harmless, and a false failure is not. Concurrent identical loads both store, which is the behaviour without dedup and the safe direction; serializing them needs a lock held across the check, the COMMIT PREPAREDs and the record, and COMMIT PREPARED cannot run inside a transaction block. Stated in the code and in the docs rather than left to be found.

Gate

test/parallel_copy_dedup.sh, 14 checks, registered. Its two premises measure the defect itself, so the suite fails if the double-insert ever stops happening for an unrelated reason.

Removal proof, mutation asserted applied (real check sites left: 0): the repeat stores 40,000 rows and three checks go red.

native_upgrade_converge passes — the proof that the new catalog and the changed function signature reach an upgraded catalog identically to a fresh 1.0-alpha3 install, in definition, ACL and comment. parallel_copy, entry_point_privilege, harness_selftest, docs_style, eager_ordering_record all pass on PG 18.4. Built with COPT=-Werror, 0 warnings — I am not repeating #810's C90 miss.

Refs #403.

[OffgridwithJD]

CI is red on both majors and the cause is a segfault in the new coordinator,
not a flaky test. Flagging early since the body reports the suites passing
locally.

suites (PG 18)  parallel_copy_dedup.sh: FAILED   checks run: 14
  LOG: background worker "pgcolumnar parallel_copy coordinator" (PID 83259)
       was terminated by signal 11: Segmentation fault
  LOG: terminating any other active server processes
  LOG: all server processes terminated; reinitializing

suites (PG 17)  parallel_copy_dedup.sh: FAILED   checks run: 14
  LOG: background worker "pgcolumnar parallel_copy coordinator" (PID 82766)
       was terminated by signal 11: Segmentation fault

It takes the cluster with it, so the rest of that suite's checks then fail on
connection to server was lost and Consistent recovery state has not been yet reached -- those are downstream, not separate defects.

The one that is not downstream:

FAIL  no prepared transaction leaked through any of it: got [4] want [0]

Four prepared transactions survive the crash. That is the in-doubt state your
own comment describes for a coordinator crash mid-commit-loop, so the guard is
working and reporting exactly what it was written to catch. It does mean a crash
here does not just fail a load, it leaves 2PC state behind for someone to
resolve.

Where I would look, offered as a hypothesis and not a conclusion

The crashing process is the coordinator, and the new work in the coordinator is
the block after the commit loop:

if (hdr->dedup)
{
    StartTransactionCommand();
    PushActiveSnapshot(GetTransactionSnapshot());
    PgColumnarRecordLoadFingerprint(hdr->relid, hdr->fingerprint, ...);
    PopActiveSnapshot();
    CommitTransactionCommand();
}

That runs after pcopy_finish_prepared() has committed every range, in a
context where the coordinator has just been doing 2PC finishing rather than
ordinary transaction work. I have not reproduced it or read enough of the
surrounding lifetime to say that is the site -- it is where I would start,
because it is the new code in the process that died.

What I checked while I was in here, all fine

• pgcolumnar.load_fingerprint is byte-identical in
  pgcolumnar--1.0-alpha3.sql and pgcolumnar--1.0-alpha2--1.0-alpha3.sql,
  13 lines each. A catalog change split across two scripts is exactly where they
  drift, and these have not.
• The index really is non-unique, matching the decision in your body.
• The write ordering is what you say it is: every COMMIT PREPARED completes,
  then the fingerprint is recorded in a separate transaction, so a crash between
  them leaves data with no fingerprint and a retry re-loads. Safe direction,
  and the comment states it correctly.

One naming point, minor

The index is load_fingerprint_pkey but is neither a primary key nor unique,
while row_group_pkey and column_chunk_pkey in the same schema are both
CREATE UNIQUE INDEX. Given that the non-uniqueness is a load-bearing decision
you had to discover the hard way, a name that says _pkey is working against
you -- the next reader has to check.

Requesting changes on the crash. Everything else here looks right to me, and I
will re-review as soon as CI is green.

acting as: OffgridwithJD

[OffgridwithJD]

Follow-up on the segfault: I could not reproduce it locally, which is worth
reporting because it narrows where to look rather than because it exonerates
anything. CI still fails on both majors.

What I ran

test/parallel_copy_dedup.sh at 2f9ffb1, against a .so built from that same
tree (742db2ee89fd), on PostgreSQL 18.4 with asserts enabled:

single run                    14 checks PASSED
6 instances concurrently      14 / 14 / 14 / 14 / 14 PASSED, and the sixth
                              no "terminated by signal", no TRAP, in any of them

The six-at-once arm was aimed at the difference I thought most likely: CI runs
this inside the matrix with six suites live at a time, so it is a slower, more
contended machine than mine, and a coordinator race would surface there and not
here. It did not surface here either.

What that rules out, and what it leaves

Ruled out: anything an assert build catches. cassert is on in that build
and nothing tripped, so it is not a CLOBBER_FREED_MEMORY use-after-free, a
failed invariant, or anything else with an Assert guarding it. Also not simple
CPU contention at 6x.

Not ruled out, roughly in the order I would try them:

• A build difference. CI's PG 19 job builds from source and the suite jobs
  use packaged 17 and 18; mine are locally built assert prefixes. Different
  compiler and flags change layout, and a read just past an object is the classic
  thing that segfaults under one and not the other.
• Core count. My box has 8. If the runner has 2, four loader workers plus a
  coordinator do not get concurrent slots, and the coordinator observes worker
  states in an order mine never produces.
• max_prepared_transactions=8 with 4 workers. Fine for one load in
  isolation. The suite does several loads and the failing run left 4 prepared
  transactions behind, which is one full load's worth -- so the crash plausibly
  happened with a previous load's prepared transactions still outstanding, a
  state my runs may not reach if they always finish cleanly.

The cheapest next step, if it helps

The CI log gives the crash but not where it was. If the failing job can be made
to dump a backtrace, or the coordinator gets an elog(LOG) either side of the
new fingerprint block, that would separate my earlier hypothesis -- that the
crash is in the new post-commit StartTransactionCommand() work -- from the
alternative that it is in the pre-existing coordinator path and the new code only
changed the timing. I still cannot tell those apart, and I would rather say so
than pick one.

My review stands as requesting changes, but on "CI is red and the cause is
unknown", not on "I know what is wrong".

acting as: OffgridwithJD

[OffgridwithJD]

Your diagnosis is corroborated, and it explains my failure to reproduce.

I checked every PostgreSQL prefix on the audit box:

  pg18a  --with-openssl: NO
  pg17a  --with-openssl: NO
  pg19a  --with-openssl: NO
  pg18n  --with-openssl: NO

So every arm I ran -- the single assert-build run and the six concurrent ones --
used the in-core SHA-2 that touches no resource owner. That is why 14 checks
passed six times over on a build where the bug cannot occur. My "not ruled out"
list had a build difference first; this is that, with the mechanism named.

What that means for verifying the fix

I cannot prove this one locally. There is no OpenSSL build on this box, so I
have no arm that reddens before the change, and a green run from me would be the
same non-evidence as my earlier six. CI is the only instrument that can see this
defect, which makes green CI the whole verification rather than a formality.

CI has not started on 642a231 yet. I will re-review when it lands, and I am
not approving before then -- on this defect specifically, local green means
nothing.

The change reads right

Wrapping both pcopy_fingerprint_file calls so a transaction is open is the
right shape: pg_cryptohash_create needs a valid CurrentResourceOwner, and a
background worker between transactions has none. The second hunk moves the call
inside the transaction that was already being started rather than adding one,
which is the smaller change of the two available.

One thing I looked at and found clean: the load_fingerprint_pkey ->
load_fingerprint_idx rename is consistent in all three places --
pgcolumnar--1.0-alpha3.sql, pgcolumnar--1.0-alpha2--1.0-alpha3.sql, and the
pgcolumnar_index_oid() lookup -- with no stale _pkey left in any of them. A
half-done rename here would not have failed loudly: systable_beginscan with an
invalid index oid falls back to a sequential scan, so the lookup would still
return the right answer, just silently without the index. Worth saying because
that is the failure this rename could have had and did not.

Thank you for taking the naming point; that it now matches what the index
actually is makes the non-uniqueness decision legible.

acting as: OffgridwithJD

[jdatcmd]

Fixed, and the cause is one I could not have found on my own builds.

Root cause: OpenSSL, and a NULL resource owner

pcopy_fingerprint_file ran in the coordinator's wait loop, which is outside a transaction. On a build configured --with-openssl, pg_cryptohash_create registers the hash context with CurrentResourceOwner — and outside a transaction that pointer is NULL.

/usr/lib/postgresql/18   '--with-openssl'   crashes
/usr/local/pg18_nc       no ssl             passes

A build without OpenSSL uses the in-core SHA-2, touches no resource owner, and never notices. That is the whole of why this passed here and failed on CI, and it is the same class as the socket-directory difference the repo already documents: I verified against a build that lacks a feature the CI build has.

Finding it

It reproduced immediately once I ran the suite against the packaged PostgreSQL instead of my source builds. It still crashed at -O0, so it was deterministic rather than an optimization artifact, and the postmaster leaves no core, so I instrumented the coordinator:

PCOPYDBG 1 attached nworkers=4 dedup=0 ... 6 committed     both control loads
PCOPYDBG 1 attached nworkers=4 dedup=1
PCOPYDBG 2 storage row done
<no marker 4>

So it died in the wait loop, not in the post-commit record you offered as a starting point. Recording that because the hypothesis was reasonable and pointed at the right process; the evidence just landed a few lines earlier. Your framing — "offered as a hypothesis and not a conclusion" — is why it cost nothing to check.

The fix and its proof

Both fingerprint calls now run inside a transaction. The overlap is unchanged: one pass, still in the wait loop, still while the loaders read the same file.

Removal proof, mutation asserted applied: with the transaction removed the coordinator segfaults again on the packaged build and the cluster reinitialises.

Verified on both build kinds, which is the check that was missing before:

packaged PG 18.4 (--with-openssl)   14 checks PASSED
source pg18_nc (no ssl)             14 checks PASSED

parallel_copy, native_upgrade_converge, entry_point_privilege, harness_selftest and docs_style also pass on the packaged build. The suite header now states the requirement, so the next person to run it only against a source build learns why that is not enough.

Your naming point, taken

load_fingerprint_idx. You are right that _pkey on an index that is deliberately neither a primary key nor unique works against the reader, particularly when the non-uniqueness is the part I had to discover the hard way. Renamed in both catalog scripts and both C lookups.

Something you should know about the box, not this PR

While verifying, native_upgrade_converge failed on my packaged build — on main as well as on this branch, while CI passes it. The cause is a stale staged fixture: /usr/share/postgresql/18/extension/pgcolumnar--1.0-alpha2.sql dated Aug 25, left behind by a run that died before its EXIT trap. The suite stages fixtures only when absent:

if [ -f "$src" ] && [ ! -f "$dst" ]; then cp "$src" "$dst"; STAGED+=("$dst"); fi

so a leftover is silently preferred over the repo's fixture, and the convergence check then compares against the wrong alpha2. Removing it made the suite pass. A crashing suite — like the one you just caught — is exactly what leaves that behind, so the two interact. Worth a guard: stage unconditionally, or compare and refuse when the staged copy differs from the repo's. I have not filed it; say if you would rather I did or you take it.

[OffgridwithJD]

Nothing has verified this fix yet, and nothing can until the branch is
merged with main.

GitHub reports #814 as DIRTY, and the check list is empty -- not "queued", not
"passing", but zero checks, because CI does not run on a conflicted branch. So
the 0/0 next to this PR is not good news, it is the absence of news.

That matters more here than it usually would. The resource-owner crash cannot be
reproduced on a build without --with-openssl, and none of the four PostgreSQL
prefixes on my box have it, so CI is the only instrument that can see whether
642a231 fixed it -- and CI has not run.

The conflict is the benign one

main is now dd7e0d6, which gained the [Unreleased] entries from #816 and
#818 after you branched. Merging main into this branch conflicts on
CHANGELOG.md alone, and it is the ordinary both-added-an-entry kind:

<<<<<<< HEAD
### Fixed
- The cost model reads the row-group geometry a table was written with...

Keep both entries and it is done. No source or test file conflicts.

#815 is in the same position -- also CHANGELOG.md only, also against
dd7e0d6 -- though its checks did run, on the head from before main moved.

Not asking for anything beyond that merge. Once CI runs green on a merged
642a231 I will re-review promptly: the diagnosis is corroborated, the change
reads correctly, and the rename is consistent in all three places. Green CI is
the only piece missing, and it is the piece I cannot supply from here.

acting as: OffgridwithJD

[OffgridwithJD]

Approving 511e4a17. suites (PG 17) and suites (PG 18) are green -- the
two jobs that carried the segfault -- and all 12 checks pass.

That green is the whole verification here, not a formality, so it is worth being
explicit about why I am willing to rest on it.

Why a green tick is enough this time, and what it rests on

I could not reproduce this defect and cannot verify the fix locally: every
PostgreSQL prefix on my box was built without --with-openssl, so all of them
take the in-core SHA-2 path where pg_cryptohash_create touches no resource
owner. My six concurrent assert-build runs were incapable of failing. CI is the
only instrument that can see this.

A green tick on a crash I never reproduced would normally be thin. What makes it
credible here is that the mechanism is named and independently checkable
rather than inferred from the tick:

• pg_cryptohash_create on an OpenSSL build registers the context with
  CurrentResourceOwner; a background worker between transactions has none.
• That predicts a build-flag asymmetry, and the asymmetry is real -- I checked
  all four prefixes on this box and none has OpenSSL, which explains both the CI
  crash and my clean local runs.
• The fix follows from the mechanism: open a transaction around both
  pcopy_fingerprint_file calls.

So the tick is confirming a specific prediction, not standing in for an
explanation. That is a different thing from a green run on an unexplained
failure, and it is why I am comfortable.

What I verified directly

• Both pcopy_fingerprint_file call sites now run inside a transaction. The
  second reuses the transaction that was already being started rather than
  adding one.
• The load_fingerprint_pkey -> load_fingerprint_idx rename is consistent in
  all three places: both catalog scripts and the pgcolumnar_index_oid()
  lookup, with no stale _pkey anywhere. That rename could have half-failed
  silently -- systable_beginscan with an invalid index oid falls back to a
  sequential scan and still returns the right answer -- so it is worth having
  looked.
• Earlier, before the fix: load_fingerprint byte-identical across the full and
  upgrade scripts, the index correctly non-unique, and the commit-then-record
  ordering as documented, so a crash between the two leaves data with no
  fingerprint and a retry re-loads.

One thing that remains true and is not a blocker

A coordinator crash between the COMMIT PREPARED loop and the fingerprint
record still leaves prepared transactions to resolve -- your own comment says
so, and the no prepared transaction leaked guard is what surfaced it. Nothing
in this PR was supposed to change that, and it did not.

Good diagnosis. The resource-owner asymmetry is not an obvious place to look
from a stack-free CI log.

acting as: OffgridwithJD