GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45243: Use connection limits to simplify `sqlite3` tests by erlend-aasland · Pull Request #29356 · python/cpython

Conversation

@erlend-aasland

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

https://bugs.python.org/issue45243

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 2, 2021
@bedevere-bot bedevere-bot added tests Tests in the Lib/test dir awaiting review labels Nov 2, 2021
@erlend-aasland erlend-aasland added the skip news label Nov 2, 2021
@erlend-aasland erlend-aasland mentioned this pull request Nov 2, 2021
bpo-45138: Expand traced SQL statements in sqlite3 trace callback #28240
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 E. Aasland
bpo-45243: Use sqlite3 connection limit context manager in test suite
8cdb980
@erlend-aasland
erlend-aasland force-pushed the sqlite-use-limits-in-tests branch from 07d012a to 8cdb980 Compare November 2, 2021 09:34
Erlend E. Aasland
Add test for too many SQL variables
d68d6d8
@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 E. Aasland
Adjust comment
22adb06
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka November 2, 2021 09:57
@erlend-aasland
erlend-aasland marked this pull request as ready for review November 2, 2021 09:58
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_sqlite3/test_userfunctions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@erlend-aasland

erlend-aasland commented Nov 2, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

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;
}

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Address review
cb56249
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka November 2, 2021 12:01
@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 4, 2021
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Address review: test lim-1 and lim
3a75c2d
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

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_sqlite3/test_userfunctions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting review labels Nov 4, 2021
Erlend E. Aasland
Address review: make it clear which function is failing
a6da634
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 4, 2021
View reviewed changes
@erlend-aasland

erlend-aasland commented Nov 4, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Erlend E. Aasland
Merge remote-tracking branch 'upstream/main' into sqlite-use-limits-i…
3f1ee2a 
…n-tests
@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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka reviewed Nov 5, 2021
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_userfunctions.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Erlend Egeberg Aasland and others added 4 commits November 5, 2021 13:55
Erlend Egeberg Aasland @serhiy-storchaka
Update Lib/test/test_sqlite3/test_dbapi.py
b98d623 
Co-authored-by: Serhiy Storchaka <[email protected]>
Erlend E. Aasland
Simplify test_func_too_many_args further
9521fc1
Erlend E. Aasland
Add single-argument test
c53ffd7
Erlend E. Aasland
Simplify sql statements in test_execute_too_many_params
6197b3a
@erlend-aasland
erlend-aasland requested a review from serhiy-storchaka November 5, 2021 13:09
Erlend E. Aasland
Use abs iso. round
032bfd5 
build-in round is missing if SQLITE_OMIT_FLOATING_POINT is defined
serhiy-storchaka
serhiy-storchaka reviewed Nov 5, 2021
View reviewed changes
Comment thread Lib/test/test_sqlite3/test_dbapi.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
serhiy-storchaka
serhiy-storchaka approved these changes Nov 5, 2021
View reviewed changes
Erlend E. Aasland
Make sure we test against correct limit in test_execute_too_many_params
c257c65
@serhiy-storchaka
serhiy-storchaka merged commit 3d42cd9 into python:main Nov 5, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label 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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@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
[sqlite3] add support for changing connection limits #89406
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

Uh oh!

There was an error while loading. Please reload this page.

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.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@erlend-aasland @bedevere-bot @serhiy-storchaka @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.