From: Robbie Harwood <rharwood@redhat.com>
To: Konstantin Knizhnik <k.knizhnik@postgrespro.ru>
To: Thomas Munro <thomas.munro@enterprisedb.com>
Cc: Grigory Smolkin <g.smolkin@postgrespro.ru>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: libpq compression
Date: Fri, 22 Jun 2018 11:59:17 -0400
Message-ID: <jlgin6a6cdm.fsf@redhat.com> (raw)
In-Reply-To: <5b673a96-b16e-9a79-c0ec-f7f03f207c62@postgrespro.ru>
References: <aad16e41-b3f9-e89d-fa57-fb4c694bec25@postgrespro.ru>
<dbf9035f-7f7b-b053-cc2a-52e5147d4557@postgrespro.ru>
<429cf55b-a8bf-c4ba-196d-5b45420145a1@postgrespro.ru>
<CAEepm=3CGUX4Vyw=jFaOedt99HS1HQiBKYTX4OYHo7HU2AczZg@mail.gmail.com>
<36db5d02-45e6-b863-1418-825ef20ae4c6@postgrespro.ru>
<CAEepm=3RAS8xVQK8rs+Xitzp+JXzGdGsMA7f+H8fjKQ7Do8aSA@mail.gmail.com>
<f4ad0809-dd90-e321-5c1a-0ca68a136a8f@postgrespro.ru>
<jlg4lhzdeak.fsf@redhat.com>
<3b0260da-fe47-2341-da01-51383fe9172d@postgrespro.ru>
<jlgzhzqbi84.fsf@redhat.com>
<15419613-35b5-0ff6-6261-834abf73330c@postgrespro.ru>
<jlgo9g5qjst.fsf@redhat.com>
<9b3a735c-06c2-631f-8e97-e3982e575210@postgrespro.ru>
<jlgfu1gqjbk.fsf@redhat.com>
<f7396de1-8c65-ee7f-e662-8a4c3cf59e4c@postgrespro.ru>
<jlgd0wkqcyq.fsf@redhat.com>
<5b673a96-b16e-9a79-c0ec-f7f03f207c62@postgrespro.ru>
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
Attachments:
signature.asc (application/pgp-signature, 832B) download
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: rharwood@redhat.com, k.knizhnik@postgrespro.ru, thomas.munro@enterprisedb.com, g.smolkin@postgrespro.ru, pgsql-hackers@lists.postgresql.org
Subject: Re: libpq compression
In-Reply-To: <jlgin6a6cdm.fsf@redhat.com>
* 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