psycopg icon indicating copy to clipboard operation
psycopg copied to clipboard

Allow integration of psycopg pool with SQLAlchemy

Open dvarrazzo opened this issue 7 months ago • 4 comments

Add close_returns pool parameter. If set, conn.close() will behave like pool.putconn(conn). This is the behaviour expected by SQLAlchemy's NullPool to integrate an external pool: see https://github.com/sqlalchemy/sqlalchemy/discussions/12522

Close #1046

Note that the feature will be released with psycopg_pool 3.3, and will work with any psycopg version, but, with psycopg connections < 3.3, they must be the vanilla psycopg connections, not subclasses. For psycopg < 3.3, the user will need to subclass close() to integrate with SA, instead of setting close_returns.

Please look at the changes in the documentation for the details.

dvarrazzo avatar May 04 '25 21:05 dvarrazzo

@chrispy-snps @CaselIT this branch should be ready to merge. The documentation now has a section for SQLAlchemy integration, the url should be https://www.psycopg.org/psycopg3/docs/advanced/pool.html#pool-sqlalchemy, which will be available as soon as this branch is merged to master and before the package is released.

A review is welcome, and if you wish you can adapt https://github.com/sqlalchemy/sqlalchemy/pull/12540 relying on the documentation here.

dvarrazzo avatar May 08 '25 05:05 dvarrazzo

@dvarrazzo - thank you, this is great!

I'd like to make a merge request to suggest some small grammatical fixes. For simplicity, I will submit it as a subsequent documentation merge request after this branch is merged.

Edit - I would indeed be happy to test this. How do I specifically use this version of Psycopg from a cloned Github repository when doing my testing?

chrispy-snps avatar May 08 '25 11:05 chrispy-snps

I'd like to make a merge request to suggest some small grammatical fixes.

That would be very welcome. You can clone this repo, check out this branch, make the changes you wish and push to your branch. I can just cherry-pick it from there. So we can fix the grammar before merging to master.

Edit - I would indeed be happy to test this. How do I specifically use this version of Psycopg from a cloned Github repository when doing my testing?

Follow the guide in the readme. Roughly:

  • check out this repo, on this branch
  • create and activate a virtualenv with python -m venv .venv && source .venv/bin/activate
  • install psycopg in the virtualenv with pip install --config-settings editable_mode=strict -e ./psycopg -e ./psycopg_pool
  • install SQLAlchemy from package manager via pip
  • test

dvarrazzo avatar May 08 '25 12:05 dvarrazzo

@dvarrazzo - that was a kind way of saying "RTFM," thank you. :) The instructions were easy to follow.

I tried the new functionality and it works great! I updated the examples here to reflect the new support.

chrispy-snps avatar May 12 '25 00:05 chrispy-snps

On lines 512 to 516 of pool.py

            if conn._expire_at <= monotonic():
                logger.info("discarding expired connection %s", conn)
                conn.close()
                self.run_task(AddConnection(self))
                continue

should be changed to

            if conn._expire_at <= monotonic():
                logger.info("discarding expired connection %s", conn)
                conn._pool = None
                conn.close()
                self.run_task(AddConnection(self))
                continue

Otherwise, when close_returns is opened, it will cause an infinite loop. The conn._pool property is not set to empty. self.putconn is triggered again, and the timeout is triggered and conn.close() is executed. At the same time, a new connection is added, causing the database to overload The same applies to other places where conn.close() is executed without setting self._pool to None

zhcn000000 avatar Jul 25 '25 10:07 zhcn000000

@zhcn000000 thank you for your observation, but this ticket is the wrong place to address it. I have opened #1124 for it. If you have anything else to contribute, or if you want to prepare a MR we can continue there.

dvarrazzo avatar Jul 26 '25 11:07 dvarrazzo

Hello peeps! Is there anything else to change to this branch? With the code or the docs? Thank you!

dvarrazzo avatar Sep 05 '25 22:09 dvarrazzo

@dvarrazzo - I must defer the technical assessment to others, but if you are interested, I could contribute a light grammatical edit of the .rst content.

chrispy-snps avatar Sep 07 '25 12:09 chrispy-snps

Thanks for adding this feature! Thanks for filling in the gap and making lives simpler! QQ: Are we planning to merge it to master anytime soon?

Thanks!

AdityaSoni19031997 avatar Oct 04 '25 12:10 AdityaSoni19031997

Time to merge this!

@chrispy-snps documentation review is welcome. I you want to contribute to it please make a new MR for the master branch.

Thank you very much everyone for your contributions!

dvarrazzo avatar Oct 19 '25 02:10 dvarrazzo