From c68e9c996dbaaece242e8f1ea952f6719b0fabeb Mon Sep 17 00:00:00 2001 From: Greg Burd Date: Fri, 30 Oct 2026 12:28:47 -0400 Subject: [PATCH v13 6/6] Address v12 review feedback from Hannu Krosing and Manu A combined set of fixes from the v12 review, kept as one commit here for review; it is meant to be folded into patches 0001-0005 before commit. Grouped by the patch each change belongs to. 0001-0003 (extensible encoding, stateless zstd): - Do not read the long-form method byte of a compressed-in-line datum before checking the datum is at least VARHDRSZ_COMPRESSED_LONG bytes, and reject a long header whose method byte holds pglz, lz4, or the invalid id. A corrupt 8-byte datum with the long method bits could otherwise read one byte past the allocation or decode from the wrong offset (VARDATA_COMPRESSED_GET_RAW_METHOD(), toast_decompress_datum()). - Reuse a backend-static ZSTD_CCtx for plain zstd compression instead of a one-shot ZSTD_compress() that allocates and frees a ~1.3 MB context per value. - CHECK_FOR_INTERRUPTS() in the zstd slice decompression loop. - toast_external_info_get() reports an uncompressed external datum as TOAST_INVALID_COMPRESSION_ID rather than leaving the raw method bits (which read as pglz), so toast_external_data is self-describing. 0004 (dictionaries): - lz4-without-zstd builds: dict_fetch() is shared across the two codecs; only the zstd invalidation callback stays under USE_ZSTD. - An indirect TOAST pointer to an inline dictionary datum, and a dictionary datum passed through datumTransfer()/SPI_datumTransfer(), are now decompressed instead of copied verbatim, closing two containment escapes. - pg_train_toast_dictionary() checks ownership before taking ShareUpdateExclusiveLock, rejects other sessions' temporary tables, and does its sampling and training in a short-lived memory context. - TRUNCATE retires a relation's non-current dictionaries, as a rewrite does. - The dictionary caches evict only the changed entry on invalidation (matching the precomputed syscache hash, never touching the catalog from the callback) instead of wiping the backend's whole cache. - The lz4 compress path uses a backend-static stream reset per value, and the held-out evaluation allocates its output buffer before creating the codec's dictionary object, so an OOM longjmp cannot leak a malloc'd CDict or lz4 stream. - pg_upgrade carries tdtrainrows and tdtraintime through binary_upgrade_add_toast_dictionary(), so the evolution gates are unchanged across an upgrade. 0005 (evolution): - The current-dictionary OID cache in the relcache is a per-column array, so a write into a table with more than one dictionary column no longer misses the cache on every row. - The evolution GUCs are PGC_SUSET, so a role with only MAINTAIN cannot lower the margin or raise the cap to churn dictionaries. - pg_train_toast_dictionary() clamps a reltuples estimate of -1 (a table not yet analyzed) to 0, so the growth gate does not fire forever. - compression_dict_evolve regression test uses rows wide enough to be TOAST-compressed on disk, keeps both dictionary generations present at once, and checks each still reads back correctly. Reviewed-by: Hannu Krosing Reviewed-by: Manuel Reyes Bravo --- src/backend/access/common/detoast.c | 49 +++++ src/backend/access/common/toast_compression.c | 38 +++- src/backend/access/common/toast_dictionary.c | 201 +++++++++++++++--- src/backend/commands/tablecmds.c | 9 + src/backend/utils/adt/datum.c | 14 ++ src/backend/utils/misc/guc_parameters.dat | 6 +- src/bin/pg_dump/pg_dump.c | 9 +- src/include/access/detoast.h | 9 + src/include/access/toast_dictionary.h | 2 +- src/include/catalog/pg_proc.dat | 2 +- src/include/utils/rel.h | 19 +- src/include/varatt.h | 14 ++ .../expected/compression_dict_evolve.out | 69 +++--- .../regress/sql/compression_dict_evolve.sql | 52 +++-- 14 files changed, 393 insertions(+), 100 deletions(-) diff --git a/src/backend/access/common/detoast.c b/src/backend/access/common/detoast.c index 04fa29f91aa..204afafcba2 100644 --- a/src/backend/access/common/detoast.c +++ b/src/backend/access/common/detoast.c @@ -81,6 +81,16 @@ detoast_external_attr(varlena *attr) if (VARATT_IS_EXTERNAL(attr)) return detoast_external_attr(attr); + /* + * The indirect pointer may reference an inline dictionary-compressed + * datum, which is not VARATT_IS_EXTERNAL but still depends on a + * catalog row of its owning column, so it must be decompressed here + * too; otherwise a still-compressed datum would be returned and read + * after its dictionary could be retired. + */ + if (VARATT_IS_COMPRESSED_DICT(attr)) + return toast_dict_decompress_datum(attr); + /* * Copy into the caller's memory context, in case caller tries to * pfree the result. @@ -508,11 +518,40 @@ toast_decompress_datum(varlena *attr) Assert(VARATT_IS_COMPRESSED(attr)); + /* + * A long-form compressed header carries the method in a byte of its own + * (va_cmid) past the four-byte length and tcinfo words, so it is only + * safe to read that byte once the datum is known to be at least + * VARHDRSZ_COMPRESSED_LONG bytes. A corrupt short datum whose method + * bits happen to be VARLENA_COMPRESS_METHOD_LONG would otherwise make + * VARDATA_COMPRESSED_GET_COMPRESS_METHOD() read one byte past the datum. + * Reject such a datum before decoding the method. + */ + if ((VARDATA_COMPRESSED_GET_RAW_METHOD(attr) == VARLENA_COMPRESS_METHOD_LONG) && + VARSIZE(attr) < VARHDRSZ_COMPRESSED_LONG) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("compressed data is corrupt"))); + /* * Fetch the compression method id stored in the compression header and * decompress the data using the appropriate decompression routine. */ cmid = VARDATA_COMPRESSED_GET_COMPRESS_METHOD(attr); + + /* + * A long-form header must carry a method that needs its own byte; pglz + * and lz4 use the plain header, so finding their ids (or the invalid id) + * in va_cmid means the datum is corrupt and would otherwise be decoded + * from the wrong offset. + */ + if (VARDATA_COMPRESSED_GET_RAW_METHOD(attr) == VARLENA_COMPRESS_METHOD_LONG && + (cmid == TOAST_INVALID_COMPRESSION_ID || + !toast_compression_id_needs_cmid_byte(cmid))) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("compressed data is corrupt"))); + switch (cmid) { case TOAST_PGLZ_COMPRESSION_ID: @@ -556,6 +595,16 @@ toast_decompress_datum_slice(varlena *attr, int32 slicelength) if ((uint32) slicelength >= VARDATA_COMPRESSED_GET_EXTSIZE(attr)) return toast_decompress_datum(attr); + /* + * As in toast_decompress_datum(), do not read the long-form method byte + * of a datum too short to contain it. + */ + if ((VARDATA_COMPRESSED_GET_RAW_METHOD(attr) == VARLENA_COMPRESS_METHOD_LONG) && + VARSIZE(attr) < VARHDRSZ_COMPRESSED_LONG) + ereport(ERROR, + (errcode(ERRCODE_DATA_CORRUPTED), + errmsg_internal("compressed data is corrupt"))); + /* * Fetch the compression method id stored in the compression header and * decompress the data slice using the appropriate decompression routine. diff --git a/src/backend/access/common/toast_compression.c b/src/backend/access/common/toast_compression.c index 04d2d9ba606..f4a9b77fdf6 100644 --- a/src/backend/access/common/toast_compression.c +++ b/src/backend/access/common/toast_compression.c @@ -24,6 +24,7 @@ #include "access/detoast.h" #include "access/toast_compression.h" #include "common/pg_lzcompress.h" +#include "miscadmin.h" #include "varatt.h" /* GUC */ @@ -67,6 +68,27 @@ zstd_get_dctx(void) ZSTD_DCtx_reset(zstd_dctx, ZSTD_reset_session_only); return zstd_dctx; } + +/* + * A backend-lifetime ZSTD_CCtx for plain (non-dictionary) compression, kept + * for the same reason as the decompression context above: a one-shot + * ZSTD_compress() allocates and frees a compression context of over a + * megabyte for every value, which dominates a bulk load. The dictionary + * path keeps its own context (dict_cctx in toast_dictionary.c). + */ +static ZSTD_CCtx *zstd_cctx = NULL; + +static ZSTD_CCtx * +zstd_get_cctx(void) +{ + if (zstd_cctx == NULL) + { + zstd_cctx = ZSTD_createCCtx(); + if (zstd_cctx == NULL) + elog(ERROR, "could not create zstd compression context"); + } + return zstd_cctx; +} #endif #define NO_COMPRESSION_SUPPORT(method) \ @@ -327,11 +349,12 @@ zstd_compress_datum(const varlena *value) max_size = ZSTD_compressBound(valsize); tmp = (varlena *) palloc(max_size + VARHDRSZ_COMPRESSED_LONG); - len = ZSTD_compress((char *) tmp + VARHDRSZ_COMPRESSED_LONG, - max_size, - VARDATA_ANY(value), - valsize, - ZSTD_CLEVEL_DEFAULT); + len = ZSTD_compressCCtx(zstd_get_cctx(), + (char *) tmp + VARHDRSZ_COMPRESSED_LONG, + max_size, + VARDATA_ANY(value), + valsize, + ZSTD_CLEVEL_DEFAULT); if (ZSTD_isError(len)) elog(ERROR, "zstd compression failed: %s", ZSTD_getErrorName(len)); @@ -448,7 +471,10 @@ zstd_decompress_datum_slice(const varlena *value, int32 slicelength) while (outbuf.pos < outbuf.size) { - size_t ret = ZSTD_decompressStream(dctx, &outbuf, &inbuf); + size_t ret; + + CHECK_FOR_INTERRUPTS(); + ret = ZSTD_decompressStream(dctx, &outbuf, &inbuf); if (ZSTD_isError(ret)) { diff --git a/src/backend/access/common/toast_dictionary.c b/src/backend/access/common/toast_dictionary.c index a4f61c63e56..b9c4d6ec0ad 100644 --- a/src/backend/access/common/toast_dictionary.c +++ b/src/backend/access/common/toast_dictionary.c @@ -71,6 +71,7 @@ #include "fmgr.h" #include "lib/stringinfo.h" #include "miscadmin.h" +#include "nodes/bitmapset.h" #include "utils/acl.h" #include "utils/attoptcache.h" #include "utils/builtins.h" @@ -223,9 +224,15 @@ toast_dictionary_current(Relation rel, AttrNumber attnum) SysScanDesc scan; HeapTuple tup; Oid result = InvalidOid; + int natts = RelationGetNumberOfAttributes(rel); + MemoryContext oldcxt; - if (rel->rd_toastdictattnum == attnum) - return rel->rd_toastdictoid; + Assert(attnum >= 1 && attnum <= natts); + + /* fast path: a filled per-column cache entry */ + if (rel->rd_toastdictoids != NULL && + bms_is_member(attnum, rel->rd_toastdictvalid)) + return rel->rd_toastdictoids[attnum - 1]; drel = table_open(ToastDictionaryRelationId, AccessShareLock); tup = dict_lookup(dict_owner_relid(rel), attnum, true, drel, &scan); @@ -234,8 +241,18 @@ toast_dictionary_current(Relation rel, AttrNumber attnum) systable_endscan(scan); table_close(drel, AccessShareLock); - rel->rd_toastdictoid = result; - rel->rd_toastdictattnum = attnum; + /* + * Cache the result in a per-column array, kept in CacheMemoryContext so + * it lives as long as the relcache entry and is freed with it; a relcache + * invalidation from dict_install() rebuilds the entry and drops it. + */ + oldcxt = MemoryContextSwitchTo(CacheMemoryContext); + if (rel->rd_toastdictoids == NULL) + rel->rd_toastdictoids = palloc0_array(Oid, natts); + rel->rd_toastdictoids[attnum - 1] = result; + rel->rd_toastdictvalid = bms_add_member(rel->rd_toastdictvalid, attnum); + MemoryContextSwitchTo(oldcxt); + return result; } @@ -345,9 +362,11 @@ dict_cache_victim(Oid *ids, uint64 *used) * DICT_CACHE_SIZE entries per direction. */ static Oid cdict_ids[DICT_CACHE_SIZE]; +static uint32 cdict_hashes[DICT_CACHE_SIZE]; static uint64 cdict_used[DICT_CACHE_SIZE]; static ZSTD_CDict *cdicts[DICT_CACHE_SIZE]; static Oid ddict_ids[DICT_CACHE_SIZE]; +static uint32 ddict_hashes[DICT_CACHE_SIZE]; static uint64 ddict_used[DICT_CACHE_SIZE]; static ZSTD_DDict *ddicts[DICT_CACHE_SIZE]; static ZSTD_CCtx *dict_cctx = NULL; @@ -358,13 +377,28 @@ dict_cache_invalidate(Datum arg, SysCacheIdentifier cacheid, uint32 hashvalue) { for (int i = 0; i < DICT_CACHE_SIZE; i++) { - if (cdicts[i]) + /* + * hashvalue == 0 is the "reset everything" signal; otherwise evict + * only entries whose dictionary row hashes to the changed one, so an + * unrelated pg_toast_dictionary change elsewhere does not throw away + * a backend's whole cache. The hash is the one computed when the + * entry was cached; this callback must not touch the catalogs, since + * it can run when no transaction is in progress. + */ + if (cdicts[i] && + (hashvalue == 0 || cdict_hashes[i] == hashvalue)) + { ZSTD_freeCDict(cdicts[i]); - if (ddicts[i]) + cdicts[i] = NULL; + cdict_ids[i] = InvalidOid; + } + if (ddicts[i] && + (hashvalue == 0 || ddict_hashes[i] == hashvalue)) + { ZSTD_freeDDict(ddicts[i]); - cdicts[i] = NULL; - ddicts[i] = NULL; - cdict_ids[i] = ddict_ids[i] = InvalidOid; + ddicts[i] = NULL; + ddict_ids[i] = InvalidOid; + } } } #endif /* USE_ZSTD */ @@ -423,6 +457,8 @@ dict_get_cdict(Oid dictid) if (cdicts[slot] == NULL) elog(ERROR, "could not load compression dictionary %u", dictid); cdict_ids[slot] = dictid; + cdict_hashes[slot] = GetSysCacheHashValue1(TOASTDICTOID, + ObjectIdGetDatum(dictid)); cdict_used[slot] = ++dict_cache_clock; } if (dict_cctx == NULL && (dict_cctx = ZSTD_createCCtx()) == NULL) @@ -448,6 +484,8 @@ dict_get_ddict(Oid dictid) if (ddicts[slot] == NULL) elog(ERROR, "could not load compression dictionary %u", dictid); ddict_ids[slot] = dictid; + ddict_hashes[slot] = GetSysCacheHashValue1(TOASTDICTOID, + ObjectIdGetDatum(dictid)); ddict_used[slot] = ++dict_cache_clock; } if (dict_dctx == NULL && (dict_dctx = ZSTD_createDCtx()) == NULL) @@ -565,10 +603,21 @@ zstd_dict_compressed_total(const char *dict, size_t dictlen, maxlen = Max(maxlen, sizes[i]); if (dict_cctx == NULL && (dict_cctx = ZSTD_createCCtx()) == NULL) elog(ERROR, "could not create zstd compression context"); + + /* + * Allocate the output buffer before creating the CDict, so no palloc + * (which can ereport on OOM) sits between ZSTD_createCDict() and + * ZSTD_freeCDict(): toast_dictionary_maybe_evolve() catches errors in a + * subtransaction, and a longjmp from between the two would leak the + * malloc'd CDict for the life of the backend. + */ + out = palloc(ZSTD_compressBound(maxlen)); cd = ZSTD_createCDict(dict, dictlen, ZSTD_CLEVEL_DEFAULT); if (cd == NULL) + { + pfree(out); return 0; - out = palloc(ZSTD_compressBound(maxlen)); + } for (int i = 0; i < nvals; i++) { size_t clen = ZSTD_compress_usingCDict(dict_cctx, out, @@ -598,18 +647,42 @@ zstd_dict_compressed_total(const char *dict, size_t dictlen, * pg_toast_dictionary. */ static Oid lz4dict_ids[DICT_CACHE_SIZE]; +static uint32 lz4dict_hashes[DICT_CACHE_SIZE]; static uint64 lz4dict_used[DICT_CACHE_SIZE]; static bytea *lz4dicts[DICT_CACHE_SIZE]; +/* + * Backend-static lz4 compression stream, reset before each use. Keeping it + * static avoids LZ4_createStream()/LZ4_freeStream() per value (a malloc that + * would also leak on an OOM longjmp out of the compression path), as the zstd + * side keeps a static CCtx. LZ4_stream_t is a fixed-size POD, safe as a + * file-scope static. + */ +static LZ4_stream_t lz4_cstream_storage; +static LZ4_stream_t *lz4_cstream = NULL; + +static void +lz4_cstream_reset(void) +{ + if (lz4_cstream == NULL) + lz4_cstream = LZ4_initStream(&lz4_cstream_storage, + sizeof(lz4_cstream_storage)); + else + LZ4_resetStream_fast(lz4_cstream); +} + static void lz4_dict_cache_invalidate(Datum arg, SysCacheIdentifier cacheid, uint32 hashvalue) { for (int i = 0; i < DICT_CACHE_SIZE; i++) { - if (lz4dicts[i]) + if (lz4dicts[i] && + (hashvalue == 0 || lz4dict_hashes[i] == hashvalue)) + { pfree(lz4dicts[i]); - lz4dicts[i] = NULL; - lz4dict_ids[i] = InvalidOid; + lz4dicts[i] = NULL; + lz4dict_ids[i] = InvalidOid; + } } } @@ -645,6 +718,8 @@ lz4_dict_get(Oid dictid) pfree(lz4dicts[slot]); lz4dicts[slot] = cached; lz4dict_ids[slot] = dictid; + lz4dict_hashes[slot] = GetSysCacheHashValue1(TOASTDICTOID, + ObjectIdGetDatum(dictid)); lz4dict_used[slot] = ++dict_cache_clock; } return lz4dicts[slot]; @@ -667,21 +742,23 @@ lz4_dict_compress_datum(const varlena *value, Oid dictid) int32 valsize = VARSIZE_ANY_EXHDR(value); int32 max_size = LZ4_compressBound(valsize); bytea *dict = lz4_dict_get(dictid); - LZ4_stream_t *stream; varlena *tmp; int32 len; - stream = LZ4_createStream(); - if (stream == NULL) - elog(ERROR, "could not create lz4 stream"); - LZ4_loadDict(stream, VARDATA_ANY(dict), VARSIZE_ANY_EXHDR(dict)); + /* + * Use a backend-static stream, reset per call, rather than + * LZ4_createStream(): a malloc'd stream between here and the palloc below + * would leak on an OOM longjmp, and avoiding the per-value malloc/free is + * cheaper on bulk loads. + */ + lz4_cstream_reset(); + LZ4_loadDict(lz4_cstream, VARDATA_ANY(dict), VARSIZE_ANY_EXHDR(dict)); tmp = (varlena *) palloc(max_size + VARHDRSZ_COMPRESSED_DICT); - len = LZ4_compress_fast_continue(stream, + len = LZ4_compress_fast_continue(lz4_cstream, VARDATA_ANY(value), (char *) tmp + VARHDRSZ_COMPRESSED_DICT, valsize, max_size, 1); - LZ4_freeStream(stream); if (len <= 0) elog(ERROR, "lz4 compression failed"); if (len > valsize) @@ -758,20 +835,17 @@ lz4_dict_compressed_total(const char *dict, size_t dictlen, for (int i = 0; i < nvals; i++) { - LZ4_stream_t *stream; int32 bound = LZ4_compressBound(sizes[i]); char *out; int32 clen; if (bound <= 0) return 0; - stream = LZ4_createStream(); - if (stream == NULL) - return 0; - LZ4_loadDict(stream, dict, dictlen); + /* static stream reset per value; no malloc to leak on OOM */ out = palloc(bound); - clen = LZ4_compress_fast_continue(stream, vals, out, sizes[i], bound, 1); - LZ4_freeStream(stream); + lz4_cstream_reset(); + LZ4_loadDict(lz4_cstream, dict, dictlen); + clen = LZ4_compress_fast_continue(lz4_cstream, vals, out, sizes[i], bound, 1); pfree(out); if (clen <= 0) return 0; @@ -918,10 +992,13 @@ toast_dictionary_retire(Oid relid) * previous current one. dictid may be given (binary upgrade) or chosen here. * trainrows is the reltuples estimate at training time, recorded for the * auto-evolution growth trigger; pass 0 when not training from a live count. + * traintime is the training timestamp to record (0 means now); pg_upgrade + * passes the original so the evolution cooldown survives an upgrade. */ static Oid dict_install(Relation rel, AttrNumber attnum, Oid dictid, bool current, - int64 trainrows, const void *dict, size_t dictlen) + int64 trainrows, TimestampTz traintime, + const void *dict, size_t dictlen) { Relation drel; SysScanDesc scan; @@ -960,7 +1037,7 @@ dict_install(Relation rel, AttrNumber attnum, Oid dictid, bool current, values[Anum_pg_toast_dictionary_tdcurrent - 1] = BoolGetDatum(current); values[Anum_pg_toast_dictionary_tdtrainrows - 1] = Int64GetDatum(trainrows); values[Anum_pg_toast_dictionary_tdtraintime - 1] = - TimestampTzGetDatum(GetCurrentTimestamp()); + TimestampTzGetDatum(traintime != 0 ? traintime : GetCurrentTimestamp()); values[Anum_pg_toast_dictionary_tddict - 1] = PointerGetDatum(dictval); tup = heap_form_tuple(RelationGetDescr(drel), values, nulls); CatalogTupleInsert(drel, tup); @@ -1003,6 +1080,10 @@ dict_check_column(Relation rel, const char *colname) ereport(ERROR, (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), errmsg("cannot use compression dictionaries on a system catalog"))); + if (RELATION_IS_OTHER_TEMP(rel)) + ereport(ERROR, + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED), + errmsg("cannot use compression dictionaries on temporary tables of other sessions"))); attnum = get_attnum(RelationGetRelid(rel), colname); if (attnum <= 0) @@ -1055,6 +1136,9 @@ pg_train_toast_dictionary(PG_FUNCTION_ARGS) void *dictbuf; size_t dictlen; Oid dictid; + int64 trainrows; + MemoryContext workcxt, + oldcxt; if (dict_size < DICT_MIN_SIZE || dict_size > DICT_MAX_SIZE) ereport(ERROR, @@ -1062,9 +1146,40 @@ pg_train_toast_dictionary(PG_FUNCTION_ARGS) errmsg("dictionary size must be between %d and %d bytes", DICT_MIN_SIZE, DICT_MAX_SIZE))); + /* + * Check ownership before taking a lock on the table, so that an + * unprivileged caller (the function is executable by PUBLIC) cannot take + * ShareUpdateExclusiveLock on an arbitrary relation and block its + * maintenance just by failing the later ACL check, the way VACUUM and + * ANALYZE check first. + */ + if (!object_ownercheck(RelationRelationId, relid, GetUserId())) + { + char relkind = get_rel_relkind(relid); + + if (relkind == '\0') + ereport(ERROR, + (errcode(ERRCODE_UNDEFINED_TABLE), + errmsg("relation with OID %u does not exist", relid))); + aclcheck_error(ACLCHECK_NOT_OWNER, get_relkind_objtype(relkind), + get_rel_name(relid)); + } + rel = table_open(relid, ShareUpdateExclusiveLock); attnum = dict_check_column(rel, colname); + /* + * Sampling and training allocate up to DICT_SAMPLE_FACTOR times the + * dictionary size (about 100 MB at a 1 MB dictionary) plus the sizes + * array and the trained dictionary. Do it in a short-lived context so a + * call across many columns or partitions in one query does not accumulate + * that in the caller's (executor) context. + */ + workcxt = AllocSetContextCreate(CurrentMemoryContext, + "pg_train_toast_dictionary", + ALLOCSET_DEFAULT_SIZES); + oldcxt = MemoryContextSwitchTo(workcxt); + initStringInfo(&samples); sizes = palloc_array(size_t, DICT_MAX_SAMPLES); slot = table_slot_create(rel, NULL); @@ -1107,12 +1222,23 @@ pg_train_toast_dictionary(PG_FUNCTION_ARGS) ereport(NOTICE, (errmsg("could not train a compression dictionary for column \"%s\" from %d values", colname, nsamples))); + MemoryContextSwitchTo(oldcxt); + MemoryContextDelete(workcxt); table_close(rel, NoLock); PG_RETURN_NULL(); } + /* + * reltuples is -1 on a table that has never been vacuumed or analyzed; + * store 0 then, so the evolution growth gate (currows * growth) is not + * negative, which would make it fire on every ANALYZE. + */ + trainrows = rel->rd_rel->reltuples < 0 ? 0 : (int64) rel->rd_rel->reltuples; dictid = dict_install(rel, attnum, InvalidOid, true, - (int64) rel->rd_rel->reltuples, dictbuf, dictlen); + trainrows, 0, dictbuf, dictlen); + + MemoryContextSwitchTo(oldcxt); + MemoryContextDelete(workcxt); table_close(rel, NoLock); PG_RETURN_OID(dictid); @@ -1377,7 +1503,7 @@ toast_dictionary_maybe_evolve(Relation onerel, AttrNumber attnum, /* (g) adopt on a win; otherwise change nothing */ if (adopt) dict_install(onerel, attnum, InvalidOid, true, - (int64) totalrows, candbuf, candlen); + (int64) totalrows, 0, candbuf, candlen); done: ReleaseCurrentSubTransaction(); @@ -1421,10 +1547,13 @@ done: /* * binary_upgrade_add_toast_dictionary(rel regclass, attnum int2, dictid oid, - * current bool, dict bytea) + * current bool, trainrows int8, + * traintime timestamptz, dict bytea) * * pg_upgrade copies the heap files as they are, and with them the dictionary * OIDs embedded in each datum, so dictionaries are restored with their OIDs. + * The evolution bookkeeping (trainrows, traintime) is carried too, so the + * growth and cooldown gates are unchanged across an upgrade. */ Datum binary_upgrade_add_toast_dictionary(PG_FUNCTION_ARGS) @@ -1433,7 +1562,9 @@ binary_upgrade_add_toast_dictionary(PG_FUNCTION_ARGS) AttrNumber attnum = PG_GETARG_INT16(1); Oid dictid = PG_GETARG_OID(2); bool current = PG_GETARG_BOOL(3); - bytea *dict = PG_GETARG_BYTEA_PP(4); + int64 trainrows = PG_GETARG_INT64(4); + TimestampTz traintime = PG_GETARG_TIMESTAMPTZ(5); + bytea *dict = PG_GETARG_BYTEA_PP(6); Relation rel; if (!IsBinaryUpgrade) @@ -1451,8 +1582,8 @@ binary_upgrade_add_toast_dictionary(PG_FUNCTION_ARGS) if (attnum <= 0 || attnum > RelationGetNumberOfAttributes(rel)) elog(ERROR, "invalid attribute number %d for relation %u", attnum, relid); - dict_install(rel, attnum, dictid, current, 0, VARDATA_ANY(dict), - VARSIZE_ANY_EXHDR(dict)); + dict_install(rel, attnum, dictid, current, trainrows, traintime, + VARDATA_ANY(dict), VARSIZE_ANY_EXHDR(dict)); table_close(rel, NoLock); PG_RETURN_VOID(); diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 97e16f3eb47..1987afd168e 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -25,6 +25,7 @@ #include "access/sysattr.h" #include "access/tableam.h" #include "access/toast_compression.h" +#include "access/toast_dictionary.h" #include "access/tupconvert.h" #include "access/xact.h" #include "access/xlog.h" @@ -2277,6 +2278,14 @@ ExecuteTruncateGuts(List *explicit_rels, */ reindex_relation(NULL, heap_relid, REINDEX_REL_PROCESS_TOAST, &reindex_params); + + /* + * TRUNCATE just discarded every row, so no value references any + * compression dictionary of this relation's columns any more. + * Retire the non-current ones, as a table rewrite would; the + * current dictionary stays for new rows. + */ + toast_dictionary_retire(heap_relid); } pgstat_count_truncate(rel); diff --git a/src/backend/utils/adt/datum.c b/src/backend/utils/adt/datum.c index 1d622e31a83..cbc5545a067 100644 --- a/src/backend/utils/adt/datum.c +++ b/src/backend/utils/adt/datum.c @@ -43,6 +43,7 @@ #include "postgres.h" #include "access/detoast.h" +#include "access/toast_compression.h" #include "common/hashfn.h" #include "fmgr.h" #include "utils/datum.h" @@ -196,6 +197,19 @@ datumTransfer(Datum value, bool typByVal, int typLen) if (!typByVal && typLen == -1 && VARATT_IS_EXTERNAL_EXPANDED_RW(DatumGetPointer(value))) value = TransferExpandedObject(value, CurrentMemoryContext); + else if (!typByVal && typLen == -1 && + VARATT_IS_COMPRESSED_DICT(DatumGetPointer(value))) + + /* + * An inline dictionary-compressed datum depends on a catalog row of + * the column it came from. Transferring it verbatim to an outer + * context (as SPI_datumTransfer() does across SPI_commit()) could + * leave it readable after a concurrent rewrite retires its + * dictionary, so decompress it here, the way a self-contained copy of + * an external value is detoasted. + */ + value = PointerGetDatum(detoast_external_attr((varlena *) + DatumGetPointer(value))); else value = datumCopy(value, typByVal, typLen); return value; diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index 5d9da797ba0..0adc4cdc6b1 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -811,7 +811,7 @@ check_hook => 'check_default_with_oids', }, -{ name => 'dictionary_evolution_max_retained', type => 'int', context => 'PGC_USERSET', group => 'CLIENT_CONN_STATEMENT', +{ name => 'dictionary_evolution_max_retained', type => 'int', context => 'PGC_SUSET', group => 'CLIENT_CONN_STATEMENT', short_desc => 'Maximum number of TOAST compression dictionaries retained per column by automatic evolution.', long_desc => 'When a column reaches this many dictionaries, automatic evolution stops retraining it until a table rewrite (VACUUM FULL / CLUSTER / REPACK) retires the old ones.', variable => 'dictionary_evolution_max_retained', @@ -820,7 +820,7 @@ max => 'INT_MAX', }, -{ name => 'dictionary_evolution_min_gain', type => 'real', context => 'PGC_USERSET', group => 'CLIENT_CONN_STATEMENT', +{ name => 'dictionary_evolution_min_gain', type => 'real', context => 'PGC_SUSET', group => 'CLIENT_CONN_STATEMENT', short_desc => 'Minimum fractional size reduction a candidate TOAST compression dictionary must achieve to be adopted.', long_desc => 'A candidate dictionary is adopted only if it compresses a held-out sample to at most (1 - this) times the size the current dictionary achieves.', variable => 'dictionary_evolution_min_gain', @@ -829,7 +829,7 @@ max => '1.0', }, -{ name => 'dictionary_evolution_min_interval', type => 'int', context => 'PGC_USERSET', group => 'CLIENT_CONN_STATEMENT', +{ name => 'dictionary_evolution_min_interval', type => 'int', context => 'PGC_SUSET', group => 'CLIENT_CONN_STATEMENT', short_desc => 'Minimum time between automatic retrainings of a column\'s TOAST compression dictionary.', flags => 'GUC_UNIT_S', variable => 'dictionary_evolution_min_interval', diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 318411d337a..bea5d68b0a1 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -5859,7 +5859,8 @@ binary_upgrade_dump_toast_dictionaries(Archive *fout, PQExpBuffer q, query = createPQExpBuffer(); appendPQExpBuffer(query, - "SELECT d.oid, d.tdattnum, d.tdcurrent, d.tddict " + "SELECT d.oid, d.tdattnum, d.tdcurrent, d.tdtrainrows, " + "d.tdtraintime, d.tddict " "FROM pg_catalog.pg_toast_dictionary d " "WHERE d.tdrelid = '%u'::pg_catalog.oid " "ORDER BY d.oid", @@ -5875,7 +5876,11 @@ binary_upgrade_dump_toast_dictionaries(Archive *fout, PQExpBuffer q, PQgetvalue(res, i, 1), PQgetvalue(res, i, 0), PQgetvalue(res, i, 2)[0] == 't' ? "true" : "false"); - appendStringLiteralAH(q, PQgetvalue(res, i, 3), fout); + appendPQExpBuffer(q, "%s::pg_catalog.int8, ", + PQgetvalue(res, i, 3)); + appendStringLiteralAH(q, PQgetvalue(res, i, 4), fout); + appendPQExpBufferStr(q, "::pg_catalog.timestamptz, "); + appendStringLiteralAH(q, PQgetvalue(res, i, 5), fout); appendPQExpBufferStr(q, "::pg_catalog.bytea);\n"); } diff --git a/src/include/access/detoast.h b/src/include/access/detoast.h index 222474bcf1e..54c086d5ef3 100644 --- a/src/include/access/detoast.h +++ b/src/include/access/detoast.h @@ -122,6 +122,15 @@ toast_external_info_get(const struct varlena *attr, toast_external_data *toast_e else toast_ext_data->compress_method = TOAST_INVALID_COMPRESSION_ID; } + + /* + * An external datum that is not compressed has no method; report it as + * the invalid id rather than leaving the raw method bits (which read as + * pglz) in place, so toast_external_data is self-describing. + */ + if (!VARATT_EXTINFO_IS_COMPRESSED(toast_ext_data->extinfo, + toast_ext_data->rawsize)) + toast_ext_data->compress_method = TOAST_INVALID_COMPRESSION_ID; } /* ---------- diff --git a/src/include/access/toast_dictionary.h b/src/include/access/toast_dictionary.h index ce368d785ec..b9219b8b89e 100644 --- a/src/include/access/toast_dictionary.h +++ b/src/include/access/toast_dictionary.h @@ -36,7 +36,7 @@ * the value did not get smaller. decompress() and decompress_slice() read a * datum this codec produced. * - * train() and compressed_size() exist only for automatic dictionary + * train() and compressed_total() exist only for automatic dictionary * evolution (see toast_dictionary_maybe_evolve()). train() builds a * dictionary of at most max_dict_bytes from nsamples concatenated samples * (sizes[] gives each one's length), writing it into dictbuf and returning diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index 62f151b3cc6..704ef3be073 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -12028,7 +12028,7 @@ { oid => '9266', descr => 'for use by pg_upgrade', proname => 'binary_upgrade_add_toast_dictionary', provolatile => 'v', proparallel => 'u', prorettype => 'void', - proargtypes => 'regclass int2 oid bool bytea', + proargtypes => 'regclass int2 oid bool int8 timestamptz bytea', prosrc => 'binary_upgrade_add_toast_dictionary' }, { oid => '4545', descr => 'for use by pg_upgrade', proname => 'binary_upgrade_set_next_heap_relfilenode', provolatile => 'v', diff --git a/src/include/utils/rel.h b/src/include/utils/rel.h index f3b322e36ac..0390edc78f6 100644 --- a/src/include/utils/rel.h +++ b/src/include/utils/rel.h @@ -158,16 +158,19 @@ typedef struct RelationData List *rd_statlist; /* list of OIDs of extended stats */ /* - * Single-entry cache of a column's current TOAST compression dictionary + * Per-column cache of each column's current TOAST compression dictionary * OID, filled lazily by toast_dictionary_current() to avoid a catalog - * index probe per compressed value. rd_toastdictattnum is the cached - * column (0 means the cache is empty); rd_toastdictoid is its current - * dictionary OID (InvalidOid if the column has none). dict_install() - * issues CacheInvalidateRelcacheByRelid(), which rebuilds the entry and so - * drops this cache, whenever a column's current dictionary changes. + * index probe per compressed value. rd_toastdictoids is a natts-long + * array in CacheMemoryContext (NULL until first use); an entry is + * InvalidOid both when a column has no dictionary and when it has not been + * looked up yet, so rd_toastdictvalid flags which entries are filled. + * A single array avoids thrashing on the write path when a table has more + * than one dictionary column. dict_install() issues + * CacheInvalidateRelcacheByRelid(), which rebuilds the entry and so drops + * this cache, whenever a column's current dictionary changes. */ - AttrNumber rd_toastdictattnum; - Oid rd_toastdictoid; + Oid *rd_toastdictoids; + Bitmapset *rd_toastdictvalid; /* data managed by RelationGetIndexAttrBitmap: */ bool rd_attrsvalid; /* are bitmaps of attrs valid? */ diff --git a/src/include/varatt.h b/src/include/varatt.h index 62ace34d83b..733ce4ec61c 100644 --- a/src/include/varatt.h +++ b/src/include/varatt.h @@ -623,6 +623,20 @@ VARDATA_COMPRESSED_GET_EXTSIZE(const void *PTR) return ((const varattrib_4b *) PTR)->va_compressed.va_tcinfo & VARLENA_EXTSIZE_MASK; } +/* + * Raw two-bit method field of a compressed-in-line varlena, without reading + * the long-form method byte. A value of VARLENA_COMPRESS_METHOD_LONG means + * the real method is in va_cmid, which is only present (and safe to read) + * when the datum is at least VARHDRSZ_COMPRESSED_LONG bytes. + */ +static inline uint32 +VARDATA_COMPRESSED_GET_RAW_METHOD(const void *PTR) +{ + const varattrib_4b *va = (const varattrib_4b *) PTR; + + return va->va_compressed.va_tcinfo >> VARLENA_EXTSIZE_BITS; +} + /* * Compression method of a compressed-in-line varlena datum. This handles * both header forms, so it always returns the actual method ID. diff --git a/src/test/regress/expected/compression_dict_evolve.out b/src/test/regress/expected/compression_dict_evolve.out index 277f87b2f7c..c3b039eaa09 100644 --- a/src/test/regress/expected/compression_dict_evolve.out +++ b/src/test/regress/expected/compression_dict_evolve.out @@ -16,20 +16,24 @@ SET search_path TO dict_evolve, public; -- equality), with a low row floor. The 5% margin keeps its default so the -- held-out size test is the real adoption decision. SET dictionary_evolution_min_interval = 0; --- Two visibly different value distributions. A dictionary trained on one --- compresses it well and the other poorly, so switching the stored --- distribution produces a candidate that wins the held-out test. +-- Two visibly different value distributions, each wide enough to be TOASTed +-- and compressed on disk (a few kB), so the test exercises real +-- dictionary compression and not just catalog-row churn. A dictionary +-- trained on one compresses it well and the other poorly, so switching the +-- stored distribution produces a candidate that wins the held-out test. CREATE FUNCTION docA(i int) RETURNS text LANGUAGE SQL IMMUTABLE AS $$ - SELECT format('{"customer_id": %s, "status": "active", "region": "us-east", ' - '"plan": "enterprise", "features": ["sso", "audit", "backup"], ' - '"env": "production", "tier": "gold", "shard": "a-%s"}', - i, i % 7) + SELECT repeat( + format('{"customer_id": %s, "status": "active", "region": "us-east", ' + '"plan": "enterprise", "features": ["sso", "audit", "backup"], ' + '"env": "production", "tier": "gold", "shard": "a-%s"}' || chr(10), + i, i % 7), 40) $$; CREATE FUNCTION docB(i int) RETURNS text LANGUAGE SQL IMMUTABLE AS $$ - SELECT format('warehousepallet' - 'cold-storage%sin-transit' - 'freightwaysstandard', - i, i % 7) + SELECT repeat( + format('warehousepallet' + 'cold-storage%sin-transit' + 'freightwaysstandard' || chr(10), + i, i % 7), 40) $$; CREATE TABLE evo (id int, f1 text COMPRESSION zstd_dict) WITH (autovacuum_enabled = off); @@ -57,6 +61,16 @@ SELECT count(*) AS ndicts, count(*) FILTER (WHERE tdcurrent) AS ncurrent SELECT oid AS dicta FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent \gset +-- Confirm the rows really are wide enough to be TOAST-compressed on disk +-- (stored smaller than raw), so the test exercises real on-disk compression +-- and not just catalog-row churn on sub-threshold values. +SELECT bool_and(pg_column_size(f1) < octet_length(f1)) AS compressed_on_disk + FROM evo; + compressed_on_disk +-------------------- + t +(1 row) + -- Same distribution again: a re-trained candidate does not beat the current -- dictionary by the margin, so nothing is adopted and the catalog is quiet. ANALYZE evo; @@ -73,11 +87,12 @@ SELECT oid = :dicta AS still_same_current FROM pg_toast_dictionary t (1 row) --- Drift: rewrite every value to distribution B (row count unchanged, so the --- growth gate still accepts). ANALYZE should find a candidate that beats the --- A-trained dictionary on the held-out B sample and adopt it: the count rises --- and the current dictionary changes. -UPDATE evo SET f1 = docB(id); +-- Drift: add a batch of distribution-B rows (the A rows stay, so values +-- compressed with the A dictionary coexist with B rows on disk). ANALYZE +-- should find a candidate that beats the A-trained dictionary on the held-out +-- B-dominated sample and adopt it: the count rises and the current dictionary +-- changes. +INSERT INTO evo SELECT g, docB(g) FROM generate_series(4001, 12000) g; ANALYZE evo; SELECT count(*) AS ndicts FROM pg_toast_dictionary WHERE tdrelid = :evo_oid; ndicts @@ -94,6 +109,20 @@ SELECT oid <> :dicta AS current_changed FROM pg_toast_dictionary SELECT oid AS dictb FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent \gset +-- Both generations read back correctly: the A rows decode with the retired +-- dictionary stamped into them, the B rows with the current one. +SELECT bool_and(f1 = docA(id)) AS a_ok FROM evo WHERE id <= 4000; + a_ok +------ + t +(1 row) + +SELECT bool_and(f1 = docB(id)) AS b_ok FROM evo WHERE id > 4000; + b_ok +------ + t +(1 row) + -- Stable again on B: ANALYZE must not churn. ANALYZE evo; SELECT oid = :dictb AS still_same_current FROM pg_toast_dictionary @@ -103,14 +132,6 @@ SELECT oid = :dictb AS still_same_current FROM pg_toast_dictionary t (1 row) --- Every value still reads back correctly (each decodes with the dictionary --- stamped into it, old or new). -SELECT bool_and(f1 = docB(id)) AS all_equal FROM evo; - all_equal ------------ - t -(1 row) - -- Cap: with a low cap, repeated drift keeps adopting only until the cap is -- reached, after which evolution declines and the catalog stops growing. The -- cap message goes to the server log; here we just check the bound holds. diff --git a/src/test/regress/sql/compression_dict_evolve.sql b/src/test/regress/sql/compression_dict_evolve.sql index ed85aa94640..3f6ce6fae70 100644 --- a/src/test/regress/sql/compression_dict_evolve.sql +++ b/src/test/regress/sql/compression_dict_evolve.sql @@ -20,20 +20,24 @@ SET search_path TO dict_evolve, public; -- held-out size test is the real adoption decision. SET dictionary_evolution_min_interval = 0; --- Two visibly different value distributions. A dictionary trained on one --- compresses it well and the other poorly, so switching the stored --- distribution produces a candidate that wins the held-out test. +-- Two visibly different value distributions, each wide enough to be TOASTed +-- and compressed on disk (a few kB), so the test exercises real +-- dictionary compression and not just catalog-row churn. A dictionary +-- trained on one compresses it well and the other poorly, so switching the +-- stored distribution produces a candidate that wins the held-out test. CREATE FUNCTION docA(i int) RETURNS text LANGUAGE SQL IMMUTABLE AS $$ - SELECT format('{"customer_id": %s, "status": "active", "region": "us-east", ' - '"plan": "enterprise", "features": ["sso", "audit", "backup"], ' - '"env": "production", "tier": "gold", "shard": "a-%s"}', - i, i % 7) + SELECT repeat( + format('{"customer_id": %s, "status": "active", "region": "us-east", ' + '"plan": "enterprise", "features": ["sso", "audit", "backup"], ' + '"env": "production", "tier": "gold", "shard": "a-%s"}' || chr(10), + i, i % 7), 40) $$; CREATE FUNCTION docB(i int) RETURNS text LANGUAGE SQL IMMUTABLE AS $$ - SELECT format('warehousepallet' - 'cold-storage%sin-transit' - 'freightwaysstandard', - i, i % 7) + SELECT repeat( + format('warehousepallet' + 'cold-storage%sin-transit' + 'freightwaysstandard' || chr(10), + i, i % 7), 40) $$; CREATE TABLE evo (id int, f1 text COMPRESSION zstd_dict) @@ -55,6 +59,12 @@ SELECT count(*) AS ndicts, count(*) FILTER (WHERE tdcurrent) AS ncurrent SELECT oid AS dicta FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent \gset +-- Confirm the rows really are wide enough to be TOAST-compressed on disk +-- (stored smaller than raw), so the test exercises real on-disk compression +-- and not just catalog-row churn on sub-threshold values. +SELECT bool_and(pg_column_size(f1) < octet_length(f1)) AS compressed_on_disk + FROM evo; + -- Same distribution again: a re-trained candidate does not beat the current -- dictionary by the margin, so nothing is adopted and the catalog is quiet. ANALYZE evo; @@ -62,11 +72,12 @@ SELECT count(*) AS ndicts FROM pg_toast_dictionary WHERE tdrelid = :evo_oid; SELECT oid = :dicta AS still_same_current FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent; --- Drift: rewrite every value to distribution B (row count unchanged, so the --- growth gate still accepts). ANALYZE should find a candidate that beats the --- A-trained dictionary on the held-out B sample and adopt it: the count rises --- and the current dictionary changes. -UPDATE evo SET f1 = docB(id); +-- Drift: add a batch of distribution-B rows (the A rows stay, so values +-- compressed with the A dictionary coexist with B rows on disk). ANALYZE +-- should find a candidate that beats the A-trained dictionary on the held-out +-- B-dominated sample and adopt it: the count rises and the current dictionary +-- changes. +INSERT INTO evo SELECT g, docB(g) FROM generate_series(4001, 12000) g; ANALYZE evo; SELECT count(*) AS ndicts FROM pg_toast_dictionary WHERE tdrelid = :evo_oid; SELECT oid <> :dicta AS current_changed FROM pg_toast_dictionary @@ -74,15 +85,16 @@ SELECT oid <> :dicta AS current_changed FROM pg_toast_dictionary SELECT oid AS dictb FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent \gset +-- Both generations read back correctly: the A rows decode with the retired +-- dictionary stamped into them, the B rows with the current one. +SELECT bool_and(f1 = docA(id)) AS a_ok FROM evo WHERE id <= 4000; +SELECT bool_and(f1 = docB(id)) AS b_ok FROM evo WHERE id > 4000; + -- Stable again on B: ANALYZE must not churn. ANALYZE evo; SELECT oid = :dictb AS still_same_current FROM pg_toast_dictionary WHERE tdrelid = :evo_oid AND tdcurrent; --- Every value still reads back correctly (each decodes with the dictionary --- stamped into it, old or new). -SELECT bool_and(f1 = docB(id)) AS all_equal FROM evo; - -- Cap: with a low cap, repeated drift keeps adopting only until the cap is -- reached, after which evolution declines and the catalog stops growing. The -- cap message goes to the server log; here we just check the bound holds. -- 2.50.1