Re: libpq compression

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(dot)knizhnik(at)postgrespro(dot)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(at)redhat(dot)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