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

Hi Greg,

On Mon, Nov 2, 2026 at 8:46 PM Greg Burd <greg(at)burd(dot)me> wrote:
> 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.

Thanks! I ran another full review and test pass on v14 (zstd+lz4 and
--without-zstd, with cassert, 002_pg_upgrade, isolation/repack-concurrently,
and logical decoding).

All of the v13 regressions and major issues look cleanly resolved:
- 002_pg_upgrade passes with active + retired dictionaries in
public.zstd_dict_upgrade (and pg_dump's remoteVersion < 200000 check
looks right too);
- CacheMemoryContext shows zero growth across repeated relcache
invalidations + dictionary writes (and your reasoning on not bumping
RELCACHE_INIT_FILEMAGIC and leaving heap_toast_delete() alone makes
sense to me);
- the UPDATE fast path in toast_tuple_init() preserves old inline
dictionaries and HOT updates on unrelated-column UPDATEs while still
recompressing and retiring under VACUUM FULL and REPACK CONCURRENTLY;
- dict_set_gates(), the pre-lock RELOID ownership check, and the SGML
doc updates all read well.

What I found in this pass is much smaller and mostly surgical:

1. Missed VARATT_IS_COMPRESSED_DICT() in check_domain_for_new_field()
(expandedrecord.c, 0004)

0004 updated 5 of the 6 VARATT_IS_EXTERNAL() checks in expandedrecord.c,
but missed the 6th one in check_domain_for_new_field()
(src/backend/utils/adt/expandedrecord.c:1553):

if (!attr->attbyval && attr->attlen == -1 &&
VARATT_IS_EXTERNAL(DatumGetPointer(newValue)))
dummy_erh->flags |= ER_FLAG_HAVE_EXTERNAL;

Because expanded_record_set_field_internal() runs
check_domain_for_new_field() before setting ER_FLAG_HAVE_EXTERNAL on erh,
assigning an inline dictionary datum to a field of a domain-over-composite
variable leaves ER_FLAG_HAVE_EXTERNAL unset on dummy_erh. If the domain's
CHECK expression flattens VALUE (e.g., stores it or passes it to a SQL
function), ER_get_flat_size() skips toast_flatten_tuple_to_datum() and the
raw inline dictionary datum escapes into another table/composite:

CREATE TYPE comp_t AS (a text, b int);
CREATE TABLE escaped_sink (tag text, val comp_t);
CREATE FUNCTION capture_comp_sql(v comp_t) RETURNS bool LANGUAGE sql AS $$
INSERT INTO escaped_sink VALUES ('sql_fn', v) RETURNING true;
$$;
CREATE DOMAIN dom_comp_sql AS comp_t CHECK (capture_comp_sql(VALUE));

CREATE TABLE src_dict (id int, d text COMPRESSION zstd_dict);
INSERT INTO src_dict
SELECT g, repeat(format('{"user":%s,"role":"admin","org":"acme"}', g), 40)
FROM generate_series(1, 100) g;
SELECT pg_train_toast_dictionary('src_dict', 'd', 4096);
INSERT INTO src_dict
VALUES (101, repeat('{"user":101,"role":"admin","org":"acme"}', 40));

DO $$
DECLARE v dom_comp_sql;
BEGIN
v.a := (SELECT d FROM src_dict WHERE id = 101);
END;
$$;

SELECT pg_column_compression((val).a) FROM escaped_sink; -- zstd_dict
DROP TABLE src_dict;
SELECT length((val).a) FROM escaped_sink;
-- ERROR: compression dictionary 16480 does not exist

Adding `|| VARATT_IS_COMPRESSED_DICT(DatumGetPointer(newValue))` there
closes it.

2. Missed toast_inline_compression_id() caller in toast_save_datum()
(toast_internals.c, 0002 & 0004)

toast_save_datum() (toast_internals.c:334 after 0004) still calls raw
VARDATA_COMPRESSED_GET_COMPRESS_METHOD(dval) followed by Assert(...):

