Thread: zstd compression for pg_dump

I found that our largest tables are 40% smaller and 20% faster to pipe
pg_dump -Fc -Z0 |zstd relative to native zlib

So I wondered how much better when integrated in pg_dump, and found that
there's some additional improvement, but a big disadvantage of piping through
zstd is that it's not identified as a PGDMP file, and, /usr/bin/file on centos7
fails to even identify zstd by its magic number..

I looked for previous discussion about alternate compressions, but didn't find
anything for pg_dump.

-- 
Justin

Justin Pryzby <pryzby@telsasoft.com> writes:
> I found that our largest tables are 40% smaller and 20% faster to pipe
> pg_dump -Fc -Z0 |zstd relative to native zlib

The patch might be a tad smaller if you hadn't included a core file in it.

            regards, tom lane

On Mon, Dec 21, 2020 at 03:02:40PM -0500, Tom Lane wrote:
> Justin Pryzby <pryzby@telsasoft.com> writes:
> > I found that our largest tables are 40% smaller and 20% faster to pipe
> > pg_dump -Fc -Z0 |zstd relative to native zlib
> 
> The patch might be a tad smaller if you hadn't included a core file in it.

About 89% smaller.

This also fixes the extension (.zst)
And fixes zlib default compression.
And a bunch of cleanup.

-- 
Justin

On Mon, Dec 21, 2020 at 01:49:24PM -0600, Justin Pryzby wrote:
> a big disadvantage of piping through zstd is that it's not identified as a
> PGDMP file, and, /usr/bin/file on centos7 fails to even identify zstd by its
> magic number..

Other reasons are that pg_dump |zstd >output.zst loses the exit status of
pg_dump, and that it's not "transparent" (one needs to type
"zstd -dq |pg_restore").

On Mon, Dec 21, 2020 at 08:32:35PM -0600, Justin Pryzby wrote:
> On Mon, Dec 21, 2020 at 03:02:40PM -0500, Tom Lane wrote:
> > Justin Pryzby <pryzby@telsasoft.com> writes:
> > > I found that our largest tables are 40% smaller and 20% faster to pipe
> > > pg_dump -Fc -Z0 |zstd relative to native zlib
> > 
> > The patch might be a tad smaller if you hadn't included a core file in it.
> 
> About 89% smaller.
> 
> This also fixes the extension (.zst)
> And fixes zlib default compression.
> And a bunch of cleanup.

I rebased so the "typedef struct compression" patch is first and zstd on top of
that (say, in case someone wants to bikeshed about which compression algorithm
to support).  And made a central struct with all the compression-specific info
to further isolate the compress-specific changes.

And handle compression of "plain" archive format.
And fix compilation for MSVC and make --without-zstd the default.

And fix cfgets() (which I think is actually unused code for the code paths for
compressed FP).

And add fix for pre-existing problem: ftello() on unseekable input.

I also started a patch to allow compression of "tar" format, but I didn't
include that here yet.

Note, there's currently several "compression" patches in CF app.  This patch
seems to be independent of the others, but probably shouldn't be totally
uncoordinated (like adding lz4 in one and ztsd in another might be poor
execution).

https://commitfest.postgresql.org/31/2897/
 - Faster pglz compression
https://commitfest.postgresql.org/31/2813/
 - custom compression methods for toast
https://commitfest.postgresql.org/31/2773/
 - libpq compression

-- 
Justin

> 4 янв. 2021 г., в 07:53, Justin Pryzby <pryzby@telsasoft.com> написал(а):
>
> Note, there's currently several "compression" patches in CF app.  This patch
> seems to be independent of the others, but probably shouldn't be totally
> uncoordinated (like adding lz4 in one and ztsd in another might be poor
> execution).
>
> https://commitfest.postgresql.org/31/2897/
> - Faster pglz compression
> https://commitfest.postgresql.org/31/2813/
> - custom compression methods for toast
> https://commitfest.postgresql.org/31/2773/
> - libpq compression

I think that's downside of our development system: patch authors do not want to create dependencies on other patches.
I'd say that both lz4 and zstd should be supported in TOAST, FPIs, libpq, and pg_dump. As to pglz - I think we should
notproliferate it any further. 
Lz4 and Zstd represent a different tradeoff actually. Basically, lz4 is so CPU-cheap that one should use it whenever
theywrite to disk or network interface. Zstd represent an actual bandwith\CPU tradeoff. 
Also, all patchsets do not touch important possibility - preexisting dictionary could radically improve compression of
smalldata (event in pglz). 

Some minor notes on patchset at this thread.

Libpq compression encountered some problems with memory consumption which required some extra config efforts. Did you
measurememory usage for this patchset? 

[PATCH 03/20] Support multiple compression algs/levels/opts..
abtracts -> abstracts
enum CompressionAlgorithm actually represent the very same thing as in "Custom compression methods"

