Skip to content
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

make page size configurable in SQLiteDatastore.new #25

Closed
wants to merge 1 commit into from

Conversation

michaelsbradleyjr
Copy link
Contributor

Also make cache size and journal mode configurable.

Closes #6.
Related to codex-storage/nim-codex#234.

also make cache size and journal mode configurable

closes #6
related to codex-storage/nim-codex#234
@Bulat-Ziganshin
Copy link
Contributor

Looks great for me

Copy link

@emizzle emizzle left a comment

Choose a reason for hiding this comment

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

LGTM, nice work! I just had one question but likely isn't related to this PR.


if (let x = sqlite3_step(s2); x != SQLITE_ROW):
s2.dispose
return failure $sqlite3_errstr(x)
Copy link

Choose a reason for hiding this comment

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

Probably not relevant to this PR, but how does a Result[T, string] equate to the return type of RawStmtPtr?

Copy link
Contributor Author

@michaelsbradleyjr michaelsbradleyjr Sep 5, 2022

Choose a reason for hiding this comment

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

It's a template, so gets expanded into the body of proc new*(T: type SQLiteDatastore, ...): ?!T, and in the case of pageSizePragmaStmt(...) the returned RawStmtPtr is assigned to let pageSizePragmaStmt = ..., for which that type is the desired one.

@Bulat-Ziganshin
Copy link
Contributor

From Michael's message:

There are some other pragmas (possibly some compile-time options) we may want to consider re: improving nim-codex's performance. I'm no expert, just becoming more aware of what's possible after spending more time with SQLite's docs and following links therein.

https://www.sqlite.org/pragma.html#pragma_wal_autocheckpoint
https://www.sqlite.org/wal.html#ckpt
https://www.sqlite.org/compile.html#default_wal_autocheckpoint

https://www.sqlite.org/pragma.html#pragma_mmap_size
https://www.sqlite.org/mmap.html

It's pretty straightforward to add support for other pragmas; doesn't necessarily have to be in a PR, could just be on a branch in status-im/nim-datastore for sake of experimentation.

@Bulat-Ziganshin
Copy link
Contributor

Bulat-Ziganshin commented Sep 15, 2022

I think that it should have different logic - if sqlite store already exists and has different pagesize (and other parameters), it should continue to work. Ideally, it should print a notice about that, but don't stop the work.

For a starter, we may just comment out code that checks parameters after setting them.

Current behavior means that unlucky user will have to guess DB parameters before he can use this DB :)

@dryajov dryajov closed this Nov 18, 2022
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.

Make page size configurable in SQLiteDatastore.new
4 participants