Hi, > On Oct 29, 2020, at 12:27 AM, Andres Freund <and...@anarazel.de> wrote: > > The protocol sounds to me like there's no way to enable/disable > compression in an existing connection. To me it seems better to have an > explicit, client initiated, request to use a specific method of > compression (including none). That allows to enable compression for bulk > work, and disable it in other cases (e.g. for security sensitive > content, or for unlikely to compress well content). > > I think that would also make cross-version handling easier, because a > newer client driver can send the compression request and handle the > error, without needing to reconnect or such. > > Most importantly, I think such a design is basically a necessity to make > connection poolers to work in a sensible way.
Can you please clarify your opinion about connection poolers? Do you mean by sensible way that in some cases they can just forward the compressed stream without parsing? >> +/* >> + * Index of used compression algorithm in zpq_algorithms array. >> + */ >> +static int zpq_algorithm_impl; > > This is just odd API design imo. Why doesn't the dispatch work based on > an argument for zpq_create() and the ZpqStream * for the rest? > > What if there's two libpq connections in one process? To servers > supporting different compression algorithms? This isn't going to fly. I agree, I think that moving the zpq_algorithm_impl to the ZpqStream struct seems like an easy fix for this issue. >> + /* initialize compression */ >> + if (zpq_set_algorithm(compression_algorithm)) >> + PqStream = zpq_create((zpq_tx_func)secure_write, >> (zpq_rx_func)secure_read, MyProcPort); >> + } >> + return 0; >> +} > > Why is zpq a wrapper around secure_write/read? I'm a bit worried this > will reduce the other places we could use zpq. Maybe we can just split the PqStream into PqCompressStream and PqDecompressStream? It looks like that they can work independently. — Daniil Zakhlystov
