Re: Compression of bigger WAL records

On Fri, Aug 14, 2026 at 08:30:03PM +0300, Andrey Borodin wrote:
> Rebased, v9 is attached. Three of those conflicts were worth more than
> a mechanical fixup.

I don't really have a ground-breaking opinion about v9-0002 and
v9-0003 (sorry!), but I do think that v9-0001 has a lot of value in
terms of reusing the [de]compression context. Particularly for WAL
replay where we have WAL mostly made of FPIs, this could be a huge
deal in terms of performance. Your numbers regarding wal_log_hints
are also appealing. So let's get this part done.

Now, the patch needs a few adjustments from what I can see. Not that
many adjustments, and they should be easy enough to fix.

> That commit is also why 0001 matters more than it did. With "on" now
> reaching for zstd first, every installation that turns compression on
> without naming an algorithm gets the codec that allocates a context per
> full-page image, in both directions.

v9-0001 should be split into two parts: one for the WAL insert path,
and a second one for the xlogreader bits and WAL replay. Both are
independently useful on their own. The replay part is much more
interesting to me, though, with the startup process replaying
everything, so I'd suggest to focus on this part first. The code
paths touched are independent, so the order does not really matter,
fine. :D

+#ifdef USE_ZSTD
+/*
+ * Compression context reused across all block images compressed by this
+ * backend. zstd keeps its match tables and window in here, roughly 1.3MB at
+ * the default level, and allocates them on first use. Creating a context per
+ * call would repeat that allocation for every full-page image.
+ *
+ * It is deliberately not freed when wal_compression changes: a backend that
+ * compressed once is likely to do it again, and the context is only reachable
+ * from here.
+ */
+static ZSTD_CCtx *zstd_cctx = NULL;
+#endif

I suspect that we could cut most of the comment here without losing in
readability.

+ zstd_cctx = ZSTD_createCCtx();
[...]
+ record->fpi_dctx = ZSTD_createDCtx();

Postgres has a bad history of losing track of memory that's been
allocated in a context different than a palloc(), especially when it
comes from external libraries, and I don't think we should do in each
backend an allocation that's under the control of zstd, or we should
at least have some safeguards to clean up that correctly. We have a
bunch of facilities in place to make that safer by design, like
resowners or memory context callback. Based on how long-lived these
context are going to be, the xloginsert.c bit will be living with the
process, so we could just leave ZSTD_createDCtx as is. However,
XLogReaderState.fpi_dctx is not OK. You should be safe with having a
reset memory context callback, linked to the memory context where the
XLogReaderState has been allocated. This has to be backend-only.

+ /*
+ * Kept across calls so that decompressing a full page image does not
+ * create and destroy a decompression context every time. Void because
+ * this header is included where zstd.h is not.
+ */
+ void *fpi_dctx;

I think that this is an incorrect design, as it could encourage
reusing fpi_dctx across more compression methods than just zstd.
Let's just hide that within a USE_ZSTD block with a proper pointer
type. Note that wal_compression is user-settable, so one could mix
different compression methods on a record-basis. Perhaps your idea of
using a void* is better at the end, but please at least mark it with a
"zstd" naming and hide it behind a fpi_dctx.

- if (ZSTD_isError(len))
- len = -1; /* failure */
+ if (zstd_cctx == NULL)
+ zstd_cctx = ZSTD_createCCtx();
+
+ if (zstd_cctx == NULL)
+ len = -1; /* out of memory; store the i

This is incorrect. Compression failures are fail-safe, but an
allocation failure (OOM) on the context should be a *hard* failure.
The replay part makes the good choice, so I'm puzzled as of why you
have chosen a safe failure mode here.

I can see some APIs available in zstd for more dynamic allocations,
like ZSTD_createDCtx_advanced() or some "static" versions that can be
given a pre-allocated area, but these are guarded by
ZSTDLIB_STATIC_API, aka we cannot use that with a dynamically-linked
zstd library. Sad.
--
Michael