GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-93421: Fix sqlite3 cursor `.rowcount` for UPDATE ... RETURNING queries by erlend-aasland · Pull Request #93520 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Jun 5, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

Resolves #93421

Sorry, something went wrong.

Uh oh!

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

All reactions
erlend-aasland added 3 commits June 5, 2022 22:11
@erlend-aasland
Try lazy rowcount approach
5436db7
Does not work bca. executemany, but it solves the repro
@erlend-aasland
Calculate rowcount using sqlite3_total_changes() diff
84891c6
@erlend-aasland
Add NEWS
a5fca9d
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner June 5, 2022 20:46
@bedevere-bot bedevere-bot added the awaiting core review label Jun 5, 2022
@erlend-aasland erlend-aasland linked an issue Jun 5, 2022 that may be closed by this pull request
SQLite rowcount is corrupted when combining UPDATE RETURNING w/ table that is dropped and recreated #93421
Closed
@erlend-aasland
Fix return value
5819902
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@bedevere-bot

bedevere-bot commented Jun 5, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 5819902 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka June 5, 2022 21:27
@erlend-aasland erlend-aasland self-assigned this Jun 5, 2022
@erlend-aasland erlend-aasland added needs backport to 3.10 needs backport to 3.11 only security fixes labels Jun 5, 2022
@erlend-aasland
Add tests
5165a3a
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@bedevere-bot

bedevere-bot commented Jun 5, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 5165a3a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@erlend-aasland
Remove spurious change
1da6390
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@bedevere-bot

bedevere-bot commented Jun 5, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @erlend-aasland for commit 1da6390 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 5, 2022
@erlend-aasland erlend-aasland mentioned this pull request Jun 6, 2022
gh-93421: Update sqlite3 cursor.rowcount after SQLITE_DONE #93526
Merged
@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

FYI, the failing buildbot are also failing on main; they are unrelated to this PR.

All reactions

Sorry, something went wrong.

Uh oh!

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

This was referenced Jun 6, 2022
SQLite rowcount is corrupted when combining UPDATE RETURNING w/ table that is dropped and recreated #93421
Closed
gh-79579: Update sqlite3.Cursor.rowcount for all data-modifying queries #93532
Closed
@ghost

ghost commented Jun 6, 2022

Copy link
Copy Markdown

Haven't checked this method for problems.
Please consider using sqlite3_total_changes64(), I'm afraid sqlite3_total_changes() will be easier to get wrong result.

total_changes doc:

The two functions are identical except for the type of the return value and that if the number of rows modified by the connection exceeds the maximum value supported by type "int", then the return value of sqlite3_total_changes() is undefined.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

Haven't checked this method for problems.

Please do; I appreciate your review.

Please consider using sqlite3_total_changes64(), I'm afraid sqlite3_total_changes() will be easier to get wrong result.

Yes, I will make sure the 64-bit version is used, when available. Thanks for the heads-up!

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

FTR, I will revert the DML check change in this PR; that is cleaner.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
Merge branch 'main' into sqlite-changes
0df7652
erlend-aasland added 2 commits June 6, 2022 22:08
@erlend-aasland
Revert sqlite3_stmt_readonly change
c4f6926
@erlend-aasland
Use sqlite3_total_changes64 if available
20397c5
@ghost

ghost commented Jun 7, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

Just found sqlite3_total_changes64() is a very new function, added in 3.37.0 (2021-11-27)

If change one row per second, it will overflow after 68 years:

>>> (2**31-1)/3600/24/365
68.09625973490614

Not afraid of you laughing, I hope a program can run 1000 years (or forever) without errors, in theory we can achieve this goal.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 7, 2022

Copy link
Copy Markdown
Contributor Author

Just found sqlite3_total_changes64() is a very new function, added in 3.37.0 (2021-11-27)

Yes, both of the 64-bit changes APIs were added in 3.37.0.

Not afraid of you laughing, I hope a program can run 1000 years (or forever) without errors, in theory we can achieve this goal.

Absolutely. But I also like to keep in mind that perfect is the enemy of good. IMO, a some of the sqlite3 issues have fallen to this; the proposed solution is not perfect, so we're stuck with API pains and bugs. A pragmatic view may, in some cases, be beneficial.

For the record: for now, I think my alternative PR is the better solution for this issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland added the DO-NOT-MERGE label Jun 7, 2022
@erlend-aasland

erlend-aasland commented Jun 7, 2022

Copy link
Copy Markdown
Contributor Author

Closing in favour of gh-93526

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland closed this Jun 7, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-changes branch June 7, 2022 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag
@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

Uh oh!

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

Assignees

@erlend-aasland erlend-aasland

Labels

awaiting core review DO-NOT-MERGE needs backport to 3.11 only security fixes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

SQLite rowcount is corrupted when combining UPDATE RETURNING w/ table that is dropped and recreated

Uh oh!

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

2 participants

@erlend-aasland @bedevere-bot
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.