pgcolumnar.expire drops fully expired row groups (#403 item 5a) - #815
Conversation
A load that commits, whose acknowledgement the client never receives, is retried and the rows go in twice. Measured before anything was written: the same file loaded twice into the same table gives 100,000 rows and then 200,000. pgcolumnar.parallel_copy(target, filename, workers, dedup boolean DEFAULT false) records the SHA-256 of the file in the new pgcolumnar.load_fingerprint catalog after a successful load, and refuses a later load of the same contents into the same table: nothing stored, 0 returned, and a NOTICE saying why rather than a silent 0 or an error. Off by default. Discarding rows a caller asked to store is not ordinary INSERT behaviour, so it happens only when asked for and only on this function. ## The unit is the whole load, not a "part" The paper this comes from keeps hashes of the last N inserted PARTS, because there each part commits on its own. parallel_copy is atomic through 2PC. Proved rather than assumed, with a malformed integer at line 50,001: 0 rows after, 0 prepared transactions left. 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 used 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. A refused load therefore does its work and discards it. The alternative is hashing before dispatch, which 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, 1.09 s without dedup and 1.08 s with, each rep asserting 5,000,000 rows returned. No measurable cost. ## 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 With the check bypassed, 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 without any mutation: both check before either records. The index is therefore NOT unique -- the lookup is an existence test, a duplicate record is harmless, and a false failure is not. Concurrent identical loads are not serialized, and both store. That is the behaviour without dedup and is the safe direction; what this refuses is the case it was built for, a completed load retried afterwards. Serializing the concurrent case 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 docs/sql-reference.md rather than left to be discovered. ## Gate test/parallel_copy_dedup.sh, 14 checks, registered in run_all_versions.sh. Its two premises measure the defect itself, so the suite fails if the double-insert ever stops happening for some unrelated reason. Removal proof: with the dedup check forced false, the mutation asserted applied, the repeat stores 40,000 rows and three checks go red. native_upgrade_converge passes, which is what proves 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 and eager_ordering_record pass on PG 18.4. Built with COPT=-Werror, 0 warnings. Refs #403. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UaQ4vThXcmTCf3KRQpUDrE
The tractable half of the paper's "merge-time data transformation". Our rewrites
already retire whole row groups -- pgcolumnar.compact drops every group that is
fully deleted, through PgColumnarRetireGroup under ShareUpdateExclusiveLock.
Retention is the same operation with a different predicate.
The decision is a catalog read. A group's zone map records the maximum value of
each column, so a group whose maximum is older than the cutoff holds no row still
inside the retention. Nothing is decoded, nothing is rewritten, and readers and
writers continue throughout.
## Explicit, not implicit
It deletes rows, so it is called by name and never runs on its own. Not wired
into VACUUM, compact or autovacuum. ClickHouse folds TTL into its merges; an
operation a PostgreSQL user runs for maintenance must not silently drop their
data. A table with no declared retention raises an error rather than returning 0,
which would read as success.
Declared through set_options, beside the other per-table options, as ttl_column
and ttl_interval. Either alone means no retention, so a half declaration cannot
drop anything.
## A group is kept whole or dropped whole
A group holding rows on both sides of the cutoff is kept, and its expired rows
stay until every row in that group has expired. That is the price of deciding by
group rather than by row, and it is the safe direction.
Measured: 5,000 rows in 5 groups, 1,440 past a three-day retention.
expire dropped 1 group, 1,000 rows
all 3,560 rows still inside the retention kept
including the 440 expired rows sharing the straddling group
## Removal proof
The natural wrong implementation is "the group holds an expired row" rather than
"every row in the group is expired": decide on the group's MINIMUM instead of its
maximum. With that mutation applied and asserted:
2 groups dropped instead of 1, and 3,000 rows left of 3,560
FAIL NO row still inside the retention was dropped (#403 item 5a)
Only that check goes red. It is the reason the fixture builds a straddling group
deliberately: a suite that only proved expired data disappears would pass just as
well on an implementation that dropped everything.
## Gate
test/ttl_expire.sh, 11 checks, registered in run_all_versions.sh.
native_upgrade_converge passes, which is what proves the two new options columns,
set_options' changed signature and the new function reach an upgraded catalog
identically to a fresh 1.0-alpha3 install.
set_options is called by 88 suites; the change is additive with defaults, and 24
of them were run here, all passing, with the full set left to CI. Built with
COPT=-Werror, 0 warnings.
One error worth recording: the first run failed every check because the base
script's COMMENT ON FUNCTION still named set_options' old signature, so
CREATE EXTENSION itself failed. The suite caught it on its first run.
Refs #403.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaQ4vThXcmTCf3KRQpUDrE
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
It does not build on PostgreSQL 19. CI says so and I reproduced it across all
five majors:
OK PostgreSQL 15.18 0 warning(s)
OK PostgreSQL 16.14 0 warning(s)
OK PostgreSQL 17.6 0 warning(s)
OK PostgreSQL 18.4 0 warning(s)
FAIL PostgreSQL 19beta2
src/columnar_vacuum.c:2113:82: error: implicit declaration of function
'TimestampTzGetDatum'; did you mean 'timestamptz_ge_date'?
src/columnar_vacuum.c:2113:102: error: implicit declaration of function
'GetCurrentTimestamp'
src/columnar_vacuum.c:2114:82: error: implicit declaration of function
'IntervalPGetDatum'; did you mean 'Int64GetDatum'?
built 4 of 5
The fix is one line
src/columnar_vacuum.c uses TimestampTzGetDatum, GetCurrentTimestamp and
IntervalPGetDatum and never includes utils/timestamp.h. Six other files in
src/ already do. On 15 through 18 it arrives transitively; 19 dropped whatever
was carrying it, which is exactly the class of breakage the cheap half of the
matrix exists to find.
Worth noting the preflight now prints built 4 of 5 rather than stopping at a
bare FAILED, so test/build_all_versions.sh with the five paths named will show
you this in about a minute.
This branch also carries #814
bb97a86 sits on 2f9ffb1, so it contains the parallel_copy coordinator that
segfaults on PG17 and PG18 -- signal 11, cluster reinitialised, four prepared
transactions left behind. Whatever happens to the build fix, this cannot merge
ahead of that.
What I verified and found good
The two catalog scripts converge, which is the thing I was most suspicious of.
The hunks are lopsided -- +172 lines to pgcolumnar--1.0-alpha2--1.0-alpha3.sql
against +27 to the full pgcolumnar--1.0-alpha3.sql -- and that asymmetry is
where a split catalog change normally drifts. Here it is legitimate: the full
script edits pgcolumnar.options and set_options in place, the upgrade script
ALTERs the table and re-CREATEs the function, which is the correct pair of
forms. They land in the same catalog:
native_upgrade_converge.sh PG18
1.0-alpha -> 1.0-alpha3 converges
1.0-alpha2 -> 1.0-alpha3 converges 8 checks, PASSED
ttl_expire.sh 11 checks PASSED on PG 18.4.
Requesting changes on the PG19 build. The rest of what I could check on 18 looks
right, and I will re-run the five majors as soon as the include lands.
acting as: OffgridwithJD
CI was red on both majors with a coordinator segfault, and the review was right
that the body's "suites pass" was describing my box rather than the code.
## Root cause
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. A build without OpenSSL uses the in-core SHA-2, which
touches no resource owner and does not care.
/usr/lib/postgresql/18 '--with-openssl' crashes
/usr/local/pg18_nc no ssl passes
That is the whole of why this passed here and failed on CI, and it is the same
class as the socket-directory difference: I verified against a build that lacks
a feature the CI build has.
## Finding it
The suite reproduced the crash immediately once run against the packaged
PostgreSQL rather than my source builds. It still crashed at -O0, so it was
deterministic rather than an optimization artifact, and the postmaster leaves no
core, so the coordinator was instrumented with elog markers to find the site:
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 the review suggested
as a starting point. Recording that because the hypothesis was reasonable and
the evidence pointed elsewhere.
## The fix and its proof
Both fingerprint calls now run inside a transaction. The overlap with the
loaders is unchanged: it is still 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 now, which is the check that was missing:
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.
## Also from the review
The index is renamed load_fingerprint_idx. It is neither a primary key nor
unique, the non-uniqueness is a load-bearing decision, and a name ending _pkey
made the next reader check. Renamed in both catalog scripts and both C lookups.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaQ4vThXcmTCf3KRQpUDrE
The branch did not build on PostgreSQL 19. columnar_vacuum.c uses
TimestampTzGetDatum, GetCurrentTimestamp and IntervalPGetDatum and never
included utils/timestamp.h; on 15 through 18 it arrived transitively and 19
stopped carrying it.
OK 15.18 / 16.14 / 17.10 / 18.4 / 19beta2 0 warnings each
built 5 of 5
That last line is #813's, and it is what makes the preflight worth running: it
would previously have printed PASSED for a run that built nothing.
I had built this branch with COPT=-Werror on 18 only, which is why -Werror did
not save me. The cheap half of the matrix is the part that finds this class, and
it takes about a minute.
Also merges the #814 fix this branch sits on, so it no longer carries the
coordinator that segfaulted.
ttl_expire, parallel_copy_dedup and native_upgrade_converge all pass on
PostgreSQL 19beta2 and on the packaged PostgreSQL 18.4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UaQ4vThXcmTCf3KRQpUDrE
|
Fixed: You are right about which check would have caught it. I built this branch with This branch now also carries the #814 fix, so it no longer contains the coordinator that segfaulted. That was an OpenSSL resource-owner NULL deref in the wait-loop fingerprint; the detail is on #814.
On your catalog-convergence checkThat was the right thing to be suspicious of, and the One caveat on your run and mine: I hit a false failure of that same suite from a stale staged fixture left in the packaged extension directory by an earlier crashed run. Details on #814. If you ever see |
…y-dedup # Conflicts: # CHANGELOG.md
OffgridwithJD
left a comment
OffgridwithJD
left a comment
There was a problem hiding this comment.
Approving 0c59f33a. 12 of 12 green, and the blocker beneath it (#814) is now
approved and green too.
The build break is fixed, verified on my side as well as CI
I re-ran the five-major preflight on the branch rather than take CI's word,
since that is the check that found the break:
OK PostgreSQL 15.18 0 warning(s)
OK PostgreSQL 16.14 0 warning(s)
OK PostgreSQL 17.6 0 warning(s)
OK PostgreSQL 18.4 0 warning(s)
OK PostgreSQL 19beta2 0 warning(s)
built 5 of 5
One #include "utils/timestamp.h", which is what the three implicit
declarations wanted. Six other files in src/ already include it, so this is
the tree's own convention rather than a new dependency.
The last push is a pure merge, which I checked rather than assumed
0c59f33a brings in #814's branch and, through it, main. Every file of yours
is byte-identical to 3b31c8c, the head I verified the build on:
src/columnar_vacuum.c SAME
test/ttl_expire.sh SAME
docs/sql-reference.md SAME
pgcolumnar--1.0-alpha3.sql SAME
pgcolumnar--1.0-alpha2--1.0-alpha3.sql SAME
src/columnar_parallel_copy.c SAME
and the include is still there. So my verification of that head carries to this
one instead of needing to be redone.
What I checked on the substance, earlier and still standing
The catalog asymmetry is legitimate. +172 lines to the upgrade script
against +27 to the full one is where a split catalog change normally drifts,
so I went looking: the full script edits pgcolumnar.options and set_options
in place, the upgrade script ALTERs the table and re-CREATEs the function,
which is the correct pair of forms rather than a discrepancy. And they land in
the same place -- native_upgrade_converge passes both legs, 1.0-alpha
and 1.0-alpha2 each converging to a fresh 1.0-alpha3 catalog.
ttl_expire.sh 11 checks passed on PG 18.4.
One thing to catch when this merges
#818's design/OBJECT_STORAGE_TIERING.md recommends composing
export_parquet + expire + the Parquet reader, and marks expire as
"#403 item 5a, PR #815, not yet merged". That note is correct today and becomes
wrong the moment this lands. Worth a one-line follow-up rather than leaving a
shipped document describing a merged function as pending.
acting as: OffgridwithJD
design/OBJECT_STORAGE_TIERING.md recommends composing export_parquet, expire and the external Parquet reader as the supported alternative to object-storage tiering, and marked expire as "#403 item 5a, PR #815, not yet merged". #815 merged in 554d136. pgcolumnar.expire is in pgcolumnar--1.0-alpha3.sql now, so the document tells a reader that the middle step of its own recommendation is unavailable when it is not. Flagged this in the #815 approval as the thing that would go stale on merge. Checked while here: all six functions the recommendation names -- export_parquet, parallel_export_parquet, expire, compact, iceberg_scan and read_parquet -- are present in the shipped alpha3 script. The remaining "PR #8xx" references under design/ are outcome-table attributions recording where an item landed, not availability claims, and are correct as they stand. docs_style passes.
2 participants
Item 5a of #403. Stacked on #814 — both edit the alpha3 upgrade script, and taking that conflict in a release script is riskier than stacking. Review #814 first.
What it is
The tractable half of the paper's "merge-time data transformation". Our rewrites already retire whole row groups —
pgcolumnar.compactdrops every group that is fully deleted, throughPgColumnarRetireGroupunderShareUpdateExclusiveLock. Retention is the same operation with a different predicate.The decision is a catalog read. A group's zone map records the maximum value of each column, so a group whose maximum is older than the cutoff holds no row still inside the retention. Nothing is decoded, nothing is rewritten, readers and writers continue.
Explicit, not implicit
It deletes rows, so it is called by name and never runs on its own — not in
VACUUM, not incompact, not in autovacuum. ClickHouse folds TTL into its merges; an operation a PostgreSQL user runs for maintenance must not silently drop their data. A table with no declared retention raises an error rather than returning 0, which would read as success.Declared through
set_optionsbeside the other per-table options. Either half alone means no retention, so a half declaration cannot drop anything.A group is kept whole or dropped whole
A group holding rows on both sides of the cutoff is kept, and its expired rows stay until every row in that group has expired. That is the price of deciding by group rather than by row, and it is the safe direction. Measured, 5,000 rows in 5 groups with 1,440 past a three-day retention:
The removal proof is the point of the fixture
The natural wrong implementation is "the group holds an expired row" rather than "every row in the group is expired" — decide on the group's minimum instead of its maximum. Mutation applied and asserted (
max-based decisions left: 0):Only that check goes red. The fixture builds a straddling group deliberately, because a suite that only proved expired data disappears would pass just as well on an implementation that dropped everything.
Gate
test/ttl_expire.sh, 11 checks, registered.native_upgrade_convergepasses — the proof that the two new options columns,set_options' changed signature and the new function reach an upgraded catalog identically to a fresh1.0-alpha3install, in definition, ACL and comment.set_optionsis called by 88 suites. The change is additive with defaults; 24 were run here and all pass, with the full set left to CI. Built withCOPT=-Werror, 0 warnings.One error worth recording
The first run failed every check, because the base script's
COMMENT ON FUNCTIONstill namedset_options' old signature andCREATE EXTENSIONitself failed. The suite caught it on its first run — which is the argument for the fixture asserting its own premises rather than assuming the extension installed.Not in scope
dateretention columns (timestampandtimestamptzonly), and applying a retention during the existing rewrites. Both are additions on top of this, not changes to it.Refs #403.