On Fri, Aug 10, 2018 at 5:55 PM, Andrew Dunstan <andrew.duns...@2ndquadrant.com> wrote: > This thread appears to have gone quiet. What concerns me is that there > appears to be substantial disagreement between the author and the reviewers. > Since the last thing was this new patch it should really have been put back > into "needs review" (my fault to some extent - I missed that). So rather > than return the patch with feedfack I'm going to set it to "needs review" > and move it to the next CF. However, if we can't arrive at a consensus about > the direction during the next CF it should be returned with feedback.
I agree with the critiques from Robbie Harwood and Michael Paquier that the way in that compression is being hooked into the existing architecture looks like a kludge. I'm not sure I know exactly how it should be done, but the current approach doesn't look natural; it looks like it was bolted on. I agree with the critique from Peter Eisentraut and others that we should not go around and add -Z as a command-line option to all of our tools; this feature hasn't yet proved itself to be useful enough to justify that. Better to let people specify it via a connection string option if they want it. I think Thomas Munro was right to ask about what will happen when different compression libraries are in use, and I think failing uncleanly is quite unacceptable. I think there needs to be some method for negotiating the compression type; the client can, for example, provide a comma-separated list of methods it supports in preference order, and the server can pick the first one it likes. In short, I think that a number of people have provided really good feedback on this patch, and I suggest to Konstantin that he should consider accepting all of those suggestions. Commit ae65f6066dc3d19a55f4fdcd3b30003c5ad8dbed tried to introduce some facilities that can be used for protocol version negotiation as new features are added, but this patch doesn't use them. It looks to me like it instead just breaks backward compatibility. The new 'compression' option won't be recognized by older servers. If it were spelled '_pq_.compression' then the facilities in that commit would cause a NegotiateProtocolVersion message to be sent by servers which have that commit but don't support the compression option. I'm not exactly sure what will happen on even-older servers that don't have that commit either, but I hope they'll treat it as a GUC name; note that setting an unknown GUC name with a namespace prefix is not an error, but setting one without such a prefix IS an ERROR. Servers which do support compression would respond with a message indicating that compression had been enabled or, maybe, just start sending back compressed-packet messages, if we go with including some framing in the libpq protocol. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
