Skip to content

bpo-45243: Use connection limits to simplify sqlite3 tests #29356

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 13 commits into from
Nov 5, 2021
Merged

bpo-45243: Use connection limits to simplify sqlite3 tests #29356

merged 13 commits into from
Nov 5, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Nov 2, 2021 •
edited by bedevere-bot
Loading

@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 2, 2021
@erlend-aasland erlend-aasland mentioned this pull request Nov 2, 2021
Merged
@erlend-aasland erlend-aasland force-pushed the sqlite-use-limits-in-tests branch from 13a40b0 to 07d012a Compare November 2, 2021 09:09
@erlend-aasland erlend-aasland force-pushed the sqlite-use-limits-in-tests branch from 07d012a to 8cdb980 Compare November 2, 2021 09:34
@erlend-aasland erlend-aasland changed the title bpo-45243: Use sqlite3 connection limit context manager in test suite bpo-45243: Use connection limits to simplify sqlite3 tests Nov 2, 2021
@erlend-aasland erlend-aasland marked this pull request as ready for review November 2, 2021 09:58
@erlend-aasland erlend-aasland added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2021
@bedevere-bot
Copy link

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

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 Nov 2, 2021
serhiy-storchaka
serhiy-storchaka reviewed Nov 2, 2021
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Bigmem tests rewriting does not look correct to me. These tests are specially purposed to test the integer overflow. They try with scripts of specific length (slightly below or above INT_MAX). It does not make sense to test scripts with smaller length. They are decorated with bigmemtest because they require a large amount of RAM and should be skipped if it is not enough.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Nov 2, 2021 •
edited
Loading

Bigmem tests rewriting does not look correct to me. These tests are specially purposed to test the integer overflow. [...]

The C code explicitly checks the current connection limit (SQLITE_LIMIT_LENGTH); it does not check against INT_MAX. The maximum limit is defined by the compile-time constant SQLITE_MAX_LENGTH, which defaults to 1_000_000_000 (and can be maximum 2**31-1). This limit can be altered using sqlite3_setlimit(db, SQLITE_LIMIT_LENGTH, new_limit) on the database connection. Using bigmem tests we of course ensure that we will hit the roof, but only if the test machine has enough RAM. Using custom connection limits in the test, we can lower the limit temporarily and ensure that we can run these tests on any machine.

executemany check:

cpython/Modules/_sqlite/cursor.c

Lines 730 to 737 in 77a1f8d

size_t sql_len = strlen(sql_script);
int max_length = sqlite3_limit(self->connection->db,
SQLITE_LIMIT_LENGTH, -1);
if (sql_len >= (unsigned)max_length) {
PyErr_SetString(self->connection->DataError,
"query string is too large");
return NULL;
}

execute / create statement check:

cpython/Modules/_sqlite/statement.c

Lines 62 to 68 in 77a1f8d

sqlite3 *db = connection->db;
int max_length = sqlite3_limit(db, SQLITE_LIMIT_LENGTH, -1);
if (size >= max_length) {
PyErr_SetString(connection->DataError,
"query string is too large");
return NULL;
}

@erlend-aasland
Copy link
Contributor Author

@serhiy-storchaka, let me know if you want me to revert the bigmem tests. I believe it is correct to change them, though (see my previous comment).

serhiy-storchaka
serhiy-storchaka reviewed Nov 4, 2021
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Okay, let change these tests.

serhiy-storchaka
serhiy-storchaka approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM!

@erlend-aasland
Copy link
Contributor Author

Thanks for reviewing, @serhiy-storchaka. What do you think of my proposed patch in #29356 (comment)? I guess it is out of scope for this PR, but I think we should apply it.

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Nov 4, 2021 •
edited
Loading

FYI, I'll merge in main, in order to rerun the CI. (asyncio failed again)

@erlend-aasland
Copy link
Contributor Author

CI is ok; ready for merge :) I'll create an issue regarding the length checks.

serhiy-storchaka
serhiy-storchaka reviewed Nov 5, 2021
View reviewed changes
Use abs iso. round
032bfd5 
build-in round is missing if SQLITE_OMIT_FLOATING_POINT is defined
@serhiy-storchaka serhiy-storchaka merged commit 3d42cd9 into python:main Nov 5, 2021
@erlend-aasland
Copy link
Contributor Author

Thank you for reviewing, Serhiy. The PR is in a better shape now.

@erlend-aasland erlend-aasland deleted the sqlite-use-limits-in-tests branch November 5, 2021 17:27
@erlend-aasland erlend-aasland mentioned this pull request May 16, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@erlend-aasland @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni