set statement name in `set_type_codec`

[CobaltCause]

Description

This is necessary when using PgBouncer because otherwise the default simplistically-generated statement names can fight with other programs using asyncpg due to PgBouncer sharing the same underlying PostgreSQL connection and thus the prepared statement names.

Supposedly asyncpg can be made to use an unnamed statement here by turning the cache off, but I can't for the life of me figure out how to do that when using asyncpg through SQLAlchemy; I observe that setting prepared_statement_cache_size to 0 as described here doesn't cause use_cache to be False.

I think this functionality should probably be provided either way since it'd be kinda weird to only allow configuring the names of some prepared statements.

Requires: https://github.com/MagicStack/asyncpg/pull/1131 Fixes: https://github.com/sqlalchemy/sqlalchemy/issues/11141

As for testing, I'm not sure how to go about adding tests for this.

Checklist

This pull request is:

• [ ] A documentation / typographical / small typing error fix
  • Good to go, no issue or tests are needed
• [X] A short code fix
  • please include the issue number, and create an issue if none exists, which must include a complete example of the issue. one line code fixes without an issue and demonstration will not be accepted.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests. one line code fixes without tests will not be accepted.
• [ ] A new feature implementation
  • please include the issue number, and create an issue if none exists, which must include a complete example of how the feature would look.
  • Please include: Fixes: #<issue number> in the commit message
  • please include tests.

Have a nice day!

[zzzeek]

hi -

these are type codecs, not prepared statements? why would the prepared staetment name function be used here? if you just need a "distinct name", this is not the way to do that. a name like a uuid would be better, but then I think this should be in asyncpg anyway.

[CobaltCause]

I think this should be in asyncpg anyway

if you are proposing that asyncpg adds that field

Yeah, see https://github.com/MagicStack/asyncpg/pull/1131 which I linked in the PR description. That's why this PR is drafted.

as mentioned at https://github.com/sqlalchemy/sqlalchemy/issues/11141 there are no details or information what the actual problem looks like

Sorry, I tried to elaborate on this in https://github.com/sqlalchemy/sqlalchemy/issues/11141#issuecomment-1989677259.

these are connection-wide structures, not prepared statements

Sorry if this is a stupid question, but if they aren't prepared statements, why is asyncpg's code for preparing statements here invoked when calling set_type_codec?

we can't just flip it on like that, that would be massively backwards incompatible

Again sorry if this is a stupid question, but is it not feasible to require a minimum version of asyncpg?

[zzzeek]

I think this should be in asyncpg anyway

if you are proposing that asyncpg adds that field

Yeah, see MagicStack/asyncpg#1131 which I linked in the PR description. That's why this PR is drafted.

as mentioned at #11141 there are no details or information what the actual problem looks like

Sorry, I tried to elaborate on this in #11141 (comment).

these are connection-wide structures, not prepared statements

Sorry if this is a stupid question, but if they aren't prepared statements, why is asyncpg's code for preparing statements here invoked when calling set_type_codec?

If it's a prepared statement name inside of asyncpg, that's fine, but from our POV this is not one of our PreparedStatements, we know nothing about what this function does internally. it's also not used in the same scope - it's per connection, not per statement.

if you really want this to be considered as a prepared statement then "name" is a poor name for the argument on the asyncpg side, it should be "prepared_statement_name".

this is really all things that asynpg should be handling. it's pretty awful they are leaking all these internal details.

Again sorry if this is a stupid question, but is it not feasible to require a minimum version of asyncpg?

None of your questions are "stupid" and there's no need to qualify for that.

asyncpg is not a requirement of SQLAlchemy, you can put asyncpg and SQLAlchemy on separate lines in a requirements file and there is no mechanism to have an older asyncpg blocked unless they are also using the postgresql_asyncpg extra, which is optional. I think we even looked into this with the pypa folks and it's not a thing right now.

also I dont want to force an asyncpg upgrade on a SQLAlchemy minor version in any case.

[CaselIT]

Sorry if this is a stupid question, but if they aren't prepared statements, why is asyncpg's code for preparing statements here invoked when calling set_type_codec?

In the default calling form (named=False) seems to use an empty string as the name of the statement, that you mentioned makes postgresql use unnamed statements. So again what is the issue you are seeing? Is there a stack trace?

[CobaltCause]

In the default calling form (named=False) seems to use an empty string as the name of the statement

Because of this code in asyncpg, the statement can and does still get a name because use_cache is True. I briefly mentioned this in the PR description.

[CaselIT]

This in on hold until the asyncpg devs decide what to do. I personally don't think it makes much sense to add named to set_type_codec, since from the user prospective this call does imply any query (or it may be 1000 queries)

but that's on asyncpg to decide what to do here

[CobaltCause]

Closing this since it's been a while and I've since solved my problem by using psycopg3 instead, which supports disabling prepared statements altogether.