Different compression methods for FPI

On Sat, Feb 27, 2021 at 12:43:52PM +0500, Andrey Borodin wrote:

So I think it worth to propose a patch to make wal_compression_method = {"pglz", "lz4", "zlib"}. Probably, "zstd" can be added to the list.
Attached is a draft taking CompressionId from "custom compression methods" patch and adding zlib to it.

Thanks for submitting it.

Does this need to patch ./configure{,.ac} and Solution.pm for HAVE_LIBLZ4 ?
I suggest to also include an 0002 patch (not for commit) which changes to use a
non-default compression, to exercise this on the CIs - linux and bsd
environments now have liblz4 installed, and for windows you can keep "undef".

Daniil had a patch to add src/common/z_stream.c:
https://github.com/postgrespro/libpq_compression/blob/0a9c70d582cd4b1ef60ff39f8d535f6e800bd7d4/src/common/z_stream.c
/messages/by-id/470E411E-681D-46A2-A1E9-6DE11B5F59F3@yandex-team.ru

Your patch looks fine, but I wonder if we should first implement a generic
compression API. Then, the callers don't need to have a bunch of #ifdef.
If someone calls zs_create() for an algorithm which isn't enabled at compile
time, it throws the error at a lower level.

That also allows a central place to do things like handle options (compression
level, and things like zstd --long, --rsyncable, etc).

In some cases there's an argument that the compression ID should be globally
defined constant, not like a dynamic "plugable" OID. That may be true for the
libpq compression, WAL compression, and pg_dump, since there's separate
"producer" and "consumer". I think if there's "pluggable" compression (like
the TOAST patch), then it may have to map between the static or dynamic OIDs
and the global compression ID.

--
Justin

On Sun, Feb 28, 2021 at 05:08:17PM -0600, Justin Pryzby wrote:

Does this need to patch ./configure{,.ac} and Solution.pm for HAVE_LIBLZ4 ?
I suggest to also include an 0002 patch (not for commit) which changes to use a
non-default compression, to exercise this on the CIs - linux and bsd
environments now have liblz4 installed, and for windows you can keep "undef".

Good idea. But are you sure that lz4 is available in the CF bot
environments?

Your patch looks fine, but I wonder if we should first implement a generic
compression API. Then, the callers don't need to have a bunch of #ifdef.
If someone calls zs_create() for an algorithm which isn't enabled at compile
time, it throws the error at a lower level.

Yeah, the patch feels incomplete with its footprint in xloginsert.c
for something that could be available in src/common/ like pglz,
particularly knowing that you will need to have this information
available to frontend tools, no?

In some cases there's an argument that the compression ID should be globally
defined constant, not like a dynamic "plugable" OID. That may be true for the
libpq compression, WAL compression, and pg_dump, since there's separate
"producer" and "consumer". I think if there's "pluggable" compression (like
the TOAST patch), then it may have to map between the static or dynamic OIDs
and the global compression ID.

This declaration should be frontend-aware. As presented, the patch
would enforce zlib or lz4 on top of pglz, but I guess that it would be
more user-friendly to complain when attempting to set up
wal_compression_method (why not just using wal_compression?) to an
unsupported option rather than doing it after-the-fact for the first
full page generated once the new parameter value is loaded.

This stuff could just add tests in src/test/recovery/. See for
example src/test/ssl and with_ssl to see how conditional tests happen
depending on the configure options.

Not much a fan either of assuming that it is just fine to add one byte
to XLogRecordBlockImageHeader for the compression_method field.
--
Michael

On Mon, Mar 01, 2021 at 01:57:12PM +0900, Michael Paquier wrote:

On Sun, Feb 28, 2021 at 05:08:17PM -0600, Justin Pryzby wrote:

Does this need to patch ./configure{,.ac} and Solution.pm for HAVE_LIBLZ4 ?
I suggest to also include an 0002 patch (not for commit) which changes to use a
non-default compression, to exercise this on the CIs - linux and bsd
environments now have liblz4 installed, and for windows you can keep "undef".

Good idea. But are you sure that lz4 is available in the CF bot
environments?

Yes, see here
/messages/by-id/20210119205643.GA1941@telsasoft.com

--
Justin

1 марта 2021 г., в 10:03, Justin Pryzby <pryzby@telsasoft.com> написал(а):

Justin, Michael, thanks for comments!

As far as I understood TODO list for the patch looks as follows:

1. Reuse compression API of other patches. But which one? Or should I invent new one? "compressamapi.h" from "custom compression methods" bothers only with varlena <-> varlena conversions, and only for lz4. And it is "access method" after all, residing in backend...
ZStream looks good, but it lacks OID identification for compression methods and lz4.
2. Store OID in FPIs instead of 1-byte CompressionId. Make sure frontend is able to recognize builtin compression OIDs.
3. Complain if wal_compression_method is set to lib which is not compiled in.
4. Add tests for different WAL compression methods similar to src/test/ssl

Did I miss something?
I would appreciate a help with item 1, I do not know how to choose starting point.

wal_compression_method (why not just using wal_compression?)

I hope one day we will compress all WAL, not just FPIs. Advanced archive management tools already do so, why not compress it in walwriter?
When this will be implemented, we could have wal_compression = {off, fpi, all}.

Best regards, Andrey Borodin.

On Sat, Mar 06, 2021 at 12:29:14PM +0500, Andrey Borodin wrote:

1 марта 2021 г., в 10:03, Justin Pryzby <pryzby@telsasoft.com> написал(а):

Justin, Michael, thanks for comments!

As far as I understood TODO list for the patch looks as follows:

Your patch can be simplified some, and then the only ifdef are in two short
functions. Moving the compression calls to another function/file is hardly
worth it, and anyone that implements a generic compression API could refactor
easily, if it's a win. So I don't want to impose the burden on your small
patch of setting up the compression API for everyone else's patches. Since
this is non-streaming compression, the calls are trivial.

One advantage of a generic API is that it's a good place to handle things like
compression options, like zstd:9 or zstd:3,rsyncable (I am not suggesting this
syntax).

Today, I re-sent an Dillip's patch with a change to use pkg-config for liblz4,
and it now also compiles on mac, so I used those changes to configure.ac (using
pkg-config) and src/tools/msvc/Solution.pm, and changed HAVE_LIBLZ4 to USE_LZ4.

This also resolves conflict with 32fd2b57d7f64948e649fc205c43f007762ecaac.

wal_compression_method is PGC_SIGHUP, not SUSET.

I think that the COMPRESSION_ID should have a prefix like XLOG_* - but didn't
do it here.

Does this patch need to bump XLOG_PAGE_MAGIC ?

wal_compression_options: I made this conditional compilation, so the GUC
machinery rejects methods which aren't supported. That means that xloginsert
doesn't need to check for unsupported methods. sync_method_options also uses
conditional compilation, but a GUC "check" hook would be more friendly, since
it could distinguish between "not supported" and "valid":
|ERROR: invalid value for parameter "wal_compression_method": "lz4"

--
Justin

On Fri, Mar 12, 2021 at 01:45:47AM -0600, Justin Pryzby wrote:

On Sat, Mar 06, 2021 at 12:29:14PM +0500, Andrey Borodin wrote:

1 марта 2021 г., в 10:03, Justin Pryzby <pryzby@telsasoft.com> написал(а):

Justin, Michael, thanks for comments!

As far as I understood TODO list for the patch looks as follows:

Your patch can be simplified some, and then the only ifdef are in two short
functions. Moving the compression calls to another function/file is hardly
worth it, and anyone that implements a generic compression API could refactor
easily, if it's a win. So I don't want to impose the burden on your small
patch of setting up the compression API for everyone else's patches. Since
this is non-streaming compression, the calls are trivial.

One advantage of a generic API is that it's a good place to handle things like
compression options, like zstd:9 or zstd:3,rsyncable (I am not suggesting this
syntax).