Daniil, is levels definition compatible with libpq compression patch?
+typedef struct Compress {
+    CompressionAlgorithm    alg;
+    int            level;
+    /* Is a nondefault level set ?  This is useful since different compression
+     * methods have different "default" levels.  For now we assume the levels
+     * are all integer, though.
+    */
+    bool        level_set;
+} Compress;

[PATCH 04/20] struct compressLibs
I think this directive would be correct.
+// #ifdef HAVE_LIBZ?

Here's extra comment
// && errno == ENOENT)


[PATCH 06/20] pg_dump: zstd compression

I'd propose to build with Zstd by default. It seems other patches do it this way. Though, I there are possible
downsides.


Thanks for working on this! We will have very IO-efficient Postgres :)

Best regards, Andrey Borodin.

On Mon, Jan 04, 2021 at 11:04:57AM +0500, Andrey Borodin wrote:
> > 4 янв. 2021 г., в 07:53, Justin Pryzby <pryzby@telsasoft.com> написал(а):
> > Note, there's currently several "compression" patches in CF app.  This patch
> > seems to be independent of the others, but probably shouldn't be totally
> > uncoordinated (like adding lz4 in one and ztsd in another might be poor
> > execution).
> > 
> > https://commitfest.postgresql.org/31/2897/
> > - Faster pglz compression
> > https://commitfest.postgresql.org/31/2813/
> > - custom compression methods for toast
> > https://commitfest.postgresql.org/31/2773/
> > - libpq compression
> 
> I think that's downside of our development system: patch authors do not want to create dependencies on other
patches.

I think in these cases, someone who notices common/overlapping patches should
suggest that the authors review each other's work.  In some cases, I think it's
appropriate to come up with a "shared" preliminary patch(es), which both (all)
patch authors can include as 0001 until its finalized and merged.  That might
be true for some things like the tableam work, or the two "online checksum"
patches.

> I'd say that both lz4 and zstd should be supported in TOAST, FPIs, libpq, and pg_dump. As to pglz - I think we should
notproliferate it any further.
 

pg_basebackup came up as another use on another thread, I think related to
libpq protocol compression.

> Libpq compression encountered some problems with memory consumption which
> required some extra config efforts. Did you measure memory usage for this
> patchset?

RAM use is not significantly different from zlib, except that zstd --long adds
more memory.

$ command time -v pg_dump -d ts -t ... -Fc -Z0 |wc -c
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:28.77
        Maximum resident set size (kbytes): 40504
    1397288924 # no compression: 1400MB

$ command time -v pg_dump -d ts -t ... -Fc |wc -c
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:37.17
        Maximum resident set size (kbytes): 40504
    132932415 # default (zlib) compression: 132 MB

$ command time -v ./pg_dump -d ts -t ... -Fc |wc -c
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:29.28
        Maximum resident set size (kbytes): 40568
    86048139 # zstd: 86MB

$ command time -v ./pg_dump -d ts -t ... -Fc -Z 'alg=zstd opt=zstdlong' |wc -c
        Elapsed (wall clock) time (h:mm:ss or m:ss): 0:30.49
        Maximum resident set size (kbytes): 180332
    72202937 # zstd long: 180MB

> [PATCH 04/20] struct compressLibs
> I think this directive would be correct.
> +// #ifdef HAVE_LIBZ?

I'm not sure .. I'm thinking of making the COMPR_ALG_* always defined, and then
fail later if an operation is unsupported.  There's an excessive number of
#ifdefs already, so the early commits are intended to minimize as far as
possible what's needed for each additional compression
algorithm(lib/method/whatever it's called).  I haven't tested much with
pg_restore of files with unsupported compression libs.

> [PATCH 06/20] pg_dump: zstd compression
> I'd propose to build with Zstd by default. It seems other patches do it this way. Though, I there are possible
downsides.

Yes...but the cfbot turns red if the patch require zstd, so it defaults to
off until it's included in the build environments (but for now, the main patch
isn't being tested).

Thanks for looking.

-- 
Justin

Hi!

> On Jan 4, 2021, at 11:04 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> Daniil, is levels definition compatible with libpq compression patch?
> +typedef struct Compress {
> +    CompressionAlgorithm    alg;
> +    int            level;
> +    /* Is a nondefault level set ?  This is useful since different compression
> +     * methods have different "default" levels.  For now we assume the levels
> +     * are all integer, though.
> +    */
> +    bool        level_set;
> +} Compress;

Similarly to this patch, it is also possible to define the compression level at the initialization stage in libpq
compressionpatch. 

The difference is that in libpq compression patch the default compression level always equal to 1, independently of the
chosencompression algorithm. 

> On Jan 4, 2021, at 11:04 AM, Andrey Borodin <x4mmm@yandex-team.ru> wrote:
>
> Libpq compression encountered some problems with memory consumption which required some extra config efforts.


> On Jan 4, 2021, at 12:06 PM, Justin Pryzby <pryzby@telsasoft.com> wrote:
>
> RAM use is not significantly different from zlib, except that zstd --long adds
> more memory.

Regarding ZSTD memory usage:

Recently I’ve made a couple of tests of libpq compression with different ZLIB/ZSTD compression levels which shown that
compressing/decompressingZSTD w/ high compression levels  
require to allocate more virtual (Commited_AS) memory, which may be exploited by malicious clients:

https://www.postgresql.org/message-id/62527092-16BD-479F-B503-FA527AF3B0C2%40yandex-team.ru

We can avoid high memory usage by limiting the max window size to 8MB. This should effectively disable the support of
compressionlevels above 19: 
https://www.postgresql.org/message-id/6A45DFAA-1682-4EF2-B835-C5F46615EC49%40yandex-team.ru

So maybe it is worthwhile to use similar restrictions in this patch.

—
Daniil Zakhlystov

On 1/4/21 3:53 AM, Justin Pryzby wrote:
>> About 89% smaller.

Did a quick code review of the patch. I have not yet taken it for a spin 
yet and there are parts of the code I have not read yet.

## Is there any reason for this diff?

-    cfp        *fp = pg_malloc(sizeof(cfp));
+    cfp        *fp = pg_malloc0(sizeof(cfp));

## Since we know have multiple returns in cfopen() I am not sure that 
setting fp to NULL is still clearer than just returning NULL.

## I do not like that this pretends to support r+, w+ and a+ but does 
not actually do so since it does not create an input stream in those cases.

else if (mode[0] == 'w' || mode[0] == 'a' ||
    strchr(mode, '+') != NULL)
[...]
else if (strchr(mode, 'r'))

## Wouldn't cfread(), cfwrite(), cfgetc(), cfgets(), cfclose() and 
cfeof() be cleaner with sitch statments similar to cfopen()?

## "/* Should be called "method" or "library" ? */"

Maybe, but personally I think algorithm is fine too.

## "Is a nondefault level set ?"

The PostgreSQL project does not use space before question mark (at least 
not in English).

## Why isn't level_set just a local variable in parse_compression()? It 
does not seem to be used elsewhere.

## Shouldn't we call the Compression variable in OpenArchive() 
nocompress to match with the naming convention in other places.

And in general I wonder if we should not write "nocompression = 
{COMPR_ALG_NONE}" rather than "nocompression = {0}".

## Why not use const on the pointers to Compression for functions like 
cfopen()? As far as I can see several of them could be const.

## Shouldn't "AH->compression.alg = Z_DEFAULT_COMPRESSION" in ReadHead() 
be "AH->compression.alg = COMPR_ALG_DEFAULT"?

Additionally I am not convinced that returning COMPR_ALG_DEFAULT will 
even work but I have not had the time to test that theory yet. And in 
general I am quite sceptical of that we really need of COMPR_ALG_DEFAULT.

## Some white space issues

Add spaces around plus in "atoi(1+eq)" and "pg_log_error("unknown 
compression algorithm: %s", 1+eq)".

Add spaces around plus in parse_compression(), e.g. in "strlen(1+eq)".

## Shouldn't hasSuffix() take the current compression algorithm as a 
parameter? Or alternatively look up which compression algorithm to use 
from the suffix?

## Why support multiple ways to write zlib on the command line? I do not 
see any advatange of being able to write it as libz.

## I feel renaming SaveOutput() to GetOutput() would make it more clear 
what it does now that you have changed the return type.

## You have accidentally committed "-runstatedir" in configure. I have 
no idea why we do not have it (maybe it is something Debian specific) 
but even if we are going to add it it should not be in this patch. Same 
with the parenthesis changes to LARGE_OFF_T.

## This is probably out of scope of your patch but I am not a fan of the 
fallback logic in cfopen_read(). I feel ideally we should always know if 
there is a suffix or not and not try to guess file names and do 
pointless syscalls.

## COMPR_ALG_DEFAULT looks like it would error out for archive and 
directory if someone has neither zlib nor zstandard. It feels like it 
should default to uncompressed if we have neither. Or at least give a 
better error message.

> Note, there's currently several "compression" patches in CF app.  This patch
> seems to be independent of the others, but probably shouldn't be totally
> uncoordinated (like adding lz4 in one and ztsd in another might be poor
> execution).

A thought here is that maybe we want to use the same values for the 
enums in all patches. Especially if we write the numeric value to pg 
dump files.

Andreas

On 1/3/21 9:53 PM, Justin Pryzby wrote:

> I rebased so the "typedef struct compression" patch is first and zstd on top of
> that (say, in case someone wants to bikeshed about which compression algorithm
> to support).  And made a central struct with all the compression-specific info
> to further isolate the compress-specific changes.

It has been a few months since there was a new patch and the current one 
no longer applies, so marking Returned with Feedback.

Please resubmit to the next CF when you have a new patch.

Regards,
-- 
-David
david@pgmasters.net