[1/n][dialect_support]Generalize indexer code to support different sql dialects #17167

sadhansood · 2024-04-16T00:19:16Z

Description

This PR generalizes the code in sui-indexer crate such that different diesel backends can be enabled through feature flags when compiling. The default backend is still postgres which means nothing changes in terms of how we build our binaries today.

The most important thing is that we do all kinds of db operation through macros which are feature gated such that we only pull in relevant db specific dependencies.
In Cargo.toml by default we do not add any db specific dependencies but only pull them through features:
In sui-indexer, all instances of diesel::PgConnection are pulled with postgres-feature and similary all instances of diesel::MySqlConnection are pulled with mysql-feature
run_query and run_query_async are replaced with their corresponding macro invocations
We do not try to make other dependencies of sui-indexer generic in this PR. This includes sui-graphql-rpc which just works with IndexerReader<PgConnection> for now (in the future we will refactor read for other db backends but for now the focus is to get writes to be compatible first)
Usages of #[Cached(..)] had to be replaced with an explicit usage of SizedCache as this macro doesn't seem to work for functions which take type parameters.
Next: we are going to introduce migrations for mysql and build sui-indexer for mysql

Test plan

Running this locally works: cargo run --bin sui-indexer -- --db-url "postgres://sadhansood:sadhansood@localhost/test" --rpc-client-url "<>" --fullnode-sync-worker --reset-db

vercel · 2024-04-16T00:19:25Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-core	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Apr 23, 2024 8:21pm

3 Ignored Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Apr 23, 2024 8:21pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Apr 23, 2024 8:21pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Apr 23, 2024 8:21pm

wlmyng · 2024-04-16T00:40:17Z

crates/sui-indexer/src/store/mod.rs

