Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented Nov 2, 2021

@erlend-aaslanderlend-aaslandforce-pushed the sqlite-use-limits-in-tests branch from 07d012a to 8cdb980CompareNovember 2, 2021 09:34
@erlend-aaslanderlend-aasland changed the title bpo-45243: Use sqlite3 connection limit context manager in test suitebpo-45243: Use connection limits to simplify sqlite3 testsNov 2, 2021
@erlend-aaslanderlend-aasland marked this pull request as ready for review November 2, 2021 09:58
@erlend-aaslanderlend-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-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 2, 2021
Copy link
Member

@serhiy-storchakaserhiy-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
ContributorAuthor

erlend-aasland commented Nov 2, 2021

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:

size_tsql_len=strlen(sql_script);
intmax_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");
returnNULL;
}

execute / create statement check:

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

@erlend-aasland
Copy link
ContributorAuthor

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

Copy link
Member

@serhiy-storchakaserhiy-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.

Copy link
Member

@serhiy-storchakaserhiy-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
ContributorAuthor

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
ContributorAuthor

erlend-aasland commented Nov 4, 2021

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

@erlend-aasland
Copy link
ContributorAuthor

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

build-in round is missing if SQLITE_OMIT_FLOATING_POINT is defined
@serhiy-storchakaserhiy-storchaka merged commit 3d42cd9 into python:mainNov 5, 2021
@erlend-aasland
Copy link
ContributorAuthor

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

@erlend-aaslanderlend-aasland deleted the sqlite-use-limits-in-tests branch November 5, 2021 17:27
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip newstestsTests in the Lib/test dir

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland@bedevere-bot@serhiy-storchaka@the-knights-who-say-ni