| From: | Andrey Borodin <x4mmm(at)yandex-team(dot)ru> |
|---|---|
| To: | Japin Li <japinli(at)hotmail(dot)com> |
| Cc: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com>, wenhui qiu <qiuwenhuifx(at)gmail(dot)com>, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
| Subject: | Re: Compression of bigger WAL records |
| Date: | 2026-08-14 17:30:03 |
| Message-ID: | 04119E77-9C0F-415A-A9E0-65E6E4A57940@yandex-team.ru |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
> On 14 Aug 2026, at 08:32, Japin Li <japinli(at)hotmail(dot)com> wrote:
>
> The patches cannot be applied to the current tree because they conflict with
> commits 931c9701f25, 18992dc9d98, and b614de4876b.
>
> Below are some initial review comments.
Hi Japin,
Thanks for the review, and for catching that the patches had gone stale.
Rebased, v9 is attached. Three of those conflicts were worth more than
a mechanical fixup.
931c9701f25 changed what wal_compression = on means, so the sample file
and the docs now describe "the first of zstd, lz4, pglz that is
available" and my paragraph about whole-record compression sits under
that rather than replacing it. I also removed the sentence saying that
"on" is a historical spelling of pglz, which that commit made untrue.
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.
18992dc9d98 added a check that a record's length does not exceed
XLogRecordMaxSize before reconstructing it. 0002 splits that length in
two, the bytes a record occupies in WAL and the bytes it decodes to, so
the check needed a side. It now guards the physical length, which is
what the reassembly buffer actually holds. I also tightened the
decompressed-length sanity check from MaxAllocSize to XLogRecordMaxSize,
since that is the limit XLogRecordAssemble() enforces on the writing
side, and it seemed wrong to accept on read what cannot be written. A
plain textual rebase left that one compiling against a variable that no
longer exists, so thank you for the nudge to look.
b614de4876b took the test number 055, so the test in the series is now
056_wal_compression.pl.
> The casting of XLogRecPtr is unnecessary.
Fixed, all three.
> I'd prefer use the palloc0_array() macro.
Done, for both stream_dctx and stream_ready.
The Windows CI run on the first rebase also turned up a bug of my own,
now fixed here: XLOGShmemAttach() re-establishes WALInsertLocks for a
process that attaches to shared memory rather than inheriting it, and
0003 added a second such pointer without doing the same for it. Under
EXEC_BACKEND the startup process therefore found a null slot array and
died on the first record it tried to insert. The slot pointer now lives
in XLogCtl->Insert beside WALInsertLocks and is restored on attach.
I reproduced that on Linux by building with -DEXEC_BACKEND rather than
guessing from the Windows backtrace. My Windows machine went offline
while I'm on vacation overseas... But I think everything should work
now, even on Windows.
Best regards, Andrey Borodin.
| Attachment | Content-Type | Size |
|---|---|---|
| v9-0001-Reuse-zstd-contexts-for-WAL-compression-and-decom.patch | application/octet-stream | 5.1 KB |
| v9-0002-Add-whole-record-WAL-compression-alongside-FPI-co.patch | application/octet-stream | 50.8 KB |
| v9-0003-WIP-compress-WAL-records-against-earlier-records-.patch | application/octet-stream | 62.4 KB |
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Robert Haas | 2026-08-14 17:34:45 | Re: CREATE OR REPLACE MATERIALIZED VIEW |
| Previous Message | Robert Treat | 2026-08-14 17:24:46 | Re: Doc update proposal for the note on log_statement in the runtime config for logging page |