-
Notifications
You must be signed in to change notification settings - Fork 2k
[aiohttp] - disable reset query and remove explicit call prepare #9829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[aiohttp] - disable reset query and remove explicit call prepare #9829
Conversation
@Dreamsorcerer please take a look. Performance almost twice better for |
If there's such a big difference, I wonder if something should be changed upstream? Looking through the code, it looks like it already skips that if the server doesn't have those capabilities: If it's CrateDB or CockroachDB then it skips all of that anyway, and on Amazon RedShift only does the RESET ALL. Only on standard PostgresQL does it run all of that query: Which makes me wonder if it's really necessary after all... |
For example, the UNLISTEN is probably not needed if there's no listeners on the connection at reset time: |
Co-authored-by: Sam Bull <[email protected]>
I do not think that disable only On the upstream we can probably expose connection ServerCapabilities to make it public and allow end-user to decide what options he want to have. P.s We can also tune this in other way, since benchmark use keep alive connections, we may try to pin db connections to the specific established connection without acquire/releasing them from pool on each request. |
If you try hardcoding the query locally, can you verify if that's true? My suspicion is that with the async nature, that maybe the contents of the query is having an impact (there are 4 statementns in there after all). I'm a little surprised to see double the performance for that, as it suggests that the reset query is actually taking more time than the regular queries (given that the DB query can't be 100% of the benchmark time). |
So, hardcode to:
Then remove each statement and verify how much impact each part of the query has. |
This is hard to check locally since a have a large deviation due to I run tests on virtual machine and low test duration. So if we want to know exact percentage of improvement. We have to prepare bare-metal machine and test only asyncpg without nginx/aiohttp overhead. SELECT pg_advisory_unlock_all();CLOSE ALL;UNLISTEN *;RESET ALL;
Select 1
|
Right, so looks like sending the query is most of the overhead, but looks like there could still be atleast a 10% improvement if upstream were to reduce the statements sent. |
Yeah, it is actually not double. Initially I have two optimisations inside this PR (remove call of prepare and remove reset query). Let's assume (1-33744.19/40608.53) around 17% |
asyncpg
reset query called each time the connection is released from pool. This do a lot of stuff like close active listeners, release advisory locks, etc. not used in our benchmark so we can omit it.asyncpg
call prepare statement internally, and cache the result inside connection cache => no need to call it explicitly.Before
After