Subject: Re: wal_compression=zstd

From: Michael Paquier <[email-masked]>
Date: Fri, 4 Mar 2022 16:19:32 +0900

On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote:
> I am not claiming that zstd is generally better for WAL.  Rather, if we're
> going to support alternate compression methods, it's nice to give a couple
> options (in addition to pglz).  Some use cases would certainly suffer from
> slower WAL.  But providing the option will benefit some others.  Note that a
> superuser can set wal_compression for a given session - this would probably
> benefit bulk-loading in an otherwise OLTP environment.

Well, I cannot say which one is better either as it depends on your
workload, mostly, but we know for sure that both have benefits, so I
don't mind adding it now.  What you are proposing is built on top of
the existing code, making it a very simple change.

> As writen, this patch uses zstd level=1 (whereas the ZSTD's default compress
> level is 6).

Why?  ZSTD using this default has its reasons, no?  And it would be
consistent to do the same for ZSTD as for the other two methods.

-        The supported methods are <literal>pglz</literal> and
-        <literal>lz4</literal> (if <productname>PostgreSQL</productname> was
-        compiled with <option>--with-lz4</option>). The default value is
-        <literal>off</literal>. Only superusers can change this setting.
+        The supported methods are <literal>pglz</literal>, and
+        (if configured when <productname>PostgreSQL</productname>was built)
+        <literal>lz4</literal> and zstd.
+        The default value is <literal>off</literal>.
+        Only superusers can change this setting.

This is making the docs less verbose.  I think that you should keep
the mention to respectively --with-lz4 and --with-zstd after each
value.

       <para>
        Build with <productname>ZSTD</productname> compression support.
+       This enables use of <productname>ZSTD</productname> for
+       compression of WAL data.

This addition is not necessary, see d7a9786.

Not related to your patch, but we have more reasons to add an
check in the block of BKPIMAGE_COMPRESSED() in RestoreBlockImage() of
xlogreader.c to make sure that only one bit is set for the compression
type.  We could use a pg_popcount() == 1 for that, returning
report_invalid_record() when we see some corrupted data.

As a whole, 0001 looks pretty good to me after a detailed read (not
tested, though).

> Only 0001 should be reviewed for pg15 - the others are optional/future work.

That's wiser for v15.  FWIW, I have the impression that we don't need
most of what's proposed in 0002~.

 /* compression methods supported */
-#define BKPIMAGE_COMPRESS_PGLZ 0x04
-#define BKPIMAGE_COMPRESS_LZ4  0x08
-#define BKPIMAGE_COMPRESS_ZSTD 0x10
-
+#define BKPIMAGE_COMPRESS_METHOD1      0x04    /* bits to encode  compression method */
+#define BKPIMAGE_COMPRESS_METHOD2      0x08    /* 0=none, 1=pglz, 2=lz4, 3=zstd */

As of 0002.  We still have some room for this data and this makes the
code harder to follow, so I'd live this part of the code as it is
proposed in 0001.

0003, defaulting to zstd, and 0004 to extend compression to support a
level would require a lot of benchmarking to be justified.  I have
argued against making the code more complicated for such things in the
past, with reloptions.  The footprint on the code is much smaller,
here, still..

0007, for ZLIB, does not make sense once one can choose between LZ4
and ZSTD.
--
Michael

---

From: Justin Pryzby <[email-masked]>
Date: Fri, 4 Mar 2022 05:44:06 -0600

> consistent to do the same for ZSTD as for the other two methods.

In my 1-off test, it gets 610/633 = 96% of the benefit at 209/273 = 77% of the
cost.

postgres=# SET wal_compression= 'zstd-6';
postgres=# \set QUIET \\ \timing on \\ SET max_parallel_maintenance_workers=0; SELECT pg_stat_reset_shared('wal'); begin; CREATE INDEX ON t(a); rollback; SELECT * FROM pg_stat_wal;
Duración: 2729,017 ms (00:02,729)
wal_bytes        | 6102403

postgres=# SET wal_compression= 'zstd-1';
postgres=# \set QUIET \\ \timing on \\ SET max_parallel_maintenance_workers=0; SELECT pg_stat_reset_shared('wal'); begin; CREATE INDEX ON t(a); rollback; SELECT * FROM pg_stat_wal;
Duración: 2090,459 ms (00:02,090)
wal_bytes        | 6330269

It may be relevant that we're only compressing 8k [0].
It would probably pay off to preserve a compression "dictionary" to be re-used
across FPIs.

> the mention to respectively --with-lz4 and --with-zstd after each

I changed this relative to your latest zstd patch in July since it reads better
to me.  YMMV.

On Tue, Feb 22, 2022 at 05:19:48PM -0600, Justin Pryzby wrote:
>> 0001 adds support for zstd
>> 0002 makes more efficient our use of bits in the WAL header
>> 0003 makes it the default compression, to exercise on CI (windows will fail).
>> 0004 adds support for zstd-level
>> 0005-6 are needed to allow recovery tests to pass when wal compression is enabled;
>> 0007 (not included) also adds support for zlib.  I'm of the impression nobody
>>      cares about this, otherwise it would've been included 5-10 years ago.

> 0003, defaulting to zstd, would require a lot of benchmarking to be
> justified.  

Maybe you didn't see that I wrote that it was for CI ?
(In any case, it's impossible to do that without first taking care of 005-6).

> and 0004 to extend compression to support a level 
> I have argued against making the code more complicated for such things in the
> past, with reloptions.

I suppose that you're referring to reloptions for toast compression, which was
about controlling LZ4 compression level.

https://www.postgresql.org/message-id/flat/[email-masked]

I think you're right that it's not interesting to control the compression level
of LZ4 - but there's no reason to say the same thing of zstd.  We're already
debating which is the "right" level to use (1 or 6).  I don't think there is a
"right" level - some people will want to trade more CPU for disk writes and
some people won't.