Skip to content

Add name_func optional attribute for asyncpg adapter#9607

Closed
teror4uks wants to merge 1 commit intosqlalchemy:mainfrom
teror4uks:issue-6467
Closed

Add name_func optional attribute for asyncpg adapter#9607
teror4uks wants to merge 1 commit intosqlalchemy:mainfrom
teror4uks:issue-6467

Conversation

@teror4uks
Copy link
Contributor

@teror4uks teror4uks commented Apr 6, 2023

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 name in prepared statement which is allowed, since 0.25.0 version in asyncpg MagicStack/asyncpg#846

UPD:
the issue with proposal: #9608

Description

Added optional parameter name_func to AsyncAdapt_asyncpg_connection class which will call on the self._connection.prepare() function and populate a unique name.

so in general instead this

from uuid import uuid4

from asyncpg import Connection


class CConnection(Connection):
    def _get_unique_id(self, prefix: str) -> str:
        return f'__asyncpg_{prefix}_{uuid4()}__'

engine = create_async_engine(...,     
    connect_args={
        'connection_class': CConnection,
    },
)

would be enough

from uuid import uuid4

engine = create_async_engine(...,     
    connect_args={
        'name_func': lambda:  f'__asyncpg_{uuid4()}__',
    },
)

Checklist

This pull request is:

  • A documentation / typographical error fix
    • Good to go, no issue or tests are needed
  • 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!

@CaselIT
Copy link
Member

CaselIT commented Apr 6, 2023

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think name_func is too generic, expecially on the engine.

Let's call this pepared_statement_name_func or similar (@zzzeek suggestions here?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah for sure, my naming choice is quite wide

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
    },
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@teror4uks
Copy link
Contributor Author

Added the issue with proposal #9608, updated the description.

@CaselIT
Copy link
Member

CaselIT commented Apr 6, 2023

Thanks! Let's wait for Mike's suggestion on the name of the paramter

Copy link
Member

@CaselIT CaselIT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@CaselIT CaselIT requested a review from sqla-tester April 13, 2023 07:03
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sqla-tester
Copy link
Collaborator

@teror4uks
Copy link
Contributor Author

I've seen few pep issues related to lines longer than 79 or 80 letters so fixed it in the latest commit.

@CaselIT CaselIT requested a review from sqla-tester April 14, 2023 13:15
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sqla-tester
Copy link
Collaborator

@teror4uks
Copy link
Contributor Author

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)

@CaselIT CaselIT requested a review from sqla-tester April 14, 2023 17:31
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sqla-tester
Copy link
Collaborator

@CaselIT
Copy link
Member

CaselIT commented Apr 14, 2023

thanks for the update!

@sqla-tester
Copy link
Collaborator

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

@CaselIT
Copy link
Member

CaselIT commented Apr 18, 2023

@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

@teror4uks
Copy link
Contributor Author

@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

Prepared Statement Name
-----------------------

By default, asyncpg enumerates prepared statements in numeric order, which 
can lead to errors if a name has already been taken for another prepared 
statement. This issue can arise if your application uses database proxies 
such as PgBouncer to handle connections. One possible workaround is to 
use dynamic prepared statement names, which asyncpg now supports through 
an optional name value for the statement name. This allows you to 
generate your own unique names that won't conflict with existing ones. 
To achieve this, you can provide a function that will be called every time 
a prepared statement is prepared::

    from uuid import uuid4

    engine = create_async_engine(
        "postgresql+asyncpg://user:pass@hostname/dbname",     
        poolclass=NullPool,
        connect_args={
            'pepared_statement_name_func': lambda:  f'__asyncpg_{uuid4()}__',
        },
    )

.. seealso::

   https://github.com/MagicStack/asyncpg/issues/837

   https://github.com/sqlalchemy/sqlalchemy/issues/6467

.. warning:: To prevent a buildup of useless prepared statements in 
   your application, it's important to use the ``NullPool`` poolclass and 
   PgBouncer with a configured `DISCARD https://www.postgresql.org/docs/current/sql-discard.html`_ 
   setup. The DISCARD command is used to release resources held by the db connection, 
   including prepared statements. Without proper setup, prepared statements can 
   accumulate quickly and cause performance issues.


@CaselIT
Copy link
Member

CaselIT commented Apr 20, 2023

@teror4uks
Copy link
Contributor Author

@CaselIT done committed changes

@CaselIT CaselIT requested a review from sqla-tester April 20, 2023 17:40
Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@sqla-tester
Copy link
Collaborator

Copy link
Collaborator

@sqla-tester sqla-tester left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hah thanks, well I might help if find the time :D

@sqla-tester
Copy link
Collaborator

mike bayer (zzzeek) wrote:

changelog quickies

View this in Gerrit at https://gerrit.sqlalchemy.org/c/sqlalchemy/sqlalchemy/+/4565

@CaselIT
Copy link
Member

CaselIT commented Apr 21, 2023

I'll fix later today

@sqla-tester
Copy link
Collaborator

@sqla-tester
Copy link
Collaborator

@sqla-tester
Copy link
Collaborator

@teror4uks teror4uks deleted the issue-6467 branch April 23, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants