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

gh-79579: Update sqlite3.Cursor.rowcount for all data-modifying queries #93532

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jun 6, 2022

Resolves #79579

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

@animalize, would you like to review this?

@erlend-aasland erlend-aasland added needs backport to 3.10 needs backport to 3.11 🔨 test-with-buildbots labels Jun 6, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 6, 2022

🤖 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 "🔨 test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots label Jun 6, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

cc. @zzzeek

@animalize
Copy link
Contributor

@animalize animalize commented Jun 6, 2022

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)

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

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.

@animalize
Copy link
Contributor

@animalize animalize commented Jun 6, 2022

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.

@animalize
Copy link
Contributor

@animalize animalize commented Jun 6, 2022

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.

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

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.

@erlend-aasland erlend-aasland marked this pull request as draft Jun 6, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jun 6, 2022

On hold till #93421 is resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants