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.knizh...@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 <rharw...@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
