Re: libpq compression

Hi all!

I took a look at proposed patches and found several typos/mistakes. Where
should I send my comments? In this thread or directly to the authors?

Thanks!

On Wed, Apr 21, 2021 at 9:03 AM Daniil Zakhlystov <usernamedt(at)yandex-team(dot)ru>
wrote:

> Hi, thanks for your review!
>
> > On Mar 19, 2021, at 11:28 AM, Justin Pryzby <pryzby(at)telsasoft(dot)com>
> wrote:
> >
> > This needs some significant effort:
> >
> > - squish commits;
> > * maybe make an 0001 commit supporting zlib, and an 0002 commit adding
> zstd;
> > * add an 0003 patch to enable zlib compression by default (for CI -
> not for merge);
> > - rebase to current master;
>
> I’ve rebased the chunked compression branch and attached it to this
> message as two diff patches:
> 0001-Add-zlib-and-zstd-streaming-compression.patch - this patch introduces
> general functionality for zlib and zstd streaming compression
> 0002-Implement-libpq-compression.patch - this patch introduces libpq
> chunked compression
>
> > - compile without warnings;
> > - assign OIDs at the end of the ranges given by find-unused-oids to avoid
> > conflicts with patches being merged to master.
>
> Done
>
> > Currently, make check-world gets stuck in several places when I use zlib.
> >
> > There's commit messages claiming that the compression can be "asymmetric"
> > between client-server vs server-client, but the commit seems to be
> unrelated,
> > and the protocol documentation doesn't describe how this works.
> >
> > Previously, I suggested that the server should have a "policy" GUC
> defining
> > which compression methods are allowed. Possibly including compression
> "level".
> > For example, the default might be to allow zstd, but only up to level 9.
>
> Support for different compression methods in each direction is implemented
> in zpq_stream.c,
> the only thing left to do is to define the GUC settings to control this
> behavior and make adjustments to the compression handshake process.
>
> Earlier in the thread, I’ve discussed the introduction of
> compress_algorithms and decompress_algorithms GUC settings with Robert Haas.
> The main issue with the decompress_algorithms setting is that the
> receiving side can’t effectively enforce the actual compression level
> chosen by the sending side.
>
> So I propose to add the two options to the client and server GUC:
> 1. compress_algorithms setting with the ability to specify the exact
> compression level for each algorithm
> 2. decompress_algorithms to control which algorithms are supported for
> decompression of the incoming messages
>
> For example:
>
> Server
> compress_algorithms = ’zstd:2; zlib:5’ // use the zstd with compression
> level 2 or zlib with compression level 5 for outgoing messages
> decompress_algorithms = ‘zstd; zlib’ // allow the zstd and zlib algorithms
> for incoming messages
>
> Client
> compress_algorithms = ’zstd; zlib:3’ // use the zstd with default
> compression level (1) or zlib with compression level 3 for outgoing
> messages
> decompress_algorithms = ‘zstd’ // allow the zstd algorithm for incoming
> messages
>
> Robert, If I missed something from our previous discussion, please let me
> know. If this approach is OK, I'll implement it.
>
> > This:
> > + /* Initialise values and NULL flags arrays */
> > + MemSet(values, 0, sizeof(values));
> > + MemSet(nulls, 0, sizeof(nulls));
> >
> > can be just:
> > + bool nulls[PG_STAT_NETWORK_TRAFFIC_COLS] = {false};
> > since values is fully populated below.
> >
>
> The current implementation matches the other functions in pgstatfuncs.c so
> I think that it is better not to change it.
>
> > typo: aslogirhm
>
> Fixed
>
> > I wrote about Solution.pm earlier in this thread, and I see that it's in
> > Konstantin's repo since Jan 12, but it's not in yours (?) so I think
> windows
> > build will fail.
> >
> > Likewise, commit 1a946e14e in his branch adds compression into to psql
> > \connninfo based on my suggestion, but it's missing in your branch.
>
> I've rebased the patch. Now it includes Konstantin's fixes.
>
> —
> Daniil Zakhlystov
>
>

--
Best Regards,

Ian Zagorskikh
Senior C Developer/Alternatives Team

CloudLinux.com | KernelCare.com | Imunify360 | AlmaLinux