Today, I re-sent an Dillip's patch with a change to use pkg-config for liblz4,
and it now also compiles on mac, so I used those changes to configure.ac (using
pkg-config) and src/tools/msvc/Solution.pm, and changed HAVE_LIBLZ4 to USE_LZ4.

Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd. These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

On Mon, Mar 01, 2021 at 01:57:12PM +0900, Michael Paquier wrote:

Your patch looks fine, but I wonder if we should first implement a generic
compression API. Then, the callers don't need to have a bunch of #ifdef.
If someone calls zs_create() for an algorithm which isn't enabled at compile
time, it throws the error at a lower level.

Yeah, the patch feels incomplete with its footprint in xloginsert.c
for something that could be available in src/common/ like pglz,
particularly knowing that you will need to have this information
available to frontend tools, no?

Michael: what frontend tools did you mean ?
pg_rewind? This may actually be okay as-is, since it uses symlinks:

$ ls -l src/bin/pg_rewind/xlogreader.c
lrwxrwxrwx 1 pryzbyj pryzbyj 48 Mar 12 17:48 src/bin/pg_rewind/xlogreader.c -> ../../../src/backend/access/transam/xlogreader.c

Not much a fan either of assuming that it is just fine to add one byte
to XLogRecordBlockImageHeader for the compression_method field.

What do you mean? Are you concerned about alignment, or the extra width, or??

These two patches are a prerequisite for this patch to progress:
* Run 011_crash_recovery.pl with wal_level=minimal
* Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.

--
Justin

13 марта 2021 г., в 06:28, Justin Pryzby <pryzby@telsasoft.com> написал(а):

Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd. These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

Thanks! Awesome work!

These two patches are a prerequisite for this patch to progress:
* Run 011_crash_recovery.pl with wal_level=minimal
* Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.

I want to note, that fixes for 011_crash_recovery.pl are not strictly necessary for this patch set.
The problem in tests arises if we turn on wal_compression, absolutely independently from wal compression method.
We turn on wal_compression in this test only for CI purposes.

Thanks!

Best regards, Andrey Borodin.

On Sat, Mar 13, 2021 at 08:48:33PM +0500, Andrey Borodin wrote:

13 марта 2021 г., в 06:28, Justin Pryzby <pryzby@telsasoft.com> написал(а):
Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd. These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

Thanks! Awesome work!

These two patches are a prerequisite for this patch to progress:
* Run 011_crash_recovery.pl with wal_level=minimal
* Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.

I want to note, that fixes for 011_crash_recovery.pl are not strictly necessary for this patch set.
The problem in tests arises if we turn on wal_compression, absolutely independently from wal compression method.
We turn on wal_compression in this test only for CI purposes.

I rearranged the patches to reflect this.
Change to zlib and zstd to level=1.
Add support for negative "zstd fast" levels.
Use correct length accounting for "hole" in LZ4 and ZSTD.
Fixed Solution.pm for zstd on windows.
Switch to zstd by default (for CI).
Add docs.

It occurred to me that the generic "compression API" might be of more
significance if we supported compression of all WAL (not just FPI). I imagine
that might use streaming compression.

--
Justin

@cfbot: Resending without duplicate patches

On Sun, Mar 14, 2021 at 07:31:35PM -0500, Justin Pryzby wrote:

On Sat, Mar 13, 2021 at 08:48:33PM +0500, Andrey Borodin wrote:

13 марта 2021 г., в 06:28, Justin Pryzby <pryzby@telsasoft.com> написал(а):
Updated patch with a minor fix to configure.ac to avoid warnings on OSX.
And 2ndary patches from another thread to allow passing recovery tests.
Renamed to WAL_COMPRESSION_*
Split LZ4 support to a separate patch and support zstd. These show that
changes needed for a new compression method have been minimized, although not
yet moved to a separate, abstracted compression/decompression function.

Thanks! Awesome work!

These two patches are a prerequisite for this patch to progress:
* Run 011_crash_recovery.pl with wal_level=minimal
* Make sure published XIDs are persistent

I don't know if anyone will consider this patch for v14 - if not, it should be
set to v15 and revisit in a month.

I want to note, that fixes for 011_crash_recovery.pl are not strictly necessary for this patch set.
The problem in tests arises if we turn on wal_compression, absolutely independently from wal compression method.
We turn on wal_compression in this test only for CI purposes.

I rearranged the patches to reflect this.
Change to zlib and zstd to level=1.
Add support for negative "zstd fast" levels.
Use correct length accounting for "hole" in LZ4 and ZSTD.
Fixed Solution.pm for zstd on windows.
Switch to zstd by default (for CI).
Add docs.

Changes:
- Allocate buffer sufficient to accommodate any supported compression method;
- Use existing info flags argument rather than adding another byte for storing
the compression method; this seems to be what was anticipated by commit
57aa5b2bb and what Michael objected to.

I think the existing structures are ugly, so maybe this suggests using a GUC
assign hook to support arbitrary compression level, and maybe other options.

--
Justin

rebased to keep cfbot happy. This will run with default=zlib.

On Sun, Mar 21, 2021 at 02:30:04PM -0500, Justin Pryzby wrote:

rebased to keep cfbot happy. This will run with default=zlib.

I have been looking at bit at this patch set, and I think that we
should do something here for 15~.

First, I think that we should make more tests and pick up one
compression method that could be used as an alternative to pglz, not
three among zlib, LZ4 or zstd. Looking at the past archives, we could
do more tests like this one:
/messages/by-id/CAB7nPqSc97o-UE5paxfMUKWcxE_JioyxO1M4A0pMnmYqAnec2g@mail.gmail.com

The I/O should not be the bottleneck here, so on top of disabling
fsync we could put the whole data folder on a ramdisk and compare the
whole. The patch set does not apply, by the way, so it needs a
rebase.

Based on my past experiences, I'd see LZ4 as a good choice in terms of
performance, speed and compression ratio, and on top of that we now
have the possibility to build PG with --with-lz4 for TOAST
compression so ./configure is already tweaked for it. For this patch,
this is going to require a bit more in terms of library linking as the
block decompression is done in xlogreader.c, so that's one thing to
worry about.

 #define BKPIMAGE_IS_COMPRESSED     0x02    /* page image is  compressed */
 #define BKPIMAGE_APPLY     0x04    /* page image should be restored during
                                      * replay */				      
+#define BKPIMAGE_COMPRESS_METHOD1  0x08    /* bits to encode
compression method */
+#define BKPIMAGE_COMPRESS_METHOD2  0x10    /* 0=pglz; 1=zlib; */

The interface used in xlogrecord.h is confusing to me with
BKPIMAGE_IS_COMPRESSED, followed by extra bits set for the compression
method. Wouldn't it be cleaner to have a set of BKPIMAGE_COMPRESS_XXX
(XXX={lz4,zlib,etc.})? There is no even need to steal one bit for
some kind of BKPIMAGE_COMPRESS_NONE as we know that the page is
compressed if we know it has a method assigned, so with pglz, the
default, and one extra method we need only two bits here.

+   {
+       {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+           gettext_noop("Set the method used to compress full page
images in the WAL."),
+           NULL
+       },
+       &wal_compression_method,
+       WAL_COMPRESSION_PGLZ, wal_compression_options,
+       NULL, NULL, NULL
+   },
The interface is not quite right to me.  I think that we should just
change wal_compression to become an enum, with extra values for pglz
and the new method.  "on" would be a synonym for "pglz".

+/* This is a mapping indexed by wal_compression */
+// XXX: maybe this is better done as a GUC hook to assign the 1)
method; and 2) level
+struct walcompression walmethods[] = {
+   {"pglz",    WAL_COMPRESSION_PGLZ},
+   {"zlib",    WAL_COMPRESSION_ZLIB},
+};
Don't think you need a hook here, but zlib, or any other method which
is not supported by the build, had better not be listed instead.  This
ensures that the value cannot be assigned if the binaries don't
support that.

+       {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+           gettext_noop("Set the method used to compress full page
images in the WAL."),
+           NULL
+       },
+       &wal_compression_method,
+       WAL_COMPRESSION_PGLZ, wal_compression_options,
+       NULL, NULL, NULL
Any reason to not make that user-settable?  If you merge that with
wal_compression, that's not an issue.

The patch set is a gathering of various things, and not only things
associated to the compression method used for FPIs.

my $node = get_new_node('primary');
-$node->init(allows_streaming => 1);
+$node->init();
$node->start;
What is the point of that in patch 0002?

Subject: [PATCH 03/12] Make sure published XIDs are persistent

Patch 0003 looks unrelated to this thread.

Subject: [PATCH 04/12] wal_compression_method: default to zlib..

Patch 0004 could happen, however there are no reasons given why this
is adapted. Changing the default is not going to happen for the time
release where this feature is added, anyway.

+       default:
+           report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)",
+                                 (uint32) (record->ReadRecPtr >> 32),
+                                 (uint32) record->ReadRecPtr,
+                                 block_id,
+                                 compression_method,
+
wal_compression_name(compression_method));
+           return false;
In xlogreader.c, the error message is helpful this way.  However, we
would not know which compression method failed if there is a
decompression failure for a method supported by the build restoring
this block.  That would be good to add.

I think that what we actually need for this thread are patches 0001,
0005 and 0006 merged together to study first the performance we have
with each one of the compression methods proposed, and then let's just
pick one. Reading around, zstd and zlib compresse more but take
longer. LZ4 is faster than the others, but can compress less.
With limited bandwidth, less data makes sense, and my guess is that
most users care most about the speed of recovery if we can afford
speed with an acceptable compression ratio.
--
Michael

On Mon, May 17, 2021 at 04:44:11PM +0900, Michael Paquier wrote:

On Sun, Mar 21, 2021 at 02:30:04PM -0500, Justin Pryzby wrote:

For this patch, this is going to require a bit more in terms of library
linking as the block decompression is done in xlogreader.c, so that's one
thing to worry about.

I'm not sure what you mean here ?

+   {
+       {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+           gettext_noop("Set the method used to compress full page images in the WAL."),
+           NULL
+       },
+       &wal_compression_method,
+       WAL_COMPRESSION_PGLZ, wal_compression_options,
+       NULL, NULL, NULL
+   },
The interface is not quite right to me.  I think that we should just
change wal_compression to become an enum, with extra values for pglz
and the new method.  "on" would be a synonym for "pglz".

Andrey gave a reason in March:

| I hope one day we will compress all WAL, not just FPIs. Advanced archive management tools already do so, why not compress it in walwriter?
| When this will be implemented, we could have wal_compression = {off, fpi, all}.

+/* This is a mapping indexed by wal_compression */
+// XXX: maybe this is better done as a GUC hook to assign the 1)
method; and 2) level
+struct walcompression walmethods[] = {
+   {"pglz",    WAL_COMPRESSION_PGLZ},
+   {"zlib",    WAL_COMPRESSION_ZLIB},
+};
Don't think you need a hook here, but zlib, or any other method which
is not supported by the build, had better not be listed instead.  This
ensures that the value cannot be assigned if the binaries don't
support that.

I think you're confusing the walmethods struct (which is unconditional) with
wal_compression_options, which is conditional.

The patch set is a gathering of various things, and not only things
associated to the compression method used for FPIs.
What is the point of that in patch 0002?

Subject: [PATCH 03/12] Make sure published XIDs are persistent

Patch 0003 looks unrelated to this thread.

..for the reason that I gave:

| And 2ndary patches from another thread to allow passing recovery tests.
|These two patches are a prerequisite for this patch to progress:
| * Run 011_crash_recovery.pl with wal_level=minimal
| * Make sure published XIDs are persistent

Subject: [PATCH 04/12] wal_compression_method: default to zlib..

Patch 0004 could happen, however there are no reasons given why this
is adapted. Changing the default is not going to happen for the time
release where this feature is added, anyway.

From the commit message:
| this is meant to exercise the CIs, and not meant to be merged

+       default:
+           report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)",
+                                 (uint32) (record->ReadRecPtr >> 32),
+                                 (uint32) record->ReadRecPtr,
+                                 block_id,
+                                 compression_method,
+                                 wal_compression_name(compression_method));
+           return false;
In xlogreader.c, the error message is helpful this way.  However, we
would not know which compression method failed if there is a
decompression failure for a method supported by the build restoring
this block.  That would be good to add.

I don't undersand you here - that's what wal_compression_name is for ?
2021-05-18 21:38:04.324 CDT [26984] FATAL: unknown compression method requested: 2(lz4)

I think that what we actually need for this thread are patches 0001,
0005 and 0006 merged together to study first the performance we have
with each one of the compression methods proposed, and then let's just
pick one. Reading around, zstd and zlib compresse more but take
longer. LZ4 is faster than the others, but can compress less.
With limited bandwidth, less data makes sense, and my guess is that
most users care most about the speed of recovery if we can afford
speed with an acceptable compression ratio.

I don't see why we'd add a guc for configuration compression but not include
the 30 lines of code needed to support a 3rd method that we already used by the
core server.

--
Justin

On Tue, May 18, 2021 at 10:06:18PM -0500, Justin Pryzby wrote:

On Mon, May 17, 2021 at 04:44:11PM +0900, Michael Paquier wrote:

On Sun, Mar 21, 2021 at 02:30:04PM -0500, Justin Pryzby wrote:

For this patch, this is going to require a bit more in terms of library
linking as the block decompression is done in xlogreader.c, so that's one
thing to worry about.

I'm not sure what you mean here ?

I was wondering if anything else was needed in terms of linking here.

Don't think you need a hook here, but zlib, or any other method which
is not supported by the build, had better not be listed instead. This
ensures that the value cannot be assigned if the binaries don't
support that.

I think you're confusing the walmethods struct (which is unconditional) with
wal_compression_options, which is conditional.

Indeed I was. For the note, walmethods is not necessary either as a
new structure. You could just store a list of strings in xlogreader.c
and make a note to keep the entries in a given order. That's simpler
as well.

The patch set is a gathering of various things, and not only things
associated to the compression method used for FPIs.
What is the point of that in patch 0002?

Subject: [PATCH 03/12] Make sure published XIDs are persistent

Patch 0003 looks unrelated to this thread.

..for the reason that I gave:

| And 2ndary patches from another thread to allow passing recovery tests.
|These two patches are a prerequisite for this patch to progress:
| * Run 011_crash_recovery.pl with wal_level=minimal
| * Make sure published XIDs are persistent

I still don't understand why XID consistency has anything to do with
the compression of FPIs. There is nothing preventing the testing of
compression of FPIs, and plese note this argument:
/messages/by-id/BEF3B1E0-0B31-4F05-8E0A-F681CB918626@yandex-team.ru

For example, I can just revert from my tree 0002 and 0003, and still
perform tests of the various compression methods. I do agree that we
are going to need to do something about this problem, but let's drop
this stuff from the set of patches of this thread and just discuss
them where they are needed.

+   {
+       {"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+           gettext_noop("Set the method used to compress full page images in the WAL."),
+           NULL
+       },
+       &wal_compression_method,
+       WAL_COMPRESSION_PGLZ, wal_compression_options,
+       NULL, NULL, NULL
+   },
The interface is not quite right to me.  I think that we should just
change wal_compression to become an enum, with extra values for pglz
and the new method.  "on" would be a synonym for "pglz".

Andrey gave a reason in March:

| I hope one day we will compress all WAL, not just FPIs. Advanced archive management tools already do so, why not compress it in walwriter?
| When this will be implemented, we could have wal_compression = {off, fpi, all}.

[...]

I don't see why we'd add a guc for configuration compression but not include
the 30 lines of code needed to support a 3rd method that we already used by the
core server.

Because that makes things confusing. We have no idea if we'll ever
reach a point or even if it makes sense to have compression applied to
multiple parts of WAL. So, for now, let's just use one single GUC and
be done with it. Your argument is not tied to what's proposed on this
thread either, and could go the other way around. If we were to
invent more compression concepts in WAL records, we could as well just
go with a new GUC that lists the parts of the WAL where compression
needs to be applied. I'd say to keep it to a minimum for now, that's
an interface less confusing than what's proposed here.

And you have not replaced BKPIMAGE_IS_COMPRESSED by a PGLZ-equivalent,
so your patch set is eating more bits for BKPIMAGE_* than it needs
to.

By the way, it would be really useful for the user to print in
pg_waldump -b the type of compression used :)

A last point, and I think that this should be part of a study of the
choice to made for an extra compression method: there is no discussion
yet about the level of compression applied, which is something that
can be applied to zstd, lz4 or even zlib. Perhaps there is an
argument for a different GUC controlling that, so more benchmarking
is a first step needed for this thread to move on. Benchmarking can
happen with what's currently posted, of course.
--
Michael

On Wed, May 19, 2021 at 06:31:15PM +0900, Michael Paquier wrote:

I still don't understand why XID consistency has anything to do with
the compression of FPIs. There is nothing preventing the testing of
compression of FPIs, and plese note this argument:
/messages/by-id/BEF3B1E0-0B31-4F05-8E0A-F681CB918626@yandex-team.ru

For example, I can just revert from my tree 0002 and 0003, and still
perform tests of the various compression methods. I do agree that we
are going to need to do something about this problem, but let's drop
this stuff from the set of patches of this thread and just discuss
them where they are needed.

They are needed here - that they're included is deliberate. Revert this and
then the tests fail. "Make sure published XIDs are persistent"

time make -C src/test/recovery check
# Failed test 'new xid after restart is greater'

And you have not replaced BKPIMAGE_IS_COMPRESSED by a PGLZ-equivalent,
so your patch set is eating more bits for BKPIMAGE_* than it needs

The goal is to support 2+ "methods" (including "none"), which takes 4 bits, so
may as well support 3 methods.

- uncompressed
- pglz
- lz4
- zlib or zstd or ??

This version:
0) repurposes the pre-existing GUC as an enum;
1) saves a bit (until zstd is included);
2) shows the compression in pg_waldump;

To support different compression levels, I think I'd change from an enum to
string and an assign hook, which sets a pair of ints.

--
Justin

On Tue, May 25, 2021 at 10:14 AM Justin Pryzby <pryzby@telsasoft.com> wrote:

Some comment.

+#define BKPIMAGE_COMPRESS_METHOD1 0x04 /* bits to encode compression method */
+#define BKPIMAGE_COMPRESS_METHOD2 0x08 /* 0=none, 1=pglz, 2=zlib */

Instead of using METHOD1, METHOD2, can we use the direct method name,
that will look cleaner?

+ unsigned long len_l = COMPRESS_BUFSIZE;
+ int ret;
+ ret = compress2((Bytef*)dest, &len_l, (Bytef*)source, orig_len, 1);

compress2((Bytef*)dest -> compress2((Bytef *) dest

diff --git a/src/test/recovery/t/011_crash_recovery.pl
b/src/test/recovery/t/011_crash_recovery.pl
index a26e99500b..2e7e3db639 100644
--- a/src/test/recovery/t/011_crash_recovery.pl
+++ b/src/test/recovery/t/011_crash_recovery.pl
@@ -14,7 +14,7 @@ use Config;
 plan tests => 3;

 my $node = get_new_node('primary');
-$node->init(allows_streaming => 1);
+$node->init();
 $node->start;

How this change is relevant?

+#ifdef USE_LZ4
+ case WAL_COMPRESSION_LZ4:
+ len = LZ4_compress_fast(source, dest, orig_len, COMPRESS_BUFSIZE, 1);
+ if (len == 0)
+ len = -1;
+ break;
+#endif

If we are passing acceleration as 1, then we can directly use
LZ4_compress_default, it is the same as LZ4_compress_fast with
acceleration=1.

--
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com

On Mon, May 24, 2021 at 11:44:45PM -0500, Justin Pryzby wrote:

The goal is to support 2+ "methods" (including "none"), which takes 4 bits, so
may as well support 3 methods.

- uncompressed
- pglz
- lz4
- zlib or zstd or ??

Let's make a proper study of all that and make a choice, the only
thing I am rather sure of is that pglz is bad compared to all the
others. There is no point to argue as long as we don't know if any of
those candidates are suited for the job.

This version:
0) repurposes the pre-existing GUC as an enum;
1) saves a bit (until zstd is included);
2) shows the compression in pg_waldump;

To support different compression levels, I think I'd change from an enum to
string and an assign hook, which sets a pair of ints.

Hmm. I am not really sure what you mean here, but let's keep that
in mind until we get more performance numbers.
--
Michael

25 мая 2021 г., в 12:26, Michael Paquier <michael@paquier.xyz> написал(а):

On Mon, May 24, 2021 at 11:44:45PM -0500, Justin Pryzby wrote:

The goal is to support 2+ "methods" (including "none"), which takes 4 bits, so
may as well support 3 methods.

- uncompressed
- pglz
- lz4
- zlib or zstd or ??

Let's make a proper study of all that and make a choice, the only
thing I am rather sure of is that pglz is bad compared to all the
others. There is no point to argue as long as we don't know if any of
those candidates are suited for the job.

There's a lot of open studies like [0,1].
In short, Lz4 is fastest codec. Zstd gives better compression at cost of more CPU usage[2]Zstd gives significantly better compression at cost of little more CPU usage. But they both have points on Pareto frontier.. Zlib is not tied to Facebook, however, it's slower and have smaller compression ratio than Zstd. Zstd is actively developed.

There is Google's Brotli codec. It is comparable to Zstd.

What kind of deeper study do we want to do?
Would it make sense to run our own benchmarks?
Or, perhaps, review other codecs?

In my opinion, anything that is sent over network or written to block storage deserves Zstd-5 compression. But milage may vary.

Thanks!

Best regards, Andrey Borodin.

[0]: https://indico.cern.ch/event/695984/contributions/2872933/attachments/1590457/2516802/ZSTD_and_ZLIB_Updates_-_January_20186.pdf
[1]: https://facebook.github.io/zstd/
[2]: Zstd gives significantly better compression at cost of little more CPU usage. But they both have points on Pareto frontier.

On Mon, May 31, 2021 at 12:33:44PM +0500, Andrey Borodin wrote:

Would it make sense to run our own benchmarks?

Yes, I think that it could be a good idea to run some custom-made
benchmarks as that could mean different bottlenecks found when it
comes to PG.

There are a couple of factors that matter here:
- Is the algo available across a maximum of platforms? ZLIB and LZ4
are everywhere and popular, for one. And we already plug with them in
the builds. No idea about the others but I can see quickly that Zstd
has support across many systems, and has a compatible license.
- Speed and CPU usage. We should worry about that for CPU-bounded
environments.
- Compression ratio, which is just monitoring the difference in WAL.
- Effect of the level of compression perhaps?
- Use a fixed amount of WAL generated, meaning a set of repeatable SQL
queries, with one backend, no benchmarks like pgbench.
- Avoid any I/O bottleneck, so run tests on a tmpfs or ramfs.
- Avoid any extra WAL interference, like checkpoints, no autovacuum
running in parallel.

It is not easy to draw a straight line here, but one could easily say
that an algo that reduces a FPI by 90% costing two times more CPU
cycles is worse than something doing only a 70%~75% compression for
two times less CPU cycles if environments are easily constrained on
CPU.

As mentioned upthread, I'd recomment to design tests like this one, or
just reuse this one:
/messages/by-id/CAB7nPqSc97o-UE5paxfMUKWcxE_JioyxO1M4A0pMnmYqAnec2g@mail.gmail.com

In terms of CPU usage, we should also monitor the user and system
times of the backend, and compare the various situations. See patch
0003 posted here that we used for wal_compression:
https://www.postgresql.org/message-idCAB7nPqRC20=mKgu6d2st-e11_QqqbreZg-=SF+_UYsmvwNu42g@mail.gmail.com

This just uses getrusage() to get more stats.
--
Michael