Subject: Re: libpq compression
To: Dmitry Dolgov <9erthalion6@gmail.com>,
 Robbie Harwood <rharwood@redhat.com>
Cc: Andreas Karlsson <andreas@proxel.se>,
 Peter Eisentraut <peter.eisentraut@2ndquadrant.com>,
 Andres Freund <andres@anarazel.de>,
 PostgreSQL Hackers <pgsql-hackers@postgresql.org>
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>
From: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
Message-ID: <9d3f63af-3968-4b57-d4b9-939eff5452b6@postgrespro.ru>
Date: Thu, 14 Feb 2019 20:32:15 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.2.1
MIME-Version: 1.0
In-Reply-To: 
 <CA+q6zcW=JyhpyxVK-c-Yt3OO54CYRcL6jKtOkxvrM+Jz-oCYBg@mail.gmail.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


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