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

support more options for SQLite connection #1498

Merged
merged 3 commits into from
Mar 11, 2025
Merged

Conversation

paulfitz
Copy link
Member

@paulfitz paulfitz commented Mar 5, 2025

The basic connection parameters used to open documents haven't been touched in a long time. This adds support for WAL mode, or default mode with SYNCHRONOUS=full.

Use GRIST_SQLITE_MODE=wal or GRIST_SQLITE_MODE=sync for this new behavior. Default is unchanged.

WAL mode is not recommended for Desktop, or similar situations where you may habitually interact with .grist files managed by the Grist application on the file system. If all interaction is via the API, it should be a fine mode to use.

The sync mode is a candidate for use on the Desktop.

The basic connection parameters used to open documents hasn't
been touched in a long time. This adds support for WAL mode,
or default mode with SYNCHRONOUS=full.

Use `GRIST_SQLITE_MODE=wal` or `GRIST_SQLITE_MODE=sync` for this
new behavior. Default is unchanged.

WAL mode is not recommended for Desktop, or similar situations
where you may habitually interact with .grist files managed
by the Grist application on the file system. If all interaction
is via the API, it should be a fine mode to use.

The sync mode is a candidate for use on the Desktop.
@paulfitz paulfitz marked this pull request as draft March 5, 2025 01:34
@paulfitz paulfitz marked this pull request as ready for review March 5, 2025 22:10
@georgegevoian georgegevoian self-requested a review March 10, 2025 19:40
Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Changes look ok to me. Tested all modes work.

Too early to mention it in the README?

@paulfitz
Copy link
Member Author

Too early to mention it in the README?

@georgegevoian I've added a brief README entry. Would like to hold off pushing people to this until we've got some more experience with it (e.g. use in our SaaS staging).

Copy link
Contributor

@georgegevoian georgegevoian left a comment

Choose a reason for hiding this comment

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

Thanks @paulfitz.

@paulfitz paulfitz merged commit 9fed38e into main Mar 11, 2025
12 checks passed
@paulfitz paulfitz deleted the paulfitz/sqlite-sync-wal branch March 11, 2025 13:08
Copy link
Member

@dsagal dsagal left a comment

Choose a reason for hiding this comment

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

Thanks Paul! I reviewed too, found a couple of minor notes.

@@ -72,6 +72,11 @@ export class AppSettings {
} else if (query.defaultValue !== undefined) {
this._value = query.defaultValue;
}
if (query.acceptedValues && this._value) {
if (query.acceptedValues.every(v => v !== this._value)) {
Copy link
Member

Choose a reason for hiding this comment

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

Would be clearer: !query.acceptedValues.includes(this._value)

@@ -303,6 +303,7 @@ Grist can be configured in many ways. Here are the main environment variables it
| GRIST_SESSION_DOMAIN | if set, associates the cookie with the given domain - otherwise defaults to GRIST_DOMAIN |
| GRIST_SESSION_SECRET | a key used to encode sessions |
| GRIST_SKIP_BUNDLED_WIDGETS | if set, Grist will ignore any bundled widgets included via NPM packages. |
| GRIST_SQLITE_MODE | if set to `wal`, use SQLite in [WAL mode](https://www.sqlite.org/wal.html), if set to `sync`, use SQLite with [SYNCHRONOUS=full](https://www.sqlite.org/pragma.html#pragma_synchronous)
Copy link
Member

Choose a reason for hiding this comment

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

Should we say what the default mode is?


/**
* Make a backup of the given document using either an already
* open connection, or a fresh one if to existing connection is
Copy link
Member

Choose a reason for hiding this comment

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

"if to" -> "if no"?

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