GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-42972: Track sqlite3 statement objects by erlend-aasland · Pull Request #26475 · python/cpython

Conversation

erlend-aasland commented May 31, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

https://bugs.python.org/issue42972

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          bpo-42972: Track sqlite3 statement objects

36ae3eb

erlend-aasland requested a review from berkerpeksag as a code owner

May 31, 2021 22:49

the-knights-who-say-ni added the CLA signed label

May 31, 2021

bedevere-bot added the awaiting review label

May 31, 2021

erlend-aasland requested a review from pablogsal

May 31, 2021 22:49

erlend-aasland added needs backport to 3.10 skip news labels

May 31, 2021

erlend-aasland commented May 31, 2021

Copy link

Copy Markdown

Contributor Author

cc. @vstinner

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland mentioned this pull request

May 31, 2021

bpo-42213: Remove redundant cyclic GC hack in sqlite3 #26462

Closed

pablogsal requested changes

May 31, 2021

View reviewed changes

Comment thread

Modules/_sqlite/cursor.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting review label

May 31, 2021

bedevere-bot commented May 31, 2021

Copy link

Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label

May 31, 2021

Erlend E. Aasland added 2 commits

June 1, 2021 09:26


          Allocate and track statement objects in pysqlite_statement_create

1b0f583

By allocating and tracking creation of statement object in
pysqlite_statement_create(), the caller does not need to worry about GC
syncronization, and eliminates the possibility of getting a badly
created object. All related fault handling is moved to
pysqlite_statement_create().


          Remove now unused constants

0a6ca75

vstinner approved these changes

Jun 1, 2021

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM, but I would like to see the minor issues being addressed first before merging your change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland reacted with hooray emoji

All reactions

🎉 1 reaction

Comment thread

Modules/_sqlite/statement.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/_sqlite/statement.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting merge and removed awaiting changes labels

Jun 1, 2021

erlend-aasland force-pushed the sqlite-track-stmt branch from ee00a0d to 6b1ea0c Compare

June 1, 2021 10:08

Erlend Egeberg Aasland and others added 2 commits

June 1, 2021 12:13


          Improve error message

df284eb

Co-authored-by: Victor Stinner <[email protected]>


          Group and order member initialisation

4f03b79

erlend-aasland force-pushed the sqlite-track-stmt branch from 6b1ea0c to 4f03b79 Compare

June 1, 2021 10:15

vstinner approved these changes

Jun 1, 2021

View reviewed changes

vstinner left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Modules/_sqlite/statement.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

vstinner merged commit fffa0f9 into python:main

Jun 1, 2021

bedevere-bot removed the awaiting merge label

Jun 1, 2021

vstinner commented Jun 1, 2021

Copy link

Copy Markdown

Member

Merged, thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland deleted the sqlite-track-stmt branch

June 1, 2021 10:51

erlend-aasland commented Jun 1, 2021

Copy link

Copy Markdown

Contributor Author

Merged, thanks.

Thanks for reviewing, Victor & Pablo!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request

Jun 2, 2021


          pysqlite_statement_create now returns a Py object, not an int

448ad9d

pythonGH-26206 was broken by pythonGH-26475.

erlend-aasland mentioned this pull request

Jun 2, 2021

bpo-44165: pysqlite_statement_create() returns a PyObject *, not an int #26484

Merged

pablogsal pushed a commit that referenced this pull request

Jun 2, 2021


          bpo-44165: pysqlite_statement_create now returns a Py object, not an …

fbf25b8

…int (GH-26484)

GH-26206 was broken by GH-26475.

Mariatta added needs backport to 3.10 and removed needs backport to 3.10 labels

Jun 2, 2021

miss-islington commented Jun 2, 2021

Copy link

Copy Markdown

Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington commented Jun 2, 2021

Copy link

Copy Markdown

Contributor

Sorry, @erlend-aasland and @vstinner, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker fffa0f92adaaed0bcb3907d982506f78925e9052 3.10

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

miss-islington assigned vstinner

Jun 2, 2021

vstinner commented Jun 2, 2021

Copy link

Copy Markdown

Member

@erlend-aasland: Automated backport to 3.10 failed, can you try to backport the change manually?

@pablogsal: I'm not sure if this change is really a bugfix or an enhancement. I'm ok to backport it even after beta2 release.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland commented Jun 3, 2021

Copy link

Copy Markdown

Contributor Author

@erlend-aasland: Automated backport to 3.10 failed, can you try to backport the change manually?

Will do!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request

Jun 3, 2021


          [3.10] bpo-42972: Track sqlite3 statement objects (pythonGH-26475)

93f9a45

Allocate and track statement objects in pysqlite_statement_create.

By allocating and tracking creation of statement object in
pysqlite_statement_create(), the caller does not need to worry about GC
syncronization, and eliminates the possibility of getting a badly
created object. All related fault handling is moved to
pysqlite_statement_create().

Co-authored-by: Victor Stinner <[email protected]>.
(cherry picked from commit fffa0f9)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

bedevere-bot removed the needs backport to 3.10 label

Jun 3, 2021

bedevere-bot commented Jun 3, 2021

Copy link

Copy Markdown

GH-26515 is a backport of this pull request to the 3.10 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vstinner pushed a commit that referenced this pull request

Jun 3, 2021


          [3.10] bpo-42972: Track sqlite3 statement objects (GH-26475) (GH-26515)

84d80f5

Allocate and track statement objects in pysqlite_statement_create.

By allocating and tracking creation of statement object in
pysqlite_statement_create(), the caller does not need to worry about GC
syncronization, and eliminates the possibility of getting a badly
created object. All related fault handling is moved to
pysqlite_statement_create().

Co-authored-by: Victor Stinner <[email protected]>.
(cherry picked from commit fffa0f9)

Co-authored-by: Erlend Egeberg Aasland <[email protected]>

pablogsal mentioned this pull request

Jul 18, 2021

[3.10] Correct the order of regen-abidump #27228

Closed

vstinner mentioned this pull request

Jun 9, 2022

[C API] Heap types (PyType_FromSpec) must fully implement the GC protocol #87138

Closed

Reviewers

pablogsal

pablogsal requested changes

vstinner

vstinner approved these changes

berkerpeksag

Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

vstinner

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

erlend-aasland commented May 31, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erlend-aasland commented May 31, 2021

Uh oh!

Uh oh!

bedevere-bot commented May 31, 2021

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

vstinner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vstinner commented Jun 1, 2021

Uh oh!

erlend-aasland commented Jun 1, 2021

Uh oh!

miss-islington commented Jun 2, 2021

Uh oh!

miss-islington commented Jun 2, 2021

Uh oh!

vstinner commented Jun 2, 2021

Uh oh!

erlend-aasland commented Jun 3, 2021

Uh oh!

bedevere-bot commented Jun 3, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

erlend-aasland commented May 31, 2021 •

edited by bedevere-bot

Loading

Uh oh!

There was an error while loading. Please reload this page.