Add name_func optional attribute for asyncpg adapter#9607
Add name_func optional attribute for asyncpg adapter#9607teror4uks wants to merge 1 commit intosqlalchemy:mainfrom
Conversation
|
Hi, can you please also open an issue to properly track this? |
| await_ = staticmethod(await_only) | ||
|
|
||
| def __init__(self, dbapi, connection, prepared_statement_cache_size=100): | ||
| def __init__(self, dbapi, connection, prepared_statement_cache_size=100, name_func=None): |
There was a problem hiding this comment.
I think name_func is too generic, expecially on the engine.
Let's call this pepared_statement_name_func or similar (@zzzeek suggestions here?)
There was a problem hiding this comment.
yeah for sure, my naming choice is quite wide
There was a problem hiding this comment.
I would use prepared_statement_name_func.
here is the problem. this scheme will create endless numbers of prepared statements on pooled connections that are not reused. I think if one is using this scheme, it's very important that discard is used appropriately to at the very least release prepared statements ( as well as transactional state as always) I guess if pgbouncer is used, then on the client (here) we are using NullPool and pgbouncer will have DISCARD set up appropriately but we would to make sure this is mentioned (1. use nullpool w/ pgbouncer; 2. make sure DISCARD is set up).
@teror4uks is that the configuration you are using? basically with this param I want to have an understanding of a working context for it.
There was a problem hiding this comment.
yes you are right we are using NullPool and pgbouncer, the common setup looks like this
engine = create_async_engine(
url,
poolclass=NullPool,
connect_args={
'statement_cache_size': 0,
'connection_class': CConnection,
},
)There was a problem hiding this comment.
OK so when we document this, we should put that example in place and definitely mention that these prepared statements will build up like lightning if they are not cleared out
|
Added the issue with proposal #9608, updated the description. |
|
Thanks! Let's wait for Mike's suggestion on the name of the paramter |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision bd130fe of this pull request into gerrit so we can run tests and reviews and stuff
|
New Gerrit review created for change bd130fe: https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
I've seen few pep issues related to lines longer than 79 or 80 letters so fixed it in the latest commit. |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision e1e5b8d of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset e1e5b8d added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
it seems at the first time I was too optimistic that fixed all issues, flake and black linters found something else, so I ran the same command locally before committing changes again seems now all fixed 🙈 ❯ tox -r -e pep8
......
pep8: commands[7]> python ./tools/sync_test_files.py --check
pep8: commands[8]> python ./tools/generate_sql_functions.py --check
.pkg: _exit> python /home/t4ks/projects/sqlalchemy/venv/lib/python3.10/site-packages/pyproject_api/_backend.py True setuptools.build_meta
pep8: OK (155.95=setup[52.72]+cmd[75.62,0.50,0.93,0.87,5.90,11.67,5.07,0.40,2.26] seconds)
congratulations :) (156.11 seconds) |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision f2bdac6 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset f2bdac6 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
thanks for the update! |
|
mike bayer (zzzeek) wrote: OK code is fine, now the hard part :) documentation, quick example of setup, changelog file. thanks! View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
@teror4uks Let us know if you want to give it a go at writing some docs and the changelog, otherwise we can take it over |
|
@CaselIT I wouldn't mind to add it, here is what I wrote (not yet committed, maybe you have some suggestions) I assume it should be here |
|
looks ok to me, thanks. I would put in in the paragraph above that's also talking about the statement cache, so here https://github.com/teror4uks/sqlalchemy/blob/f2bdac62b20dc6d3afa34e56fa28ae5290a92833/lib/sqlalchemy/dialects/postgresql/asyncpg.py#L100 |
|
@CaselIT done committed changes |
sqla-tester
left a comment
There was a problem hiding this comment.
OK, this is sqla-tester setting up my work on behalf of CaselIT to try to get revision b4bc8d3 of this pull request into gerrit so we can run tests and reviews and stuff
|
Patchset b4bc8d3 added to existing Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
sqla-tester
left a comment
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
two quickies in the changelog , besides that it's ready to go
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565
- doc/build/changelog/unreleased_20/9608.rst (line 5): typo
- doc/build/changelog/unreleased_20/9608.rst (line 8): we could use a seealso here since there's no paramref we can link to
| Prepared Statement Name | ||
| ----------------------- | ||
|
|
||
| By default, asyncpg enumerates prepared statements in numeric order, which |
There was a problem hiding this comment.
mike bayer (zzzeek) wrote:
this is great! nice job, well written
want some more doc tasks ? :)
View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565
There was a problem hiding this comment.
hah thanks, well I might help if find the time :D
|
mike bayer (zzzeek) wrote: changelog quickies View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
I'll fix later today |
|
mike bayer (zzzeek) wrote: alrighty View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
mike bayer (zzzeek) wrote: thanks! View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 |
|
Gerrit review https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565 has been merged. Congratulations! :) |
I faced an issue related to pg bouncer and prepared statement cache flow in asyncpg dialect. Regarding this discussion #6467 I prepared PR to support an optional parameter
namein prepared statement which is allowed, since 0.25.0 version inasyncpgMagicStack/asyncpg#846UPD:
the issue with proposal: #9608
Description
Added optional parameter
name_functoAsyncAdapt_asyncpg_connectionclass which will call on theself._connection.prepare()function and populate a unique name.so in general instead this
would be enough
Checklist
This pull request is:
must include a complete example of the issue. one line code fixes without an
issue and demonstration will not be accepted.
Fixes: #<issue number>in the commit messageinclude a complete example of how the feature would look.
Fixes: #<issue number>in the commit messageHave a nice day!