From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: Dmitry Dolgov <9erthalion6@gmail.com>
To: Robbie Harwood <rharwood@redhat.com>
Cc: Andreas Karlsson <andreas@proxel.se>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: PostgreSQL Hackers <pgsql-hackers@postgresql.org>
Subject: Re: libpq compression
Date: Thu, 14 Feb 2019 20:32:15 +0300
Message-ID: <9d3f63af-3968-4b57-d4b9-939eff5452b6@postgrespro.ru> (raw)
In-Reply-To: <CA+q6zcW=JyhpyxVK-c-Yt3OO54CYRcL6jKtOkxvrM+Jz-oCYBg@mail.gmail.com>
References: <aad16e41-b3f9-e89d-fa57-fb4c694bec25@postgrespro.ru>
<20190208071431.skfmpbeclrkpzuac@alap3.anarazel.de>
<8d210267-a2fa-662f-b167-0e5991d45dd7@proxel.se>
<eb3e3b41-24d1-8898-f71d-2975477b6a83@2ndquadrant.com>
<436d1e08-dd9c-d36a-1c0d-517a70a83cd4@proxel.se>
<92435f1b-77aa-4386-4cf1-788b3c3d4365@postgrespro.ru>
<CA+q6zcUPrssNaRS+FyoBsD-F2stK1Roa-4sAhFOfAjOWLziM4g@mail.gmail.com>
<dcc86dc6-5401-f2cc-9b3b-b5bcca2d7a45@postgrespro.ru>
<jlgpnrv8oth.fsf@redhat.com>
<CA+q6zcW=JyhpyxVK-c-Yt3OO54CYRcL6jKtOkxvrM+Jz-oCYBg@mail.gmail.com>
On 14.02.2019 19:45, Dmitry Dolgov wrote:
> For the records, I'm really afraid of interfering with the conversation at this
> point, but I believe it's necessary for the sake of a good feature :)
>
>> On Wed, Feb 13, 2019 at 4:03 PM Konstantin Knizhnik <k.knizhnik@postgrespro.ru> wrote:
>>
>> 1. When decompressor has not enough data to produce any extra output, it
>> doesn't return error. It just not moving forward output position in the
>> buffer.
> I'm confused, because here is what I see in zlib:
>
> // zlib.h
> inflate() returns Z_OK if some progress has been made, ... , Z_BUF_ERROR
> if no progress was possible or if there was not enough room in the output
> buffer when Z_FINISH is used. Note that Z_BUF_ERROR is not fatal, and
> inflate() can be called again with more input and more output space to
> continue decompressing.
>
> So, sounds like if no moving forward happened, there should be Z_BUF_ERROR. But
> I haven't experimented with this yet to figure out.
Looks like I really missed this case.
I need to handle Z_BUF_ERROR in zlib_read:
if (rc != Z_OK && rc != Z_BUF_ERROR)
{
return ZPQ_DECOMPRESS_ERROR;
}
Strange that it works without it. Looks like written compressed message
is very rarely splitted because of socket buffer overflow.
>
>> Ok, but still I think that it is better to pass tx/rx function to stream.
>> There are two important advantages:
>> 1. It eliminates code duplication.
> Ok.
>
>> 2. It allows to use (in future) this streaming compression not only for
>> libpq for for other streaming data.
> Can you elaborate on this please?
All this logic with fetching enough data to perform successful
decompression of data chunk is implemented in one place - in zpq_stream
and it is not needed to repeat it in all places where compression is used.
IMHO passing rx/tx function to compressor stream is quite natural model
- it is "decorator design pattern"
https://en.wikipedia.org/wiki/Decorator_pattern
(it is how for example streams are implemented in Java).
>
>> Concerning "layering violation" may be it is better to introduce some other
>> functions something like inflate_read, deflate_write and call them instead of
>> secure_read. But from my point of view it will not improve readability and
>> modularity of code.
> If we will unwrap the current compression logic to not contain tx/rx functions,
> isn't it going to be the same as you describing it anyway, just from the higher
> point of view? What I'm saying is that there is a compression logic, for it
> some data would be read or written from it, just not right here an now by
> compression code itself, but rather by already existing machinery (which could
> be even beneficial for the patch implementation).
I do not understand why passing rx/tx functions to zpq_create is
violating existed machinery.
>
>> And I do not see any disadvantages.
> The main disadvantage, as I see it, is that there is no agreement about this
> approach. Probably in such situations it makes sense to experiment with
> different suggestions, to see how would they look like - let's be flexible.
Well, from my point of view approach with rx/tx is more flexible and
modular.
But if most of other developers think that using read/read_drain is
preferable,
then I will not complaint against using your approach.
>
>> On Wed, Feb 13, 2019 at 8:34 PM Robbie Harwood <rharwood@redhat.com> wrote:
>>
>> In order to comply with your evident desires, consider this message a
>> courtesy notice that I will no longer be reviewing this patch or
>> accepting future code from you.
> I'm failing to see why miscommunication should necessarily lead to such
> statements, but it's your decision after all.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: k.knizhnik@postgrespro.ru, 9erthalion6@gmail.com, rharwood@redhat.com, andreas@proxel.se, peter.eisentraut@2ndquadrant.com, andres@anarazel.de
Subject: Re: libpq compression
In-Reply-To: <9d3f63af-3968-4b57-d4b9-939eff5452b6@postgrespro.ru>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox