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

remove embedded-db feature for selecting sqlite/postgres #2369

Open
imabdulbasit opened this issue Dec 6, 2024 · 1 comment
Open

remove embedded-db feature for selecting sqlite/postgres #2369

imabdulbasit opened this issue Dec 6, 2024 · 1 comment

Comments

@imabdulbasit
Copy link
Contributor

imabdulbasit commented Dec 6, 2024

We merged the SQLite PR into hotshot-query-service, which enables either PostgreSQL or SQLite code based on the embedded-db feature. If this feature is enabled, SQLite code is activated; otherwise, PostgreSQL code is used. We didn’t anticipate how messy updating the sequencer would become. Features in cargo should be additive. This is because cargo takes union of features, so a feature should not disable a functionality and activate other. What we have done is opposite of what cargo expects.

in docs

When a dependency is used by multiple packages, Cargo will use the union of all features enabled on that dependency when building it. This helps ensure that only a single copy of the dependency is used. https://doc.rust-lang.org/cargo/reference/features.html?highlight=additive#feature-unification

In order to get different sequencer-postgres and sequencer-sqlite binaries:
When the sequencer code was updated, we had to create a separate crate specifically for the SQLite binary, which is excluded from the workspace. This exclusion is necessary because building binaries with the all-features flag would enable the embedded-db feature, causing all workspace crates to inherit the feature, even if it wasn’t explicitly enabled. This meant the SQLite binary had to be built separately. Consequently, all build commands now require two runs: one for the workspace crates and another for the SQLite binary.

To clean this up, we need to move away from using feature. One potential solution is to create an enum for the databases and implement the sqlx traits for it. However, this approach could be complex due to lifetime and trait bound constraints. more explanation here

We could also explore using sqlx Any.
here
This would still require query matching based on the backend since SQLite and PostgreSQL have different syntax and queries. Currently, we are already doing this with feature so shouldn't be too bad.
AnyConnectionOptions only supports connection string to build a connection so extra flags would be passed based on matching. The connect options for sqlite/postgres provide different flags such as sqlite has journal_mode/ busy mode so this would be like connect to database and manually run the queries on database to enable those.

@tbro
Copy link
Contributor

tbro commented Dec 6, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants