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
signature.asc
Description: PGP signature
