Skip to content

Add Sqlite test for read/write row counters for large rows #2718

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

Merged
merged 1 commit into from
Sep 17, 2024

Conversation

shrima-cf
Copy link
Contributor

No description provided.

@shrima-cf shrima-cf requested a review from justin-mp September 16, 2024 23:03
@shrima-cf shrima-cf requested review from a team as code owners September 16, 2024 23:03

db.run("CREATE TABLE large_things (id INTEGER PRIMARY KEY, large_value TEXT)");

// SQLite's default page size is 4096 bytes
Copy link
Member

Choose a reason for hiding this comment

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

Can you add an assertion for the default page size? It seems pragma_page_size() is available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added!

@@ -663,6 +663,36 @@ KJ_TEST("SQLite write row counters (basic)") {
}
}

KJ_TEST("SQLite read/write row counters (large row insert)") {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to comment why we have this test (either in the commit message or with a comment here).

I believe it's to make sure that extra page reads aren't included in the row count.

Does it also make sense to have more than one row for this test? I don't know how big rows are actually stored in SQLite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Big rows are stored in what is called overflow pages - https://www.sqlite.org/fileformat2.html#ovflpgs

I don't think having multiple rows in the test is going to give us any new information.

Will add the comment

@shrima-cf shrima-cf force-pushed the shrima/add-sqlite_test_large_readwrite branch from 6741446 to e8559d7 Compare September 17, 2024 16:16
@shrima-cf shrima-cf force-pushed the shrima/add-sqlite_test_large_readwrite branch from e8559d7 to a33e1a8 Compare September 17, 2024 16:22
@shrima-cf shrima-cf merged commit 563d72f into main Sep 17, 2024
12 of 13 checks passed
@shrima-cf shrima-cf deleted the shrima/add-sqlite_test_large_readwrite branch September 17, 2024 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants