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

Attempt to convert pcli's ORM to use rusqlite instead of sqlx #2372

Merged
merged 34 commits into from
Apr 19, 2023

Conversation

plaidfinch
Copy link
Collaborator

@plaidfinch plaidfinch commented Apr 14, 2023

  • Rip out sqlx completely by commenting out anything that uses it
  • Replace the connection pool with an Arc<Mutex<rusqlite::Connection>> (this is temporary)
  • Rewrite all the queries using rusqlite
  • Test pcli, pclientd, and galileo (things might be really slow because of the global mutex, especially galileo)
  • Reintroduce connection pooling by replacing the Arc<Mutex<...>> with an r2d2::Pool<r2d2_sqlite::SqliteConnectionManager>, so we can (hopefully) read while writing, etc.
  • Test everything again
  • Might need to re-enable table-level locking using the "shared cache" like we did previously even though rusqlite (and the sqlite docs) warn against it? Consider this carefully...

@plaidfinch plaidfinch self-assigned this Apr 17, 2023
This allows the implementor of AsyncRead/AsyncWrite/Read/Write to choose what iterator or stream is
returned, which is useful when using rusqlite, because rusqlite's iterators are `!Send`, so we need to
customize that bound when returning a `dyn Iterator` (i.e. it can't be hardcoded to `Send` in the
trait definition).
We will incrementally fill in the converted functionality
@plaidfinch plaidfinch marked this pull request as ready for review April 18, 2023 20:48
@@ -0,0 +1,119 @@
-- The hash of this schema file
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to live inline with the Rust code? Could we keep it in view/schema/view.sql (rather than "migrations")?

@hdevalence
Copy link
Member

 Apr 19 02:23:05.889  INFO view worker error e=near "RETURNING": syntax error

Caused by:
    Error code 1: SQL error or missing database

@plaidfinch plaidfinch temporarily deployed to smoke-test April 19, 2023 18:07 — with GitHub Actions Inactive
@hdevalence
Copy link
Member

 Apr 19 02:23:05.889  INFO view worker error e=near "RETURNING": syntax error

Caused by:
    Error code 1: SQL error or missing database

For posterity: this was caused by not using the bundled feature for bringing our own sqlite, causing us to use whatever sqlite was available on the host system, and the RETURNING feature was added only two years ago, so the build worker had an outdated version.

@hdevalence hdevalence merged commit 52dd0ad into main Apr 19, 2023
6 checks passed
@hdevalence hdevalence deleted the pcli-rusqlite branch April 19, 2023 19:21
conorsch pushed a commit to penumbra-zone/galileo that referenced this pull request Apr 27, 2023
Uses the new storage init logic [0] to store sync state in memory.
This will cause galileo to sync at service start to memory, and abjure
any on-disk storage.

Hat tip to @plaidfinch for explaining the type inference required by the
view service changes.

[0] penumbra-zone/penumbra#2372
conorsch pushed a commit to penumbra-zone/galileo that referenced this pull request Apr 27, 2023
Uses the new storage init logic [0] to store sync state in memory.
This will cause galileo to sync at service start to memory, and abjure
any on-disk storage.

Hat tip to @plaidfinch for explaining the type inference required by the
view service changes.

[0] penumbra-zone/penumbra#2372
plaidfinch pushed a commit to penumbra-zone/galileo that referenced this pull request Apr 27, 2023
Uses the new storage init logic [0] to store sync state in memory.
This will cause galileo to sync at service start to memory, and abjure
any on-disk storage.

Hat tip to @plaidfinch for explaining the type inference required by the
view service changes.

[0] penumbra-zone/penumbra#2372
conorsch pushed a commit that referenced this pull request Jan 18, 2024
This change started as a simple addition to the dev docs, adding a handy
sqlite3 json-aware blob-decoding example, suggested by @TalDerei. While
mucking around in the docs, I discovered some old `sqlx` info that
AFAICT is dead documentation, given #2372.
conorsch pushed a commit that referenced this pull request Jan 18, 2024
This change started as a simple addition to the dev docs, adding a handy
sqlite3 json-aware blob-decoding example, suggested by @TalDerei. While
mucking around in the docs, I discovered some old `sqlx` info that
AFAICT is dead documentation, given #2372.
aubrika pushed a commit that referenced this pull request Jan 19, 2024
This change started as a simple addition to the dev docs, adding a handy
sqlite3 json-aware blob-decoding example, suggested by @TalDerei. While
mucking around in the docs, I discovered some old `sqlx` info that
AFAICT is dead documentation, given #2372.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Testnet 51: Elara
Development

Successfully merging this pull request may close these issues.

None yet

2 participants