else if (VARATT_IS_COMPRESSED(dval))
{
cmid = VARDATA_COMPRESSED_GET_COMPRESS_METHOD(dval);
...
Assert(cmid == TOAST_PGLZ_COMPRESSION_ID || ...);

When a heap tuple already has an untouched inline 4B_C attribute and an
UPDATE widens *another* column past TOAST_TUPLE_THRESHOLD (or INSERT ...
SELECT copies a non-dict inline 4B_C datum), toast_tuple_externalize()
passes that on-disk 4B_C datum straight into toast_save_datum():
- a truncated 4..8-byte header reads out-of-bounds;
- a corrupt va_cmid (3 or >= 6) hits FailedAssertion on cassert builds
(or writes an invalid external pointer on non-assert builds);
- a non-canonical long header (e.g., raw_method = 3, va_cmid = 0) gets
externalized as a plain pglz pointer with the 9-byte long header stored
in pg_toast chunks.

Switching toast_save_datum() to `cmid = toast_inline_compression_id(dval)`
and raising ERRCODE_DATA_CORRUPTED on TOAST_INVALID_COMPRESSION_ID (and
on unknown cmid values instead of Assert alone) closes that path.

(Two tiny related validation polish items if you want them:
- In toast_inline_compression_id(), rejecting unknown method IDs (>= 6,
or > TOAST_ZSTD_COMPRESSION_ID in 0002-0003) and checking VARSIZE >=
VARHDRSZ_COMPRESSED_DICT for the dict methods would turn
`pg_column_compression()`
on a corrupt va_cmid = 99 or 10-byte dict header into
ERRCODE_DATA_CORRUPTED instead of elog("invalid compression method id 99")
or returning 'zstd_dict' before detoast fails.
- In toast_external_info_get() / toast_get_compression_id(), enforcing the
same pointer-form vs. method check as verify_heapam.c (and raising
ERRCODE_DATA_CORRUPTED in toast_get_compression_id() when a compressed
external pointer decodes to TOAST_INVALID_COMPRESSION_ID) would make
external pointer validation symmetric with inline headers.)

With toast_save_datum() tightened, 0001-0003 look completely ready to
commit.

3. Temp tables with ON COMMIT DELETE ROWS miss toast_dictionary_retire()
(heap.c, 0004)

ExecuteTruncateGuts() handles explicit SQL TRUNCATE, but `CREATE TEMP TABLE
... ON COMMIT DELETE ROWS` truncates at commit via
PreCommit_on_commit_actions() -> heap_truncate(oids_to_truncate) in
src/backend/catalog/heap.c:3669, bypassing ExecuteTruncateGuts(). So in
long-lived / pooled sessions reusing an ON COMMIT DELETE ROWS temp table,
retraining leaves retired dictionaries behind in pg_toast_dictionary even
after COMMIT empties the table:

CREATE TEMP TABLE tmp_ocdr (id int, a text COMPRESSION zstd_dict)
ON COMMIT DELETE ROWS;
BEGIN;
INSERT INTO tmp_ocdr
SELECT g, repeat(format('{"k":%s,"kind":"alpha"}', g), 12)
FROM generate_series(1, 100) g;
SELECT pg_train_toast_dictionary('tmp_ocdr', 'a', 4096);
INSERT INTO tmp_ocdr
SELECT g, repeat(format('{"k":%s,"kind":"beta"}', g), 12)
FROM generate_series(101, 200) g;
SELECT pg_train_toast_dictionary('tmp_ocdr', 'a', 4096);
COMMIT;

SELECT count(*) FILTER (WHERE NOT tdcurrent)
FROM pg_toast_dictionary WHERE tdrelid = 'tmp_ocdr'::regclass; -- 1

Two small subtleties if you wire toast_dictionary_retire() into
heap_truncate():
- Put it in heap_truncate() rather than heap_truncate_one_rel(), since
ExecuteTruncateGuts() calls heap_truncate_one_rel() in its same-subxact
branch and already retires right afterward.
- Because pg_toast_dictionary.tddict can be TOASTed out-of-line,
CatalogTupleDelete() -> toast_delete_datum() asserts ActiveSnapshotSet().
PreCommit_on_commit_actions() (tablecmds.c:20095) already wraps
performMultipleDeletions() in PushActiveSnapshot(GetTransactionSnapshot())
/ PopActiveSnapshot() for that exact reason, so heap_truncate() there
needs an active snapshot too.

4. Moving rd_toastdictoids / rd_toastdictvalid from 0005 into 0004

In v14, 0004's toast_dictionary_current() still does a table_open() +
systable_beginscan() on pg_toast_dictionary for every compressed datum
written (even when a zstd_dict/lz4_dict column has no dictionary yet),
and 0005 replaces that with rd_toastdictoids / rd_toastdictvalid.

If a committer decides to apply 0001-0004 first while iterating on 0005's
ANALYZE policy, 0004 on its own would pay that per-datum catalog scan on
every write. Moving the relcache cache (and RelationDestroyRelation /
load_relcache_init_file cleanup) from 0005 into 0004 would make 0001-0004
self-sufficient.

5. Two minor gate-ordering items in toast_dictionary_maybe_evolve() (0005)

- In toast_dictionary.c:1422-1425, dict_current_info(..., &curbytes, true)
detoasts/copies tddict (up to 1 MB) and dict_count() scans the index
*before* checking min_rows, growth, and the 24h min_interval cooldown,
so steady-state no-op ANALYZEs still detoast tddict from
pg_toast_dictionary's TOAST table. Passing wantbytes = false initially
(and deferring dict_count() until after the growth/cooldown gates, and
fetching curbytes only at step (f) when OidIsValid(curid) && nheld > 0)
avoids that catalog TOAST read on normal ANALYZEs.

- In toast_dictionary.c:1429-1444, `if (totalrows < min_rows) goto done;`
runs *before* the `if (currows <= 0)` baseline update. If a manual
dictionary was trained before the first ANALYZE on a 600-row table
(tdtrainrows = 0) with min_rows = 10000, ANALYZE at 600 rows skips
setting tdtrainrows = 600; when the table later reaches 10,000 rows,
ANALYZE records 10,000 as the baseline instead of 600 and waits until
20,000 rows to evolve. Recording the currows <= 0 baseline before the
min_rows exit captures the true starting size.

Cheers,
Hannu

On Mon, Nov 2, 2026 at 8:47 PM Greg Burd <greg(at)burd(dot)me> wrote:
>
> 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
>