◐ Shell
reader mode source ↗
Skip to content

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

Merged
serhiy-storchaka merged 13 commits into
python:mainfrom
erlend-aasland:sqlite-use-limits-in-tests
Nov 5, 2021
Merged

bpo-45243: Use connection limits to simplify sqlite3 tests#29356
serhiy-storchaka merged 13 commits into
python:mainfrom
erlend-aasland:sqlite-use-limits-in-tests

Conversation

@erlend-aasland

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

Copy link
Copy Markdown
Contributor

@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 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

bedevere-bot commented Nov 2, 2021

Copy link
Copy Markdown

🤖 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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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

erlend-aasland commented Nov 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

erlend-aasland commented Nov 2, 2021

Copy link
Copy Markdown
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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

Okay, let change these tests.

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

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM!

@erlend-aasland

erlend-aasland commented Nov 4, 2021

Copy link
Copy Markdown
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.

20 hidden items Load more…
@erlend-aasland

erlend-aasland commented Nov 4, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

@erlend-aasland

erlend-aasland commented Nov 5, 2021

Copy link
Copy Markdown
Contributor Author

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

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

erlend-aasland commented Nov 5, 2021

Copy link
Copy Markdown
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

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

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