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

Hi Greg

> On Sun, Nov 9, 2026 at 17:54 UTC, Hannu Krosing <hannuk(at)google(dot)com> wrote:
> > Thanks! I ran another pass on v15 under cassert + UBSan, 002_pg_upgrade,
> > amcheck, and a streaming Hot Standby.
>
> Thanks again Hannu for giving this your thought and time. I can see
> that you are interested/invested in this thread, thanks for that effort.

Thanks!

I am still working on a (more widely ambitious :) ) Direct TOAST project
some parts of which also have the dictionary-based compression
implementation, so following this has been very informative.

This also makes me interested to see this one moving forward in a way
that will not blow up maintainer load in the future.

And of course I am also interested in getting the idea of having an
efficient compression in the core more widely accepted :)

So here is the overview of joint findings after analyzing v16 and v17 patches

On Sat, Nov 14, 2026 at 2:50 PM Greg Burd <greg(at)burd(dot)me> wrote:
> v17 is attached; it applies on master as of 77bba5454f9. There was no
> new review of v16, so this round comes from a full pass of my own over
> the series, held to the standard I'd expect a committer to apply.

Thanks! I had just finished a review pass on v16 when v17 arrived, so I
ran the full build matrix, pgindent (with DWARF-extracted typedefs),
ASan + UBSan + cassert, 002_pg_upgrade, and two-cluster logical
replication (REPLICA IDENTITY FULL across retraining + VACUUM FULL + DROP
COLUMN) on v17 as well.

All of my v15 items were resolved in v16, and the v17 cleanups read very
well -- especially normalizing toast_external_data in 0001,
toast_compression_id_resolve() + 007_verify_toast_pointer.pl in 0002, the
2^23 window cap + chunked slice loop in 0003, toast_flatten_tuple_for_wal()
and pg_toast_dictionaries in 0004, and dictionary_evolution_autovacuum +
XactReadOnly + TRUNCATE baseline reset in 0005. Your benchmark table for
non-users is very reassuring too.

Here is what turned up across the combined v16 + v17 pass:

=== Important items ===

1. Big-endian failure in contrib/amcheck/t/007_verify_toast_pointer.pl
(0002 & 0003)

In 007_verify_toast_pointer.pl:

my ($lp, $dataoff, $b4, $b5) = split /\|/, $row;
is("$b4/$b5", "1/18", "tuple $lp holds a plain on-disk TOAST pointer");
...
is("$b4/$b5", "1/19", "tuple $lp holds a long on-disk TOAST pointer");

In varatt.h, SET_VARTAG_1B_E writes va_header = 0x01 (1) on little-endian
machines, but writes va_header = 0x80 (128) under WORDS_BIGENDIAN (s390x,
ppc64, sparc64). set_method_bits() right below checks `(pack('L', 1) eq
pack('V', 1))`, so deriving `$va_ext_hdr = ... ? 0x01 : 0x80` for those
two assertions will keep big-endian buildfarm animals green.

2. RelationInvalidateRelation() leaves a rolled-back subtransaction's
dictionary OID cached in rd_toastdictoids (relcache.c, 0004)

0004 frees rd_toastdictoids and rd_toastdictvalid in
RelationDestroyRelation() and zeroes them in load_relcache_init_file(),
but does not clear them in RelationInvalidateRelation() (relcache.c:2556,
right next to rd_amcache).

When a subtransaction installs a dictionary D1 (dict_install() ->
CacheInvalidateRelcache(rel)), populates rel->rd_toastdictoids via a
write, and then rolls back while an outer statement holds `rel` open
(rd_refcnt > 0, e.g. inside a trigger), AtEOSubXact_Inval(false) runs
with IsTransactionState() == false. RelationFlushRelation() takes the
`!IsTransactionState()` branch (relcache.c:2869) and calls
RelationInvalidateRelation(rel) instead of RelationRebuildRelation(rel),
while the outer statement continues using its open Relation pointer as-is.

dict_cache_invalidate() evicts D1 from cdicts[] / lz4dicts[], but
rel->rd_toastdictoids still holds D1's OID, so when the outer statement
writes its row, toast_dictionary_current() returns D1 and dict_fetch()
fails with `ERROR: compression dictionary <D1> does not exist` -- even
when the column already had a valid committed dictionary D0:

CREATE FUNCTION doc(i int) RETURNS text LANGUAGE SQL IMMUTABLE AS $$
SELECT repeat(format('{"id":%s,"tenant":"acme","status":"active"}',
i), 50)
$$;
CREATE TABLE t_d0 (id int, f1 text COMPRESSION zstd_dict);
INSERT INTO t_d0 SELECT g, doc(g) FROM generate_series(1, 100) g;
SELECT pg_train_toast_dictionary('t_d0', 'f1', 4096); -- commits D0

CREATE FUNCTION trg_d0_fn() RETURNS trigger LANGUAGE plpgsql AS $$
BEGIN
IF NEW.id = 101 THEN
BEGIN
PERFORM pg_train_toast_dictionary('t_d0', 'f1', 4096);
INSERT INTO t_d0 VALUES (999, doc(999));
RAISE EXCEPTION 'abort subxact';
EXCEPTION WHEN OTHERS THEN
NULL;
END;
END IF;
RETURN NEW;
END $$;
CREATE TRIGGER trg_d0 BEFORE INSERT ON t_d0
FOR EACH ROW EXECUTE FUNCTION trg_d0_fn();

\c -
INSERT INTO t_d0 VALUES (101, doc(101));
-- ERROR: compression dictionary 16393 does not exist

Freeing and NULLing rd_toastdictoids and rd_toastdictvalid inside
RelationInvalidateRelation() (alongside rd_amcache) fixes this cleanly
and also covers RelationRebuildRelation()'s HistoricSnapshotActive()
early return.

=== Smaller correctness & consistency items ===

3. pg_train_toast_dictionary() ACL_MAINTAIN nuances (0004 & 0005)

Three small things to consider on the v17 switch from ownership to
ACL_MAINTAIN:
- Database-owner parity with ANALYZE: vacuum_is_permitted_for_relation()
(and RangeVarCallbackForStats) allows `(object_ownercheck(
DatabaseRelationId, MyDatabaseId, userid) && !relisshared)` in addition
to ACL_MAINTAIN. Right now a non-superuser database owner can install
a dictionary via ANALYZE in 0005, yet gets `permission denied` from
pg_train_toast_dictionary().
- Post-lock ACL recheck & pre-lock relkind: checking permissions before
table_open(relid, ShareUpdateExclusiveLock) avoids unprivileged lock
blocking, but re-checking after table_open() closes the TOCTOU window
if REVOKE MAINTAIN or ALTER TABLE OWNER TO committed while waiting on
the lock (and checking classform->relkind before table_open() avoids
locking views/sequences/partitioned/foreign tables and gives the same
error message on indexes).
- Retained-dictionary cap: in 0005, dictionary_evolution_max_retained (8)
and min_interval are PGC_SUSET so a non-owner MAINTAIN role cannot
churn ANALYZE to bloat pg_toast_dictionary; with ACL_MAINTAIN on
pg_train_toast_dictionary(rel, col, 1048576), a non-owner MAINTAIN role
can loop manual training without hitting max_retained unless manual
training either enforces max_retained (at least for non-owners) or
stays owner-only.

4. Sample starvation in pg_train_toast_dictionary() when dict_size <= 9175
on wide rows or early outliers (0004)

pg_train_toast_dictionary() slices up to DICT_SAMPLE_LIMIT (128 kB) per
row and breaks immediately on `samples.len + len > dict_size *
DICT_SAMPLE_FACTOR`, whereas zstd's ZDICT_trainFromBuffer() requires at
least 7 samples:
- On columns with >= 128 kB values, every dict_size in [256 .. 9175]
(including 4096 and 8192) breaks out with 0..6 samples and returns
NULL.
- Even on a normal small-row table (say 1,000 x 3 kB rows), a single
>= 128 kB outlier on block 0 causes dict_size = 1024 to break on row 0
with `nsamples = 0` without scanning rows 2..1000.

Capping the per-row slice in pg_train_toast_dictionary() to
`Min(DICT_SAMPLE_LIMIT, ((size_t) dict_size * DICT_SAMPLE_FACTOR) / 10)`
ensures even dict_size = 256 gathers ~2.5 kB prefixes across >= 10 rows
within the byte budget.

5. Missing DECLARE_FOREIGN_KEY in pg_toast_dictionary.h (0004)

pg_statistic.h and pg_attrdef.h both declare:

DECLARE_FOREIGN_KEY((tdrelid, tdattnum), pg_attribute, (attrelid, attnum));

so Catalog.pm emits the composite FK into system_fk_info.h for
pg_get_catalog_foreign_keys() and the oidjoins test. Since dropped
columns keep their pg_attribute row until heap_drop_with_catalog() (where
RemoveToastDictionaries() runs before DeleteAttributeTuples()), adding
that macro to pg_toast_dictionary.h (and updating oidjoins.out) gives
free catalog FK validation.

=== Minor polish ===

- 0002 (detoast.c / toast_compression.c / 007_verify_toast_pointer.pl):
verify_heapam() flags `!is_compressed && VARTAG_IS_ONDISK_LONG(tag)`
("is not compressed but has a long TOAST pointer"), whereas
toast_fetch_datum[_slice]() and toast_get_compression_id() accept an
uncompressed long pointer silently; rejecting that case at runtime (and
adding a test case to 007_verify_toast_pointer.pl) would make both
match.
- 0004 (toast_dictionary.c / compression_zstd_dict.sql):
* dict_get_ddict() creates dict_dctx without setting
ZSTD_d_windowLogMax = ZSTD_TOAST_WINDOWLOG_MAX (unlike zstd_get_dctx()
in 0003).
* compression_zstd_dict.sql comments "-- values nested in containers:
rows, arrays, ranges of text are flattened", though cmnest only tests
a composite and an array (we tested textrange/textmultirange and they
flatten properly).
- 0005 (matview.c / toast_dictionary.c):
* `REFRESH MATERIALIZED VIEW mv WITH NO DATA` swaps in an empty heap
via finish_heap_swap() -> toast_dictionary_retire(OIDOldHeap, false),
leaving the pre-empty tdtrainrows on a 0-row unpopulated matview
instead of resetting tdtrainrows = 0 like TRUNCATE does.
* zstd_dict_compressed_total() still has bare
`out = palloc(ZSTD_compressBound(maxlen));` without ZSTD_isError() /
AllocSizeIsValid() (and lz4_dict_compressed_total() omits
!AllocSizeIsValid(bound)).
* Under `--without-zstd --with-lz4`, an `#ifndef USE_ZSTD return;
#endif` at step (a) of toast_dictionary_maybe_evolve() avoids wasted
TOAST reads before dict_train_zdict() returns 0.

Cheers,
Hannu