Skip to content
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

resolve issues 535 and 364 #548

Closed
wants to merge 2 commits into from
Closed

Conversation

vir-mir
Copy link
Member

@vir-mir vir-mir commented Mar 23, 2019

Dear all.

I think we hurried using one cursor to connect.
I learned how the connection works. Cursors are not thread safe: a multithread application can create many cursors from the same connection and should use each cursor from a single thread. See Thread and process safety for details. http://initd.org/psycopg/docs/usage.html#thread-safety

This quote may be misleading:
Connections shouldn’t be shared either by different green threads: see Support for coroutine libraries for further details. this only applies to greenlet more info here (https://github.com/psycopg/psycopg2/blob/8b7506f80d62366ab6bb9e055ff4ea492d16db2c/psycopg/pqpath.c#L830)

aiopg use async inteface (https://github.com/psycopg/psycopg2/blob/8b7506f80d62366ab6bb9e055ff4ea492d16db2c/psycopg/pqpath.c#L874)

issues #364 in that we do not close the cursor, at the time of release for the connection here (https://github.com/aio-libs/aiopg/blob/master/aiopg/sa/engine.py#L244) and here (https://github.com/aio-libs/aiopg/blob/master/aiopg/utils.py#L132)

Solving the problem, was the closure of cursors at the time of release of the connection.
I also track weak references in the SAConnection class to release the cursor that closes before it is released connection.

this is necessary in order to clear cursors for users who do not reconfigure the dialect (https://github.com/aio-libs/aiopg/blob/master/aiopg/sa/engine.py#L39)

The default settings are such that insert and update requests will return to the result. what does not allow to close the cursor.

surprisingly all the tests were successful. and also this example works successfully #364 (comment)

@codecov
Copy link

codecov bot commented Mar 23, 2019

Codecov Report

Merging #548 into master will decrease coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #548      +/-   ##
==========================================
- Coverage   94.35%   94.29%   -0.06%     
==========================================
  Files          27       27              
  Lines        3740     3736       -4     
  Branches      171      172       +1     
==========================================
- Hits         3529     3523       -6     
- Misses        179      181       +2     
  Partials       32       32
Impacted Files Coverage Δ
aiopg/pool.py 100% <ø> (ø) ⬆️
aiopg/sa/result.py 91.83% <100%> (ø) ⬆️
aiopg/sa/engine.py 100% <100%> (ø) ⬆️
aiopg/sa/connection.py 85.95% <100%> (+0.15%) ⬆️
tests/test_sa_cursor.py 100% <100%> (ø) ⬆️
aiopg/cursor.py 94.8% <0%> (-1.3%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fdf7b9...0c98538. Read the comment docs.

@vir-mir
Copy link
Member Author

vir-mir commented Mar 23, 2019

I also added a dialect test. This will be described in the documentation here #534.

async def test_dialect_returning(make_engine, connect):
    dialect = get_dialect()
    dialect.implicit_returning = False

    engine = await make_engine(dialect=dialect)
    async with engine.acquire() as conn:
        res = await conn.execute(tbl.insert().values(id='1', name='test'))
        assert res.closed

        res = await conn.execute(
            tbl.insert().values(id='2', name='test').returning(tbl.c.id)
        )
        assert not res.closed
        assert await res.scalar() == '2'
        assert res.closed

@vir-mir
Copy link
Member Author

vir-mir commented Mar 23, 2019

I will be grateful if you have @runtel will time to check.

@thehesiod
Copy link
Contributor

thehesiod commented Mar 23, 2019

IIRC the prime issue was that you can't use two cursors with active SQL commands on same asynchronous connection as you'll get a complaint from the underlying psycopg library

@thehesiod
Copy link
Contributor

thehesiod commented Mar 23, 2019

Per http://initd.org/psycopg/docs/advanced.html: Two cursors can’t execute concurrent queries on the same asynchronous connection. Ergo one cursor per connection

@vir-mir
Copy link
Member Author

vir-mir commented Mar 25, 2019

This statement is true only for parallel cursors in one connection. async in python this competitiveness, which means opening a new cursor in one connection is correct. the main thing is not to overeat it in parallel processes. I will try to add a test in order to show it clearly.

@CLAassistant

This comment has been minimized.

@Pliner Pliner mentioned this pull request Mar 21, 2021
3 tasks
@Pliner
Copy link
Member

Pliner commented Mar 22, 2021

Resolved by #801

@Pliner Pliner closed this Mar 22, 2021
@Pliner Pliner deleted the resolve_issues_535_and_364 branch April 1, 2021 04: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.

None yet

4 participants