Change interaction with server to only send one SYNC message (Pgbouncer support) #493
Conversation
|
Hi @fvannee. Thanks for taking a stab at this! This looks like a working fix, albeit I don't have the setup to test properly. Do you mind outlining how you tested this? Please include the description above in the commit message. A comment in |
|
@elprans Ok, thanks for your feedback. I added a comment to _process__prepare() that explains it in a bit more detail and changed the commit message. |
You probably didn't push that yet, as I don't see it in the diff.
Cool.
That's right. |
|
@elprans Oops, indeed. Now it's there. |
asyncpg/protocol/coreproto.pyx
Outdated
| # this effectively clears the error and we then wait until we get a | ||
| # ready for new query message | ||
| self._write(SYNC_MESSAGE) | ||
| self._set_state(PROTOCOL_CANCELLED) |
There was a problem hiding this comment.
It seems that PROTOCOL_ERROR_CONSUME would be more appropriate here.
There was a problem hiding this comment.
Sure, I can change it to that if preferred.
| @@ -588,6 +588,8 @@ cdef class BaseProtocol(CoreProtocol): | |||
| }) | |||
| self.abort() | |||
|
|
|||
| if self.state == PROTOCOL_PREPARE: | |||
| self._write(SYNC_MESSAGE) | |||
There was a problem hiding this comment.
It's probably safe to send Sync unconditionally here.
There was a problem hiding this comment.
It's not - some of the testcases fail when done unconditionally (I first had it like that). We get into a situation where we may end up in IDLE state due to the first sync received, and then receive another ReadyForQuery message - the current state machine goes into error state when that happens
There was a problem hiding this comment.
that may actually have to do with existing code line 120 - on receiving a Z message it doesn't parse the state. Isn't there supposed to be a:
self._parse_msg_ready_for_query()
on line 120?
edit: ah no, i'm mixing some things up now. We have to send it conditionally, because in IDLE state we cannot receive a Z message (unless we change _read_server_messages as well to allow receiving Z message in IDLE state).
Still, I think line 120 needs this parse_msg_ready_for_query line, no?
There was a problem hiding this comment.
Yes, that is suspicious. Server messages should always be read in full. I'd say it's OK to also receive any number of ReadyForQuery in the IDLE state. This should make it safe to send any number of Sync messages.
There was a problem hiding this comment.
I've experimented a bit with removing the if-statement and putting the ReadyForQuery handling in the state machine in IDLE state. However, it's giving me some warnings I'm not sure are good.
All test cases pass, but I see some extra lines like this after some test cases and I can't figure out exactly why it's not waiting for these.
Future exception was never retrieved
future: <Future finished exception=BrokenPipeError(32, 'Broken pipe')>
Traceback (most recent call last):
File "/usr/lib64/python3.6/asyncio/selector_events.py", line 417, in _sock_sendall
n = sock.send(data)
BrokenPipeError: [Errno 32] Broken pipe
I propose to keep this line as it is for now.
There was a problem hiding this comment.
OK. Let's keep it, but please add a comment on why it's needed.
This is a simple implementation of PgBouncer support for asyncpg. It doesn't have any detection features, but at least changes asyncpg behavior in such a way that using PgBouncer is possible. This commit gets rid of the explicit SYNC after a parse/describe sequence and changes is to a FLUSH. This should work regardless of the setting of statement_cache_size and whether or not it's pgbouncer or a direct postgres connection. With this, PgBouncer is supported when setting statement_cache_size explicitly to 0.
|
Is this good to merge? |
This is a simple implementation of PgBouncer support for asyncpg. It doesn't have any detection features, but at least changes asyncpg behavior in such a way that using pgbouncer is possible.
I implemented part of your suggestions as described in #348 (comment) , but in a simpler way. I just got rid of the explicit SYNC after a parse/describe and changed it to a FLUSH. This should work regardless of the setting of statement_cache_size and whether or not it's pgbouncer or direct postgres connection.
With this, if the user wants to use PgBouncer, he'll still have to set statement_cache_size to 0, however once he does that, everything will work as expected.
Please let me know if this solution is acceptable to merge.