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

Closed

Conversation

erlend-aasland
Copy link
Contributor

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

Resolves #79579

@erlend-aasland
Copy link
Contributor Author

@animalize, would you like to review this?

@erlend-aasland erlend-aasland added needs backport to 3.10 only security fixes 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
Copy link

🤖 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.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jun 6, 2022
@erlend-aasland
Copy link
Contributor Author

cc. @zzzeek

@ghost
Copy link

ghost 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 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.

@ghost
Copy link

ghost 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.

@ghost
Copy link

ghost 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

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 June 6, 2022 21:57
@erlend-aasland
Copy link
Contributor Author

On hold till #93421 is resolved.

@erlend-aasland
Copy link
Contributor Author

Closing in favour of #93623

@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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQLite incorrect row count for UPDATE
2 participants