Date: Mon, 17 Jan 2022 22:39:20 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Daniil Zakhlystov <usernamedt@yandex-team.ru>
Cc: pgsql-hackers@lists.postgresql.org,
	Andrey Borodin <x4mmm@yandex-team.ru>
Subject: Re: libpq compression (part 2)
Message-ID: <20220118043919.GA23027@telsasoft.com>
References: <ABAA09C6-BB95-47A5-890D-90353533F9AC@yandex-team.ru>
 <20220112141526.GF14051@telsasoft.com>
 <D29C2E5B-30FA-4870-9CC8-D49DACDED7AF@yandex-team.ru>
 <20220113235827.GM14051@telsasoft.com>
 <18CFC392-0391-4439-9F02-FC1CF02FFA24@yandex-team.ru>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <18CFC392-0391-4439-9F02-FC1CF02FFA24@yandex-team.ru>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220118043919.GA23027%40telsasoft.com>
Precedence: bulk

On Tue, Jan 18, 2022 at 02:06:32AM +0500, Daniil Zakhlystov wrote:
> > => Since March, errmsg doesn't need extra parenthesis around it (e3a87b4).

> 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.

I don't know about your github account, but it works for cfbot, which is now
green.

Thanks for implementing zlib for windows.  Did you try this with default
compressions set to lz4 and zstd ?

The thread from 2019 is very long, and starts off with the guidance that
compression had been implemented at the wrong layer.  It looks like this hasn't
changed since then.  secure_read/write are passed as function pointers to the
ZPQ interface, which then calls back to them to read and flush its compression
buffers.  As I understand, the suggestion was to leave the socket reads and
writes alone.  And then conditionally de/compress buffers after reading /
before writing from the socket if compression was negotiated.

It's currently done like this
pq_recvbuf() => secure_read() - when compression is disabled 
pq_recvbuf() => ZPQ => secure_read() - when compression is enabled 

Dmitri sent a partial, POC patch which changes the de/compression to happen in
secure_read/write, which is changed to call ZPQ:  
https://www.postgresql.org/message-id/CA+q6zcUPrssNaRS+FyoBsD-F2stK1Roa-4sAhFOfAjOWLziM4g@mail.gmail.com
pq_recvbuf() => secure_read() => ZPQ

The same thing is true of the frontend: function pointers to
pqsecure_read/write are being passed to zpq_create, and then the ZPQ interface
called instead of the original functions.  Those are the functions which read
using SSL, so they should also handle compression.

That's where SSL is handled, and it seems like the right place to handle
compression.  Have you evaluated that way to do things ?

Konstantin said he put ZPQ at that layer seems to 1) avoid code duplication
between client/server; and, 2) to allow compression to happen before SSL, to
allow both (if the admin decides it's okay).  But I don't see why compression
can't happen before sending to SSL, or after reading from it?

-- 
Justin