| From: | Greg Burd <greg(at)burd(dot)me> |
|---|---|
| To: | Manu <manuelreyesbravo(at)gmail(dot)com> |
| Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org, Nikhil Kumar Veldanda <veldanda(dot)nikhilkumar17(at)gmail(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Japin Li <japinli(at)hotmail(dot)com>, Dharin Shah <dharinshah95(at)gmail(dot)com> |
| Subject: | Re: ZSTD TOAST compression, and an extensible compression method encoding |
| Date: | 2026-10-27 11:47:13 |
| Message-ID: | ormEJV9zReviaUUzJw6tZ2RSvu5Up688drMbhZWYk4jn4VshCUL9L1UNnZVadUPZqkX7FS7mtv3wzoJ50Frnd1DGTyDmTPe1JMjYpasdum0=@burd.me |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Sunday, October 25th, 2026 at 9:21 PM, Manu <manuelreyesbravo(at)gmail(dot)com> wrote:
> Hi Greg,
Hi Manu,
Thanks for the depth here. All four diffs are in v12, which I'll post as
its own message; replies inline.
> 1. lz4 without zstd does not compile. lz4_dict_get() is only under
> #ifdef USE_LZ4 and calls dict_fetch(), which is static and inside
> #ifdef USE_ZSTD; the call is at toast_dictionary.c:537.
Fixed as you had it: dict_fetch() now sits under USE_ZSTD || USE_LZ4 and
only the zstd invalidation-callback registration stays under USE_ZSTD. I
build all three configurations now (zstd+lz4, lz4-only, zstd-only), and
each passes make check (243). cfbot never caught this because it builds
with both; I should have built the single-codec configs myself.
> 2. With zstd but without lz4, compression_zstd_dict fails: its SET
> COMPRESSION lz4 gets "compression method lz4 not supported". The same
> diff uses pglz there.
Taken; the test uses pglz for the switch-away-from-zstd_dict case, which
does not depend on lz4 being built.
> 3. 0005 swallows errors, a cancel included. [...] A real SIGINT on entry
> to it: ANALYZE completed and only logged "... was skipped: canceling
> statement due to user request".
A swallowed cancel and the half-done dict_install() is fixed. v12 runs the
evaluation in an internal subtransaction and re-throws, exactly as your
diff does: BeginInternalSubTransaction, the work, then either
ReleaseCurrentSubTransaction on success or RollbackAndReleaseCurrent on
error; and it re-throws when edata->sqlerrcode is ERRCODE_QUERY_CANCELED
or an interrupt is still pending, since ProcessInterrupts() clears
QueryCancelPending before raising. A failed step now rolls back its TOAST
scan and buffer pins instead of leaking, and a mid-dict_install() error no
longer leaves the old dictionary non-current with no replacement. Under
cassert+ASan a SIGINT gives the normal ERROR, a damaged chunk gives one
WARNING and no leak, and adoption still works in plain ANALYZE, in
BEGIN/COMMIT, BEGIN/ROLLBACK, a rolled-back savepoint, and VACUUM
(ANALYZE).
> 4. The dictionary caches hold one entry. Every switch between
> dictionaries fetches the dictionary again, and zstd digests it again.
> [...] one column retrained without a rewrite, which evolution now does
> on its own [...] 258,163 (1,946 fetches for 2,000 values).
Right, and evolution makes this the common case, not a corner one. v12
takes your 4-entry LRU keyed by OID for both directions, picking the
victim after dict_fetch() since the fetch can run the invalidation
callback and empty the cache. The instruction counts match what you saw:
the two-dictionary scan drops from ~265k to ~28k per value, and the
retrain-without-rewrite case from ~258k to ~28k. I kept your 4; it is
arbitrary but it covers a scan that alternates between an old and a new
dictionary with headroom, and the per-backend memory is bounded.
> 5. Evolution without drift: [...] At 6,000 one evaluation calls
> ZSTD_createCDict() 12,005 times [...] The fourth diff replaces
> compressed_size() with compressed_total(), one call per dictionary.
Taken. compressed_total() digests each dictionary once and loops the
held-out values inside, so an evaluation now builds two CDicts instead of
one per value. Same totals, same decision, and the held-out test stops
dominating the ANALYZE it rides on.
> 6. On the ~1kB numbers: a 1 kB value alone in its row stays below
> TOAST_TUPLE_THRESHOLD (heapam.c:2266), so it is stored uncompressed with
> every method [...]. At 3 kB the dictionary takes lz4 from 1.94 to 2.50
> on docs, 3.58 to 4.76 on json.
You're right, a ~1 kB value never reaches the compressor, so the "+80%
at 1 kB" number was measuring nothing on the TOAST path; it came from a
standalone CLI run, not from values actually stored. The dictionary
helps once a value crosses TOAST_TUPLE_THRESHOLD.
> Corrupt dictionary values error cleanly under ASan, and substr() matches
> the raw text at 56 offset/length pairs on UTF8 and SQL_ASCII.
Matches my runs too, corrupt values raise the data-corrupted error with no
sanitizer report, and substr() over the offset/length grid matches the raw
text on both encodings.
Thanks again; the subtransaction and the cache fixes in particular are
ones the series needed. v12 follows.
best.
-greg
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Etsuro Fujita | 2026-10-27 11:50:16 | Re: Fix rescan costing of single-batch hash joins |
| Previous Message | Hayato Kuroda (Fujitsu) | 2026-10-27 11:40:25 | RE: Use WALReadFromBuffers in more places |