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-79579: Update `sqlite3.Cursor.rowcount` for all data-modifying queries by erlend-aasland · Pull Request #93532 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

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

Copy link
Copy Markdown
Contributor

Resolves #79579

Sorry, something went wrong.

Uh oh!

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

All reactions
@erlend-aasland
pythongh-79579: Update sqlite3.Cursor.rowcount for all datamodifying …
e82adb8
…queries
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner June 6, 2022 11:02
@bedevere-bot bedevere-bot added the awaiting core review label Jun 6, 2022
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka June 6, 2022 11:02
@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

@animalize, would you like to review this?

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland erlend-aasland added needs backport to 3.10 needs backport to 3.11 only security fixes 🔨 test-with-buildbots Test PR w/ buildbots; report in status section labels Jun 6, 2022
@bedevere-bot

bedevere-bot commented Jun 6, 2022

Copy link
Copy Markdown

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

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 6, 2022
@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

cc. @zzzeek

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Jun 6, 2022 •
edited by ghost
Loading

Uh oh!

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

Copy link
Copy Markdown

Before this change, this code prints-1.
After this change, it prints 1, it seems wrong.
IMO #10913 's method is clear, it skips the leading comments when detecting dml statement.

import sqlite3
cursor = sqlite3.connect(":memory:", isolation_level=None).cursor()

cursor.execute("""CREATE TABLE foo (id INTEGER)""")

values = ((1,), (2,), (3,))
cursor.executemany("""INSERT INTO foo (id) VALUES (?)""", values)

cursor.execute("vacuum")
print(cursor.rowcount)
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 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

After this change, it prints 1, it seems wrong.

It seems wrong is a bit vague. Would you mind to elaborate?

Actually, this will be fixed by #93520 (which currently also encompasses this change), so it does not matter. IMO, trying to parse comments is fragile approach, and I would not recommend such a solution.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Jun 6, 2022

Copy link
Copy Markdown

It seems wrong is a bit vague. Would you mind to elaborate?

In .rowcount doc:

As required by the Python DB API Spec, the rowcount attribute “is -1 in case no executeXX() has been performed on the cursor or the rowcount of the last operation is not determinable by the interface”.

IMHO, using sqlite3_stmt_readonly() to detect statement type is unreliable. It return false when the database may be modified, this may include some irrelevant statements, then give a suprise in some cases, for example:
https://bugs.python.org/issue28518
So I suggest not to use sqlite3_stmt_readonly() to detect statement type. I saw you used it in 878e726, but it's only an error message, not involving precise data.

All reactions

Sorry, something went wrong.

Uh oh!

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

@ghost

ghost commented Jun 6, 2022

Copy link
Copy Markdown

IMO, trying to parse comments is fragile approach, and I would not recommend such a solution.

Yes, but IMO this is the most reliable way.
If just skip the leading comments and whitespaces, it shouldn't be very complicated.

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

It return false when the database may be modified, this may include some irrelevant statements, then give a suprise in some cases, for example:
https://bugs.python.org/issue28518

I don't think that is a valid comparison. We're not talking about transaction control here; just setting .rowcount or not. IMO, this is the best way. I don't see any problems with DB API here.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland
erlend-aasland marked this pull request as draft June 6, 2022 21:57
@erlend-aasland

erlend-aasland commented Jun 6, 2022

Copy link
Copy Markdown
Contributor Author

On hold till #93421 is resolved.

All reactions

Sorry, something went wrong.

Uh oh!

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

@erlend-aasland

erlend-aasland commented Jun 8, 2022

Copy link
Copy Markdown
Contributor Author

Closing in favour of #93623

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 8, 2022
@erlend-aasland
erlend-aasland deleted the sqlite-rowcount/gh-79579 branch June 8, 2022 23:50
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

No one assigned

Labels

awaiting core review 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 incorrect row count for UPDATE

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.