Re: refactoring basebackup.c - Mailing list pgsql-hackers
From | Robert Haas |
---|---|
Subject | Re: refactoring basebackup.c |
Date | |
Msg-id | CA+TgmoZvqk7UuzxsX1xjJRmMGkqoUGYTZLDCH8SmU1xTPr1Xig@mail.gmail.com Whole thread Raw |
In response to | Re: refactoring basebackup.c (Jeevan Ladhe <jeevan.ladhe@enterprisedb.com>) |
Responses |
Re: refactoring basebackup.c
Re: refactoring basebackup.c |
List | pgsql-hackers |
On Tue, Nov 2, 2021 at 7:53 AM Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> wrote: > I have implemented the cleanup callback bbsink_lz4_cleanup() in the attached patch. > > Please have a look and let me know of any comments. Looks pretty good. I think you should work on stuff like documentation and tests, and I need to do some work on that stuff, too. Also, I think you should try to figure out how to support different compression levels. For gzip, I did that by making gzip1..gzip9 possible compression settings. But that might not have been the right idea because something like lz43 to mean lz4 at level 3 would be confusing. Also, for the lz4 command line utility, there's not only "lz4 -3" which means LZ4 with level 3 compression, but also "lz4 --fast=3" which selects "ultra-fast compression level 3" rather than regular old level 3. And apparently LZ4 levels go up to 12 rather than just 9 like gzip. I'm thinking maybe we should go with something like "gzip@9" rather than just "gzip9" to mean gzip with compression level 9, and then things like "lz4@3" or "lz4@fast3" would select either the regular compression levels or the ultra-fast compression levels. Meanwhile, I think it's probably OK for me to go ahead and commit 0001-0003 from my patches at this point, since it seems we have pretty good evidence that the abstraction basically works, and there doesn't seem to be any value in holding off and maybe having to do a bunch more rebasing. We may also want to look into making -Fp work with --server-compression, which would require pg_basebackup to know how to decompress. I'm actually not sure if this is worthwhile; you'd need to have a network connection slow enough that it's worth spending a lot of CPU time compressing on the server and decompressing on the client to make up for the cost of network transfer. But some people might have that case. It might make it easier to test this, too, since we probably can't rely on having an LZ4 binary installed. Another thing that you probably need to investigate is also supporting client-side LZ4 compression. I think that is probably a really desirable addition to your patch set, since people might find it odd if that were exclusively a server-side option. Hopefully it's not that much work. One minor nitpick in terms of the code: + mysink->bytes_written = mysink->bytes_written + headerSize; I would use += here. -- Robert Haas EDB: http://www.enterprisedb.com
pgsql-hackers by date: