Re: ZSTD TOAST compression, and an extensible compression method encoding

Hannu, Manu, thank you both; this was a thorough pass.

v13 is attached.

It is v12 plus a sixth patch that collects the fixes, kept separate so
the review is easy to follow; I will fold it into 0001-0005 before a
commit. It passes check-world in all three configurations (zstd+lz4,
lz4-only, zstd-only), and the containment, memory and cancel cases below
behave as you'd want under cassert+ASan.

Replies inline; Manu's confirmations folded in where they touch the same
point.

Hannu wrote:
> 1. 1-byte out-of-bounds read on corrupt 8-byte compressed varlenas

Fixed. toast_decompress_datum() and its slice form now check the datum
is at least VARHDRSZ_COMPRESSED_LONG before the long-form method byte is
read (a new VARDATA_COMPRESSED_GET_RAW_METHOD() reads only the two
bits), and reject a long header whose method is pglz, lz4, or the
invalid id, so a corrupt value no longer reads past the allocation or
decodes from the wrong offset. I did not, in this pass, extend amcheck
to validate inline compressed headers; that is a worthwhile separate
addition and I have it noted, but the out-of-bounds read itself is
closed at the decode path.

> 2. One-shot ZSTD_compress() allocates/frees a ~1.3 MB CCtx per datum

Fixed. plain zstd_compress_datum() reuses a backend-static ZSTD_CCtx
via ZSTD_compressCCtx(), like the decompression context.

> 3. Minor items in 0001-0003

CHECK_FOR_INTERRUPTS() is in the slice loop. toast_external_info_get()
now reports an uncompressed external datum as
TOAST_INVALID_COMPRESSION_ID rather than leaving the pglz-looking raw
bits. On the toast_fetch_datum_slice() 5-vs-9 byte point you are right
that it is latent (slice pushdown is disabled for the dictionary
methods); I left the code as is for now with that understanding, and it
should be tightened when prefix pushdown is enabled for them.

> 4. Three containment escapes

(b) and (c) are fixed: the indirect-pointer branch of
detoast_external_attr() decompresses an inline dictionary datum, and
datumTransfer() (so SPI_datumTransfer()) flattens one instead of copying
it verbatim. For (a), the R/W ExpandedRecord fast path, v13 already
marks inline dictionary datums with ER_FLAG_HAVE_EXTERNAL, so Muzzammil
Sarwar's fast-path fix [1] closes it without a change here, as Manu
confirmed; I'd rather rely on that core fix than special-case pl_exec.c,
and I'll add a regression test once it lands. Thanks to Manu for
reproducing all three and for pinning (a) to that patch.

> 5. Lock-before-ACL, cross-session temp tables, per-call leak

All three fixed in pg_train_toast_dictionary(): it checks ownership
before taking ShareUpdateExclusiveLock, rejects RELATION_IS_OTHER_TEMP,
and does its sampling and training in a short-lived memory context.

> 6. TRUNCATE does not retire non-current dictionaries

Fixed; ExecuteTruncateGuts() now retires the relation's non-current
dictionaries, as a rewrite does.

> 7. Inline HEAP_HASEXTERNAL overhead & unrelated-column UPDATE behavior

This one I have not changed, and I want to flag it rather than paper
over it. The extra per-UPDATE work on a row that holds an inline
dictionary datum is real, and it is the cost of making those datums
self-contained the same way a TOAST pointer is; I don't see a way to
keep containment without HEAP_HASEXTERNAL set. I did fix the misleading
comment in the test. If you see a way to narrow the flag to rows that
actually carry a dictionary datum without losing containment, I'm
interested.

> 8. Cache sizing & invalidation

Fixed. The caches now evict only the entry whose dictionary changed,
comparing the hashvalue computed when the entry was cached (the callback
must not touch the catalog, since it can run outside a transaction,
which is what an early version of this got wrong). The 4-entry size is
still arbitrary; a partitioned scan over more than four dictionaries
will still turn entries over, but no longer wipes the whole cache on an
unrelated change.

> 9. compression_dict_evolve.sql compresses ZERO rows

Fixed. The test now uses rows wide enough to be TOAST-compressed on
disk (it asserts pg_column_size < octet_length), keeps distribution-A
rows present while adding distribution-B rows so two dictionary
generations coexist, and checks each generation still reads back. You
are also right about the two policy gaps: v13 does not yet refuse to
train on a table whose rows never approach the threshold, and still
adopts the first dictionary without a held-out check against plain
compression. Those are policy refinements I'd like to do, but they felt
larger than this correctness pass, so I left them as follow-ups rather
than rush them.

> 10. Permanent C malloc() leaks across PG_CATCH() OOM rollback

Fixed. The held-out evaluation allocates its output buffer before
creating the CDict, and the lz4 paths use a backend-static stream reset
per value instead of LZ4_createStream()/free, so an OOM longjmp cannot
jump over the free.

> 11. tdtrainrows = -1 breaks the growth gate & pg_upgrade loses metadata

Both fixed. pg_train_toast_dictionary() clamps a reltuples estimate of
-1 to 0, and binary_upgrade_add_toast_dictionary() now carries
tdtrainrows and tdtraintime, so the gates are unchanged across an
upgrade.

> 12. Single-slot rd_toastdictattnum relcache cache thrashes on 2+ columns

Fixed; it is a per-column array in CacheMemoryContext now, so a write
into a table with several dictionary columns no longer misses on every
row.

> 13. PGC_USERSET evolution GUCs vs. MAINTAIN privilege

Fixed; the three evolution GUCs are PGC_SUSET. The pg_stats-like view
over pg_toast_dictionary metadata is a good idea and I've noted it as a
follow-up.

Manu wrote:
> Two leftovers: the 0005 message still says compressed_size() ...

The header comment is corrected to compressed_total(); the function
comment already described the subtransaction after your earlier diff.
And thank you for the v12 confirmation run and the containment matrix;
it is what gave me confidence these were the right fixes.

On Hannu's framing that 0001-0003 are close to committable on their own:
I agree, and splitting the series that way for PG20 makes sense to me if
a committer prefers it. 0001-0003 stand without the dictionary work.

best.

-greg

[1] https://www.postgresql.org/message-id/CABUcsRRuMHXWqgiZ5wy=4OPbET0niFQEVY_d8VofeumTxjBJjw@mail.gmail.com