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

Hi Greg,

On Fri, Nov 6, 2026 at 7:31 PM Greg Burd <greg(at)burd(dot)me> wrote:
> Thanks, Hannu, and thanks, Manu, for the containment audit and the
> UPDATE numbers. v15 is attached; it applies on master as of
> 72b9498df9a.

Thanks! I ran another pass on v15 under cassert + UBSan, 002_pg_upgrade,
amcheck, and a streaming Hot Standby.

All five v14 items look cleanly resolved and tested (and keeping
VARHDRSZ_COMPRESSED_DICT out of toast_compression.h makes sense). Modulo
rebasing onto today's 4a95f56c04d (noted in #5 below), 0001-0003 look
Ready for Committer to me, and 0004 is very close as well.

Here is what turned up on 0004-0005 (plus the rebase note):

1. Unbounded sample buffer (> 1 GB StringInfo failure + stuck gates) in
toast_dictionary_maybe_evolve() (0005)

pg_train_toast_dictionary() stops scanning once nsamples >=
DICT_MAX_SAMPLES (10,000) or samples.len + len > dict_size *
DICT_SAMPLE_FACTOR (6.4 MB for a 64 kB dictionary).

By contrast, toast_dictionary_maybe_evolve() (toast_dictionary.c:1499)
caps dictsize at DICT_DEFAULT_SIZE (64 kB), yet loops over all numrows
(30,000 at default_statistics_target = 100, and up to 3,000,000 at
SET STATISTICS 10000), detoasting up to DICT_SAMPLE_LIMIT (128 kB) per
row into StringInfoData train and held with neither a sample-count cap
nor a byte-budget cap.

Even at default_statistics_target = 100, 24,000 * 128 kB = 3.0 GB, which
blows past MaxAllocSize (1 GB) on wide rows:

SET dictionary_evolution_min_interval = 0;
CREATE TABLE big_evo (id int, f1 text COMPRESSION zstd_dict);
ALTER TABLE big_evo ALTER COLUMN f1 SET (
dictionary_evolution = on,
dictionary_evolution_min_rows = 50,
dictionary_evolution_growth = 1.0
);
INSERT INTO big_evo
SELECT g, format('{"id":%s,"hdr":"tenant-alpha"}', g) ||
repeat('{"k":"val","status":"active","role":"member"},', 2900)
FROM generate_series(1, 11000) g;

-- Table is ~1.6 MB on disk, yet ANALYZE allocates > 1.3 GB and fails:
ANALYZE big_evo;
-- WARNING: automatic dictionary evolution for column "f1" of "big_evo"
-- was skipped: string buffer exceeds maximum allowed length
-- (1073741823 bytes)

Two consequences:
- Initial evolution never trains a dictionary on wide columns (even though
pg_train_toast_dictionary('big_evo', 'f1') succeeds in milliseconds
using 6.4 MB).
- Worse, when a column already has a dictionary and its growth + cooldown
gates open, enlargeStringInfo() throws *before* `declined:` calls
dict_set_gates(). PG_CATCH() rolls back the subtransaction, leaving
tdtraintime and tdtrainrows unchanged -- so every subsequent ANALYZE /
autoanalyze repeats the > 1 GB TOAST read, > 1 GB allocation, and
WARNING forever.

Capping the sample loop in toast_dictionary_maybe_evolve() at
DICT_MAX_SAMPLES and a byte budget matching DICT_DEFAULT_SIZE *
DICT_SAMPLE_FACTOR (~6.4 MB for train + ~1.6 MB for held) fixes both.
Note that acquire_sample_rows() returns rows[0..numrows-1] in physical
block order, so when numrows is large, striding across rows[] (rather
than taking only the first ~60 rows when values are ~128 kB) will keep
the sample spread across the whole table. (Also a tiny commit-message
nit on 0005: "no extra I/O" holds for inline values, whereas external
values do fetch up to 128 kB from TOAST that std_typanalyze skipped above
WIDTH_THRESHOLD = 1024; the ~8 MB cap bounds that I/O nicely too.)

2. Hot Standby recovery guard (and READ ONLY txn semantics) in
pg_train_toast_dictionary() (0004 & 0005)