-                .read_only()
-                .run($query)
-                .map_err(|e| IndexerError::PostgresReadError(e.to_string()))
+        ($pool:expr, $query:expr, $repeatable_read:expr) => {{


nit: could we have the repeatable_read bool come before the query:expr? it took me a hot sec looking at where we use this macro before realizing a bool was added

wlmyng · 2024-04-16T00:44:40Z

crates/sui-indexer/src/apis/governance_api.rs

@@ -193,14 +208,8 @@ impl GovernanceReadApi {

 /// Cached exchange rates for validators for the given epoch, the cache size is 1, it will be cleared when the epoch changes.


suggestion: thoughts on lifting this up to the struct field?

yeah it's possible now, will do

crates/sui-indexer/src/db.rs

wlmyng · 2024-04-16T00:54:18Z

crates/sui-indexer/src/indexer_reader.rs

+{
+    pool: ConnectionPool<T>,
+    package_resolver: Arc<Resolver<PackageStoreWithLruCache<IndexerStorePackageResolver<T>>>>,
+    package_obj_type_cache: Arc<Mutex<SizedCache<String, Option<ObjectID>>>>,


is this mainly intended for CoinMetadata and TreasuryCap?

Yeah, it is meant to replace this usage of memoization specifically:

type = "SizedCache<String, Option<ObjectID>>", create = "{ SizedCache::with_size(10000) }", convert = r#"{ format!("{}{}", package_id, obj_type) }"#, result = true )] fn get_single_obj_id_from_package_publish( reader: &IndexerReader, package_id: ObjectID, obj_type: String,```

wlmyng · 2024-04-16T01:00:46Z

crates/sui-indexer/src/store/mod.rs

+                    .downcast_mut::<PoolConnection<diesel::MysqlConnection>>()
+                    .unwrap()
+                    .transaction($query)
+                    .map_err(|e| IndexerError::PostgresReadError(e.to_string()))


nit: MysqlReadError? Or a more generic DbReadError?

Yeah a more generic error type makes sense

amnn · 2024-04-16T11:14:56Z

crates/sui-graphql-rpc/src/context_data/package_cache.rs

Note that I'm about to put up a change that shifts this package cache to using GraphQL's Db type (an alias for its PgExecutor type), which should make this change unnecessary.

#17179 (and #17178)

gegaowp · 2024-04-19T15:27:13Z

I skimmed thru the DB setup & writing side of the change and looks great overall! I have mod.rs and indexer_reader.rs left, will review them later today.
cc @phoenix-o as some changes might overlap with the data ingestion framework change.

gegaowp · 2024-04-19T15:04:13Z

crates/sui-indexer/Cargo.toml

@@ -16,8 +16,14 @@ chrono.workspace = true
 serde_with.workspace = true
 clap.workspace = true
 tap.workspace = true
-diesel.workspace = true
-diesel-derive-enum.workspace = true
+diesel = { features = [


reading the doc of i-implement-a-third-party-backend-and-opt-into-breaking-changes, seems like it's recommended to pin to a minor version?
https://github.com/diesel-rs/diesel/blob/master/guide_drafts/migration_guide.md#removed-most-of-the-non-public-reachable-api

also curious what functionalities do we use of this feature?

I'll try to remove this from here, and just use the workspace (but I copied it from root level Cargo.toml)

gegaowp · 2024-04-19T15:18:49Z

crates/sui-indexer/src/db.rs

+                IndexerError::PostgresResetError(db_err_msg)
+            })?;
+        }
+        let (_registry_service, registry) = start_prometheus_server(


nit: let's keep prometheus in main.rs as they are not exactly db setup?

gegaowp · 2024-04-19T15:21:37Z

crates/sui-indexer/src/store/pg_indexer_store.rs

 use diesel::upsert::excluded;
 use diesel::ExpressionMethods;
 use diesel::OptionalExtension;
 use diesel::{QueryDsl, RunQueryDsl};
+use downcast::Any;


is this used?

Yes, this gets used in the macro but i'll double check (and remove if not)

gegaowp · 2024-04-19T15:22:22Z

crates/sui-indexer/src/handlers/checkpoint_handler.rs

@@ -93,24 +96,27 @@ where
        metrics,
        indexed_checkpoint_sender,
        package_buffer: IndexingPackageBuffer::start(package_tx),
+        _phantom: Default::default(),


what's this and how is this used?

This is because we added _phantom: PhantomData<T>, in the struct definition, we need to remember the connection type so later we can return the type in the connection pool. Check below function: fn pg_blocking_cp(&self) -> Result<ConnectionPool<T>, IndexerError> {

gegaowp

looks great other than the existing comments, thanks for working this out!

…fferent sql dialects

sadhansood requested review from amnn, wlmyng, stefan-mysten, emmazzz and suiwombat as code owners April 16, 2024 00:19

sadhansood force-pushed the sadhan/mysql_dialect branch from c59e08e to a65d285 Compare April 16, 2024 00:28

vercel bot deployed to Preview – sui-core April 16, 2024 00:29 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from a65d285 to e2de681 Compare April 16, 2024 00:41

vercel bot deployed to Preview – sui-core April 16, 2024 00:42 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from e2de681 to f005ad4 Compare April 16, 2024 00:52

vercel bot deployed to Preview – sui-core April 16, 2024 00:53 View deployment

sadhansood requested review from gegaowp, bmwill and phoenix-o April 16, 2024 00:54

wlmyng reviewed Apr 16, 2024

View reviewed changes

amnn reviewed Apr 16, 2024

View reviewed changes

vercel bot deployed to Preview – sui-core April 16, 2024 17:34 View deployment

gegaowp reviewed Apr 19, 2024

View reviewed changes

gegaowp approved these changes Apr 19, 2024

View reviewed changes

sadhansood force-pushed the sadhan/mysql_dialect branch from c03fdf2 to f6e5afc Compare April 23, 2024 07:15

vercel bot deployed to Preview – sui-core April 23, 2024 07:16 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from f6e5afc to 1a401bd Compare April 23, 2024 17:09

vercel bot deployed to Preview – sui-core April 23, 2024 17:11 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from 1a401bd to 6bb762e Compare April 23, 2024 17:43

vercel bot deployed to Preview – sui-core April 23, 2024 17:45 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from 6bb762e to c8e42dc Compare April 23, 2024 17:59

vercel bot deployed to Preview – sui-core April 23, 2024 18:00 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from c8e42dc to a8cc33b Compare April 23, 2024 20:19

[1/n][dialect_support][indexer] Generalize indexer code to support di…

a83ba36

…fferent sql dialects

vercel bot deployed to Preview – sui-core April 23, 2024 20:20 View deployment

sadhansood force-pushed the sadhan/mysql_dialect branch from a8cc33b to a83ba36 Compare April 23, 2024 20:20

vercel bot deployed to Preview – sui-core April 23, 2024 20:21 View deployment

sadhansood merged commit e305b4c into main Apr 23, 2024
46 of 47 checks passed

sadhansood deleted the sadhan/mysql_dialect branch April 23, 2024 20:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[1/n][dialect_support]Generalize indexer code to support different sql dialects #17167

[1/n][dialect_support]Generalize indexer code to support different sql dialects #17167

sadhansood commented Apr 16, 2024 •

edited

Loading

vercel bot commented Apr 16, 2024 •

edited

Loading

wlmyng Apr 16, 2024

wlmyng Apr 16, 2024

sadhansood Apr 16, 2024

wlmyng Apr 16, 2024

sadhansood Apr 16, 2024

wlmyng Apr 16, 2024

sadhansood Apr 16, 2024

amnn Apr 16, 2024

amnn Apr 16, 2024

gegaowp commented Apr 19, 2024

gegaowp Apr 19, 2024

gegaowp Apr 19, 2024

sadhansood Apr 19, 2024

gegaowp Apr 19, 2024

gegaowp Apr 19, 2024

sadhansood Apr 19, 2024

gegaowp Apr 19, 2024

sadhansood Apr 19, 2024

gegaowp left a comment

		@@ -193,14 +208,8 @@ impl GovernanceReadApi {

		/// Cached exchange rates for validators for the given epoch, the cache size is 1, it will be cleared when the epoch changes.

[1/n][dialect_support]Generalize indexer code to support different sql dialects #17167

[1/n][dialect_support]Generalize indexer code to support different sql dialects #17167

Conversation

sadhansood commented Apr 16, 2024 • edited Loading

Description

Test plan

vercel bot commented Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gegaowp commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gegaowp left a comment

Choose a reason for hiding this comment

sadhansood commented Apr 16, 2024 •

edited

Loading

vercel bot commented Apr 16, 2024 •

edited

Loading