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

On Sat, 31 Oct 2026 01:40:59 +0100, Hannu Krosing <hannuk(at)google(dot)com> wrote:
> Hi Greg,

Hello Hannu,

Thanks for the second pass. v14 is attached, rebased on master
(fa5ecff759b). The fixes are folded into 0001-0005 this time, so there
is no sixth patch.

> 1. [Blocker] pg_upgrade fails whenever pg_toast_dictionary is non-empty

Yes, I reproduced it: "date/time field value out of range:
"846872593030845"". The sixth argument of
binary_upgrade_add_toast_dictionary() is int8 now and pg_dump emits
::pg_catalog.int8, as you suggested. dict_install() no longer treats
traintime 0 as "now"; every caller passes the time explicitly.

compression_zstd_dict now leaves public.zstd_dict_upgrade behind, with a
current and a retired dictionary, so 002_pg_upgrade covers this.

> 2. CacheMemoryContext leak and unreset pg_internal.init pointers on
> rd_toastdictoids / rd_toastdictvalid

Fixed. RelationDestroyRelation() frees both, and
load_relcache_init_file() resets them to NULL with the other transient
fields. I did not bump RELCACHE_INIT_FILEMAGIC. The init files are
removed at every startup, so a file written by another build is never
read, and they only hold system catalogs, which cannot have
dictionaries. The comment above toast_dictionary_current() is fixed
too.

> 3. Remaining OOB reads & non-canonical header bypass on corrupt 4B_C
> datums (0002 / 0004 / 0006)

Done as you suggested, in one place. 0002 adds
toast_inline_compression_id() next to
toast_compression_id_needs_cmid_byte(). It returns
TOAST_INVALID_COMPRESSION_ID unless the datum has at least
VARHDRSZ_COMPRESSED bytes, has at least VARHDRSZ_COMPRESSED_LONG if the
bits say long form, and the form matches the method. Both decompression
entry points and toast_get_compression_id() use it. The slice path now
calls it before it reads the size for its whole-value shortcut, which
covers (c). toast_raw_datum_size() checks for a complete header. In
0004, VARATT_IS_COMPRESSED_DICT() and toast_dict_decompress_datum() use
the same helper, and dict_datum_get_id() still checks for
VARHDRSZ_COMPRESSED_DICT.

To test it I patched heap pages with five bad headers: 8 bytes with the
long bits, 6 bytes, long form with va_cmid 0 and 1, and the plain form
claiming zstd. On v13, pg_column_compression() returned a method for all
five, octet_length() of the 6-byte one returned 3200 (read from the
bytes after the datum), and substr() of the plain-form zstd one returned
"abcde". On v14, pg_column_compression(), length() and substr() report
"compressed data is corrupt" for all five. octet_length() reports it for
the 6-byte one; for the others it returns the size recorded in their
complete 8-byte header, as master does.

amcheck still does not look at inline compressed headers; I'd still
like to do that separately.

> 4. Same-subtransaction TRUNCATE misses toast_dictionary_retire()

Fixed: the call now follows the if/else. compression_zstd_dict has a
case for it: train twice and TRUNCATE inside the transaction that
created the table, leaving one dictionary.

> 5. tdtrainrows = 0 clamping and un-updated cooldown when adopt == false

Both fixed.

For the first: pg_train_toast_dictionary() still records 0 when
reltuples is -1, but evolution now reads 0 as "row count unknown"
instead of a baseline. The first ANALYZE that finds rows records its
own count and does not retrain, so a 4 kB dictionary trained by hand is
not replaced on the next ANALYZE. I went this way instead of passing the
scanned count because the trainer stops after DICT_MAX_SAMPLES values,
so the scanned count is not the row count. compression_dict_evolve
tests the exact sequence you gave.

For the second: when the gates pass but the candidate loses, or no
candidate can be trained, the current dictionary's tdtraintime and
tdtrainrows are reset, so the following ANALYZEs skip it until the
cooldown and growth gates pass again. The test checks that tdtraintime
moves on the rejected ANALYZE. The docs for
dictionary_evolution_min_interval and dictionary_evolution_growth say
so.

> === On #7: Narrowing inline HEAP_HASEXTERNAL UPDATE / DELETE overhead ===

(1) is in 0004, close to the form you described. toast_tuple_init()
skips toast_dictionary_keep() when there is an old tuple, the relation
is not a rewrite target (relrewrite is invalid), and the old and new
values are byte-identical dictionary datums. I checked it with
pageinspect: train, recompress every row, train again, then UPDATE an
unrelated column of one row. On v13 that row's datum is recompressed
with the new dictionary; on v14 it keeps the old one. VACUUM FULL
still recompresses it and retires the old dictionary, and every value
reads back. So the comment in compression_zstd_dict.sql is now true
for inline values as well.

I left (2) out. It is an existing heapam path, and the cost is a deform
on DELETE for a table with no TOAST table that holds inline dictionary
datums. That seems rare enough to wait until it shows up in a profile.

> - toast_dictionary.c:1158-1166: when !object_ownercheck() fails in
> pg_train_toast_dictionary(), get_rel_relkind(relid) and
> get_rel_name(relid) are called unlocked

Fixed: one RELOID syscache lookup, then has_privs_of_role() on relowner,
which is the check object_ownercheck() does.

> - toast_dictionary.c:268: "compressed zstd data is corrupt" in
> dict_datum_get_id() is also reached for lz4_dict

It says "compressed dictionary data is corrupt" now. Without zstd,
pg_train_toast_dictionary() says "training compression dictionaries is
not supported by this build", with a detail asking for zstd.

> - compression_dict_evolve.sql:18-19: stale comment

Fixed.

> - SGML docs:

catalogs.sgml documents tdtrainrows and tdtraintime, lists 'd' and 'e'
under attcompression, and says a dropped column's dictionaries stay until
the next rewrite. func-admin.sgml and alter_table.sgml mention lz4_dict
where it applies.

While I was in there, I added LZ4_stream_t to typedefs.list and ran
pgindent over the series.

Manu, the v13 containment fixes are unchanged in v14, so your repros
for them should still hold. The new parts are the header validation
in 0002 and the UPDATE fast path in 0004, if you want to point your
harness at them.

best.

-greg