Re: libpq compression (part 2)

Hi!

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> +/* GUC variable containing the allowed compression algorithms list (separated by comma) */
> +char *libpq_compress_algorithms;
>
> => The global variable is conventionally initialized to the same default as
> guc.c (even though the GUC default value will be applied at startup).

I’ve updated libpq_compress_algorithms to match the corresponding guc.c setting value.

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> + * - NegotiateProtocolVersion in cases when server does not support protocol compression
> + * Anything else probably means it's not Postgres on the other end at all.
> */
> - if (!(beresp == 'R' || beresp == 'E'))
> + if (!(beresp == 'R' || beresp == 'E' || beresp == 'z' || beresp == 'v'))
>
> => I think NegotiateProtocolVersion and 'v' are from an old version of the
> patch and no longer used ?

No, it is still relevant in the current patch version. I’ve added more comments regarding this case
and also made more transparent compression negotiation behavior.

If the client requested the compression, but the server does not support the libpq compression feature
because of the old Postgres version, the client will report an error and exit. If the server rejected
the client's compression request (for example, if libpq_compression is set to ‘off’ or no matching
algorithms found), the client will also report an error and exit.

> On 14 Jan 2022, at 04:58, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> Your 0003 patch has a couple "noise" hunks that get rid of ^M characters added
> in previous patches. The ^M shouldn't be added in the first place. Did you
> apply my fixes using git-am or something else ?

I’ve fixed the line endings and applied the pgindent. Now each patch should be OK.

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> => Since March, errmsg doesn't need extra parenthesis around it (e3a87b4).

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> + * Report current transaction start timestamp as the specified value.
> + * Zero means there is no active transaction.
>
> => The comment doesn't correspond to the function

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> + return id >= 0 && id < (sizeof(zs_algorithms) / sizeof(*zs_algorithms));
> + size_t n_algorithms = sizeof(zs_algorithms) / sizeof(*zs_algorithms);
>
> => You can use lengthof() for these

Thanks, I’ve updated the patch and corrected the highlighted issues.

> On 1 Jan 2022, at 22:25, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> I think the psql conninfo Compressor/Decompressor line may be confusing.
> Maybe it should talk about Client->Server and Server->Client.
> Maybe it should be displayed on a single line.
>
> Actually, right now the \conninfo part is hardly useful, since the compressor
> is allocated lazily/"on demand", so it shows the compression of some previous
> command, but maybe not the last command, and maybe not the next command…

I’ve updated the \conninfo part, now it shows the list of the negotiated compression algorithms.

Also, I’ve added the check for the compression option to the frontend side.

> On 14 Jan 2022, at 04:58, Justin Pryzby <pryzby(at)telsasoft(dot)com> wrote:
>
> macos: passed
> linux: timed out after 1hr
> freebsd: failed in pg_rewind: ... <= replay_lsn AND state = 'streaming' FROM ...
> windows: "Failed test 'data_checksums=on is reported on an offline cluster stdout /(?^:^on$)/'" / WARNING: 01000: algorithm zlib is not supported
>
> Note that it's possible and easy to kick off a CI run using any github account:
> see ./src/tools/ci/README
>
> For me, it's faster than running check-world -j4 locally, and runs tests on 4
> OSes.

I’ve resolved the stuck tests and added zlib support for CI Windows builds to patch 0003-*. Thanks
for the suggestion, all tests seem to be OK now, except the macOS one. It won't schedule in Cirrus
CI for some reason, but I guess it happens because of my GitHub account limitation.

--
Thanks,

Daniil Zakhlystov