Re: libpq compression

Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:

> On 21.06.2018 20:14, Robbie Harwood wrote:
>> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>>> On 21.06.2018 17:56, Robbie Harwood wrote:
>>>> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>>>>> On 20.06.2018 23:34, Robbie Harwood wrote:
>>>>>> Konstantin Knizhnik <k.knizhnik@postgrespro.ru> writes:
>>>>>>
>>>>>> Well, that's a design decision you've made.  You could put
>>>>>> lengths on chunks that are sent out - then you'd know exactly how
>>>>>> much is needed.  (For instance, 4 bytes of network-order length
>>>>>> followed by a complete payload.)  Then you'd absolutely know
>>>>>> whether you have enough to decompress or not.
>>>>>
>>>>> Do you really suggest to send extra header for each chunk of data?
>>>>> Please notice that chunk can be as small as one message: dozen of
>>>>> bytes because libpq is used for client-server communication with
>>>>> request-reply pattern.
>>>>
>>>> I want you to think critically about your design.  I *really* don't
>>>> want to design it for you - I have enough stuff to be doing.  But
>>>> again, the design I gave you doesn't necessarily need that - you
>>>> just need to properly buffer incomplete data.
>>>
>>> Right now secure_read may return any number of available bytes. But
>>> in case of using streaming compression, it can happen that available
>>> number of bytes is not enough to perform decompression. This is why
>>> we may need to try to fetch additional portion of data. This is how
>>> zpq_stream is working now.
>>
>> No, you need to buffer and wait until you're called again.  Which is
>> to say: decompress() shouldn't call secure_read().  secure_read()
>> should call decompress().
>
> I this case I will have to implement this code twice: both for backend
> and frontend, i.e. for secure_read/secure_write and
> pqsecure_read/pqsecure_write.

Likely, yes.  You can see that this is how TLS does it (which you should
be using as a model, architecture-wise).

> Frankly speaking i was very upset by design of libpq communication
> layer in Postgres: there are two different implementations of almost
> the same stuff for cbackend and frontend.

Changing the codebases so that more could be shared is not necessarily a
bad idea; however, it is a separate change from compression.

>>> I do not understand how it is possible to implement in different way
>>> and what is wrong with current implementation.
>>
>> The most succinct thing I can say is: absolutely don't modify
>> pq_recvbuf().  I gave you pseudocode for how to do that.  All of your
>> logic should be *below* the secure_read/secure_write functions.
>>
>> I cannot approve code that modifies pq_recvbuf() in the manner you
>> currently do.
>
> Well. I understand you arguments.  But please also consider my
> argument above (about avoiding code duplication).
>
> In any case, secure_read function is called only from pq_recvbuf() as
> well as pqsecure_read is called only from pqReadData.  So I do not
> think principle difference in handling compression in secure_read or
> pq_recvbuf functions and do not understand why it is "destroying the
> existing model".
>
> Frankly speaking, I will really like to destroy existed model, moving
> all system dependent stuff in Postgres to SAL and avoid this awful mix
> of code sharing and duplication between backend and frontend. But it
> is a another story and I do not want to discuss it here.

I understand you want to avoid code duplication.  I will absolutely
agree that the current setup makes it difficult to share code between
postmaster and libpq clients.  But the way I see it, you have two
choices:

1. Modify the code to make code sharing easier.  Once this has been
   done, *then* build a compression patch on top, with the nice new
   architecture.

2. Leave the architecture as-is and add compression support.
   (Optionally, you can make code sharing easier at a later point.)

Fundamentally, I think you value code sharing more than I do.  So while
I might advocate for (2), you might personally prefer (1).

But right now you're not doing either of those.

> If we are speaking about the "right design", then neither your
> suggestion, neither my implementation are good and I do not see
> principle differences between them.
>
> The right approach is using "decorator pattern": this is how streams
> are designed in .Net/Java. You can construct pipe of "secure",
> "compressed" and whatever else streams.

I *strongly* disagree, but I don't think you're seriously suggesting
this.

>>>>>> My idea was the following: client want to use compression. But
>>>>>> server may reject this attempt (for any reasons: it doesn't support
>>>>>> it, has no proper compression library, do not want to spend CPU for
>>>>>> decompression,...) Right now compression algorithm is
>>>>>> hardcoded. But in future client and server may negotiate to choose
>>>>>> proper compression protocol.  This is why I prefer to perform
>>>>>> negotiation between client and server to enable compression.  Well,
>>>>>> for negotiation you could put the name of the algorithm you want in
>>>>>> the startup.  It doesn't have to be a boolean for compression, and
>>>>>> then you don't need an additional round-trip.
>>>>> Sorry, I can only repeat arguments I already mentioned:
>>>>> - in future it may be possible to specify compression algorithm
>>>>> - even with boolean compression option server may have some reasons to
>>>>> reject client's request to use compression
>>>>>
>>>>> Extra flexibility is always good thing if it doesn't cost too
>>>>> much. And extra round of negotiation in case of enabling compression
>>>>> seems to me not to be a high price for it.
>>>> You already have this flexibility even without negotiation.  I don't
>>>> want you to lose your flexibility.  Protocol looks like this:
>>>>
>>>> - Client sends connection option "compression" with list of
>>>>    algorithms it wants to use (comma-separated, or something).
>>>>
>>>> - First packet that the server can compress one of those algorithms
>>>>    (or none, if it doesn't want to turn on compression).
>>>>
>>>> No additional round-trips needed.
>>> This is exactly how it works now...  Client includes compression
>>> option in connection string and server replies with special message
>>> ('Z') if it accepts request to compress traffic between this client
>>> and server.
>>
>> No, it's not.  You don't need this message.  If the server receives a
>> compression request, it should just turn compression on (or not), and
>> then have the client figure out whether it got compression back.
>
> How it will managed to do it. It receives some reply and first of all
> it should know whether it has to be decompressed or not.

You can tell whether a message is compressed by looking at it.  The way
the protocol works, every message has a type associated with it: a
single byte, like 'R', that says what kind of message it is.

Thanks,
--Robbie