Date: Wed, 2 Mar 2022 15:33:52 -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: <20220302213352.GA27651@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>
 <20220118043919.GA23027@telsasoft.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20220118043919.GA23027@telsasoft.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220302213352.GA27651%40telsasoft.com>
Precedence: bulk

If there's no objection, I'd like to move this to the next CF for consideration
in PG16.

On Mon, Jan 17, 2022 at 10:39:19PM -0600, Justin Pryzby wrote:
> 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?