Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fWOTJ-0001Z4-6j for pgsql-hackers@arkaria.postgresql.org; Fri, 22 Jun 2018 15:59:37 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fWOTE-0000WE-P5 for pgsql-hackers@arkaria.postgresql.org; Fri, 22 Jun 2018 15:59:32 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fWOTE-0000W6-IQ for pgsql-hackers@lists.postgresql.org; Fri, 22 Jun 2018 15:59:32 +0000 Received: from mx1.redhat.com ([209.132.183.28]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fWOT5-0003Bd-St for pgsql-hackers@lists.postgresql.org; Fri, 22 Jun 2018 15:59:31 +0000 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id D589BA459F; Fri, 22 Jun 2018 15:59:20 +0000 (UTC) Received: from localhost (ovpn-112-20.rdu2.redhat.com [10.10.112.20]) by smtp.corp.redhat.com (Postfix) with ESMTP id 815D070BC4; Fri, 22 Jun 2018 15:59:20 +0000 (UTC) From: Robbie Harwood To: Konstantin Knizhnik , Thomas Munro Cc: Grigory Smolkin , PostgreSQL Hackers Subject: Re: libpq compression In-Reply-To: <5b673a96-b16e-9a79-c0ec-f7f03f207c62@postgrespro.ru> References: <429cf55b-a8bf-c4ba-196d-5b45420145a1@postgrespro.ru> <36db5d02-45e6-b863-1418-825ef20ae4c6@postgrespro.ru> <3b0260da-fe47-2341-da01-51383fe9172d@postgrespro.ru> <15419613-35b5-0ff6-6261-834abf73330c@postgrespro.ru> <9b3a735c-06c2-631f-8e97-e3982e575210@postgrespro.ru> <5b673a96-b16e-9a79-c0ec-f7f03f207c62@postgrespro.ru> Date: Fri, 22 Jun 2018 11:59:17 -0400 Message-ID: MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" X-Scanned-By: MIMEDefang 2.79 on 10.5.11.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Fri, 22 Jun 2018 15:59:20 +0000 (UTC) List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --=-=-= Content-Type: text/plain Konstantin Knizhnik writes: > On 21.06.2018 20:14, Robbie Harwood wrote: >> Konstantin Knizhnik writes: >>> On 21.06.2018 17:56, Robbie Harwood wrote: >>>> Konstantin Knizhnik writes: >>>>> On 20.06.2018 23:34, Robbie Harwood wrote: >>>>>> Konstantin Knizhnik 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 --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEA5qc6hnelQjDaHWqJTL5F2qVpEIFAlstHNUACgkQJTL5F2qV pEKH3A//REugYx1/CtrMV5g2lr1tW7xmZKNALPNOnCgtrMB98DUU0i5hKrmTHy86 xzHayMFecfvRIoJGcu/IzpHYqgeNtzEQbpZNWRqzFdukKFO+8W461lPHuOTvjvTN I9eMIYeuqs9euNVRiTp/7PB1o6wrVNNE687diLvm7TjCNtGrWkpgX9aaNerizWrx bfJpzvqF/Kdr6lwOQ8tU2ydumgyGP1nrxOoI4HisUAKF3sEz6QvxBif9L+nZynpX n3COScGkaQe2gyhYxCcOpePf2eJ8fmk3X8RsEmxzRD6/ECXycWKhJpinTCi1de2D US0lALNPffqhaRo3kkDAbfJr9ENhBT0WIjb2lEyDUwt/t1Abr6BChfpXaydSnFBu 2m97hkr3EfFwzgR13pkdvs9rYky2d6+WX0lcx/QA3LlXo2EinMWKyzlpLpM4CaMZ Ec8wxH0nXTDo1Px9IFpou4WW42vJVeMuKWhaTz/TmU4tQIvIIKeBl37qbaFgw1yg /poXLrbU+5UJSxf/A3n9wv2j0dXEeU25eHOQN/rZPsFjBYTlPlJx9LFCCqwzG/SX YcPMjSidsWNz8lndMrPaJLoYDLWl3EHgYI8JsntuKnZH2fYyOg7YuIdHzVra2TdJ 63LIAQeQ3t8HV0h1545pObdr7F9n+pLvubQKoOogH6nA6xbsJWo= =hhUV -----END PGP SIGNATURE----- --=-=-=--