Skip to content

fix(postgres): clean up leaky cursor coming from raw_sql #11001

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

Merged
merged 3 commits into from
Mar 14, 2025

Conversation

cpcloud
Copy link
Member

@cpcloud cpcloud commented Mar 14, 2025

Description of changes

This PR cleans up a leaky cursor coming from a raw_sql call that never closed
the cursor it returns.

I've also added some SQL that should make it easier to debug the problem of
idle transactions in the future.

@cpcloud cpcloud force-pushed the cleanup-raw_sql-cursor-in-postgres branch from e6af0d6 to f63bc11 Compare March 14, 2025 12:32
@github-actions github-actions bot added the postgres The PostgreSQL backend label Mar 14, 2025
@cpcloud
Copy link
Member Author

cpcloud commented Mar 14, 2025

Ok, this doesn't seem to address the issue yet.

I originally ran a test that was hanging (reproducibly), and then made this change and then it stopped hanging. Back to drawing (read: testing) board!

@cpcloud cpcloud force-pushed the cleanup-raw_sql-cursor-in-postgres branch from f63bc11 to 08246f8 Compare March 14, 2025 14:45
@cpcloud cpcloud force-pushed the cleanup-raw_sql-cursor-in-postgres branch from 08246f8 to 0f9026b Compare March 14, 2025 14:46
@cpcloud
Copy link
Member Author

cpcloud commented Mar 14, 2025

It seems that

@contextlib.contextmanager
def begin(self):
    con = self.con
    with con.cursor() as cursor, con.transaction():
        yield cursor

...

with self.begin() as cursor:
    cursor.execute(create_stmt_sql)
    cursor.executemany(sql, data)

and

con = self.con
with con.cursor() as cursor, con.transaction():
    cursor.execute(create_stmt_sql)
    cursor.executemany(sql, data)

behave differently. The latter doesn't cause an idle transaction deadlock, while the former does.

@github-actions github-actions bot added the tests Issues or PRs related to tests label Mar 14, 2025
@cpcloud cpcloud merged commit c3097a7 into ibis-project:main Mar 14, 2025
104 of 106 checks passed
@cpcloud cpcloud deleted the cleanup-raw_sql-cursor-in-postgres branch March 14, 2025 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postgres The PostgreSQL backend tests Issues or PRs related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant