pgwire: send BackendKeyData message for pgpool compatibility #13009
Conversation
pgpool doesn't open a connection succesfully to a server unless a BackendKeyData message is received before a ReadyForQuery message. This message is meaningless for now.
|
Can we test this somehow? Perhaps we need a new acceptance test; we can add Reviewed 1 of 1 files at r1. Comments from Reviewable |
|
This either needs a test with multiple connections to make sure using process IDs of 0 works with pgpool, or we need to generate process IDs and secret keys that are useful. Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed. Comments from Reviewable |
|
I've been working on adding an acceptance test with pgpool in front of a secure cluster for the last couple of days. I got to the point where everything ran up until pgpool tried to perform authentication with the cluster but never provided any certificates. This is an issue with the version of pgpool I was using and I think that upgrading will probably fix it. |
|
Indeed, looks like the relevant commit went in last summer and was
backported to the various branches (at least back to 3.1:
pgpool/pgpool2@63de4b2
).
Looks like this change hit all 3.{1..6} branches by August of last year (
http://pgpool.net/mediawiki/index.php/Downloads).
…On Thu, Jan 26, 2017 at 3:00 PM, Alfonso Subiotto Marqués < ***@***.***> wrote:
I've been working on adding an acceptance test with pgpool in front of a
secure cluster for the last couple of days. I got to the point where
everything ran up until pgpool tried to perform authentication with the
cluster but never provided any certificates. This is an issue with the
version of pgpool I was using and I think that upgrading will probably
fix it
<http://www.sraoss.jp/pipermail/pgpool-hackers/2012-October/000137.html>.
However, I think I've spent a little too much time on this so I'm going to
leave it for a while and focus on draining work since compatibility with
pgpool is not a top priority right now.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13009 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPEZ8-2UHb1uPDrpCsYlGGMgCQoOKks5rWPtEgaJpZM4LokkR>
.
|
|
According to jordan, this is also required by a python client; it may be easier to add a test that uses that client that it is to add a pgpool test. |
|
The code LGTM. I think we should add a test that uses the official |
|
We already have such a test; it is in pkg/acceptance/c_test.go
…On Fri, Jan 27, 2017 at 3:24 PM, Jordan Lewis ***@***.***> wrote:
The code LGTM. I think we should add a test that uses the official libpq
to negotiate a connection if we don't already have one of those. Clients
have quirks - I think having a test against a reference library (and the Go
one?) is probably good enough.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13009 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPK2GaPnb0GkA2W5hWFWYFHW6BFkPks5rWlKWgaJpZM4LokkR>
.
|
|
In that case, now that this issue is relevant for more than just pgpool, I would personally feel comfortable merging this without additional testing because the pgpool compatibility (and in particular the multiple-connection test that @mjibson mentions) could be tracked in a separate issue/PR. |
|
Why not just add the python test? There's plenty of prior art for cross
language tests. In fact, we already have a python test that will just need
to extended.
What's the hurry?
…On Fri, Jan 27, 2017 at 3:37 PM, Jordan Lewis ***@***.***> wrote:
In that case, now that this issue is relevant for more than just pgpool, I
would personally feel comfortable merging this without additional testing
because the libpq test would fail if we did not provide a valid
connection handshake.
pgpool compatibility (and in particular the multiple-connection test that
@mjibson <https://github.com/mjibson> mentions) could be tracked in a
separate issue/PR.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13009 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPCcHgQ4Y1LAk9zeYVkiAYF7GpD8Sks5rWlWggaJpZM4LokkR>
.
|
|
Fixes #13191. |
|
If lib/pq#535 gets merged then we can test this with a Go client really easily, using context methods. |
|
Well we'd need to be on 1.8 as well.
…On Jan 28, 2017 10:41, "Matt Jibson" ***@***.***> wrote:
If lib/pq#535 <lib/pq#535> gets merged then we
can test this with a Go client really easily, using context methods.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#13009 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABdsPLAkf63hWUtfSNJ9CbCqGStJBYKsks5rW2GXgaJpZM4LokkR>
.
|
|
I would rather wait until we generate process IDs and secret keys that are useful once we support |
|
@asubiotto all the prerequisites for this are in place now, right? |
|
Sorry for the late response, this got lost in my inbox. No, I think the wire-level support for cancellation was decided against: #17252 (comment). The way to move forward with this might be to generate random data and return some "unimplemented" message when cancellation messages are sent. |
|
Closing this (for now?) since there is no work being done on this PR. |
pgpool doesn't open a connection succesfully to a server unless a BackendKeyData
message is received before a ReadyForQuery message. This message is meaningless
for now.
This change is