On a Hot Standby, `ANALYZE t` fails cleanly via
PreventCommandDuringRecovery() ("ERROR: cannot execute ANALYZE during
recovery"), whereas `SELECT pg_train_toast_dictionary('t', 'f1')` passes
the RELOID ownership check and fails inside table_open(relid,
ShareUpdateExclusiveLock) with the low-level lock error:

ERROR: cannot acquire lock mode ShareUpdateExclusiveLock on database
objects while recovery is in progress

Peer SQL-callable maintenance functions (pg_restore_attribute_stats(),
gin_clean_pending_list(), brin_summarize_range()) guard against recovery
up front, so pg_train_toast_dictionary() should at minimum call
PreventCommandDuringRecovery("pg_train_toast_dictionary()") before the
RELOID lookup.

Relatedly: because ClassifyUtilityCommandAsReadOnly() allows ANALYZE in
READ ONLY transactions on primary, both `BEGIN READ ONLY; ANALYZE t;` and
`BEGIN READ ONLY; SELECT pg_train_toast_dictionary(...);` currently write
to pg_toast_dictionary. Allowing both on primary (matching ANALYZE /
pg_restore_attribute_stats, with PreventCommandDuringRecovery() on
pg_train_toast_dictionary()) seems fine to me; if instead you'd rather
forbid catalog/OID writes in XactReadOnly transactions, then both
PreventCommandIfReadOnly("pg_train_toast_dictionary()") and `if
(XactReadOnly) return;` in toast_dictionary_maybe_evolve() should check
it together so manual and automatic training behave the same way.

3. Missing relkind / catalog guard in toast_dictionary_maybe_evolve()
(0005)

dict_check_column() in 0004 restricts dictionaries to RELKIND_RELATION
and RELKIND_MATVIEW (!IsCatalogRelation()), and pg_dump only dumps those.
However, PostgreSQL accepts `CREATE FOREIGN TABLE ft (id int, f1 text
COMPRESSION zstd_dict)` + `ALTER FOREIGN TABLE ft ALTER COLUMN f1 SET
(dictionary_evolution = on)`, and `ANALYZE ft` calls
toast_dictionary_maybe_evolve(), which currently doesn't check relkind or
IsCatalogRelation(onerel) and will install orphaned rows in
pg_toast_dictionary if the FDW returns sample rows. Adding the same
(RELKIND_RELATION || RELKIND_MATVIEW) && !IsCatalogRelation(onerel) guard
at step (a) closes that.

4. attstorage (STORAGE PLAIN / EXTERNAL) consistency (0004 & 0005)

`ALTER TABLE ... ALTER COLUMN ... SET STORAGE PLAIN` (or `EXTERNAL`)
leaves attcompression set ('d'/'e') while disabling compression
(heaptoast.c only compresses EXTENDED and MAIN). Right now:
- both pg_train_toast_dictionary() and toast_dictionary_maybe_evolve()
still train and install dictionaries on PLAIN / EXTERNAL columns that
the toaster will never use; and
- after `SET STORAGE EXTERNAL` (or `PLAIN`) + `VACUUM FULL`, external
dict values are decompressed, yet dict_column_uses_dict() still returns
true, so toast_dictionary_retire() never retires tdcurrent.

Checking IsStorageCompressible(att->attstorage) in dict_check_column()
and toast_dictionary_maybe_evolve() avoids training unused dictionaries.
Checking it in toast_dictionary_keep() and dict_column_uses_dict() as
well would let `SET STORAGE PLAIN/EXTERNAL` + `VACUUM FULL` decompress
inline dict values and retire all dictionaries for the column (with the
expected caveat that if an inline dict value decompresses past
MaxHeapTupleSize under STORAGE PLAIN, VACUUM FULL raises "row is too big",
matching what master already does for external compressed datums after
SET STORAGE PLAIN).

5. Rebase note on 4a95f56c04d + minor polish

- Commit 4a95f56c04d ("Harden some allocations against integer
wraparound", committed today) conflicts with 0002 in varatt.h,
detoast.c, and toast_compression.c, and adds AllocSizeIsValid() checks
in pglz_* and lz4_*. When rebasing 0003-0005, mirroring those checks
in the new codecs keeps everything consistent:
* zstd_compress_datum(), zstd_dict_compress_datum(), and
zstd_dict_compressed_total(): check `ZSTD_isError(max_size) ||
!AllocSizeIsValid(max_size)` before palloc;
* lz4_dict_compress_datum(): check `max_size <= 0 ||
!AllocSizeIsValid(max_size)` (LZ4_compressBound() returns 0 on
oversize input, where AllocSizeIsValid(0) is true);
* zstd_decompress_datum() and {zstd,lz4}_dict_decompress_datum():
`Assert(AllocSizeIsValid(VARDATA_COMPRESSED_GET_EXTSIZE(value)))`;
* zstd_decompress_datum_slice() and
{zstd,lz4}_dict_decompress_datum_slice(): check
`!AllocSizeIsValid(slicelength)` before palloc / comparison.

- Minor polish:
* doc/src/sgml/ref/alter_table.sgml: ~20 lines above 0005's new
paragraph, the existing text still says "Currently, the only defined
per-attribute options are n_distinct and n_distinct_inherited". It
may also help to mention that per-attribute options are not inherited
by partitions, so dictionary_evolution* must be set on leaf
partitions.
* detoast.h: toast_external_info_get() rejects a long pointer naming
pglz/lz4 (and could also check cmid > TOAST_LAST_COMPRESSION_ID),
while a corrupt *plain* pointer with method bits == 2 still decodes
as zstd in pg_column_compression(); adding the symmetric `else if`
branch makes external and inline validation match completely.
* toast_dictionary.c: dict_install() already has `Relation rel` open,
so CacheInvalidateRelcache(rel) avoids the extra RELOID syscache
lookup inside CacheInvalidateRelcacheByRelid().

Cheers,
Hannu