Introduce cdk-sql-common #890

2025-07-15T12:42:12Z

crodas commented

2025-07-15 12:42:12 +00:00

(Migrated from github.com)

Description

The primary purpose of this new crate is to have a common and shared codebase for all SQL storage systems. It would force us to write standard SQL using best practices for all databases.

This crate has been extracted from #878

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED

Checklist

I followed the code style guidelines
I ran just final-check before committing

### Description The primary purpose of this new crate is to have a common and shared codebase for all SQL storage systems. It would force us to write standard SQL using best practices for all databases. This crate has been extracted from #878 ----- ### Notes to the reviewers  ----- ### Suggested [CHANGELOG](https://github.com/cashubtc/cdk/blob/main/CHANGELOG.md) Updates  #### CHANGED #### ADDED #### REMOVED #### FIXED ---- ### Checklist * [x] I followed the [code style guidelines](https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md) * [x] I ran `just final-check` before committing

thesimplekid (Migrated from github.com) reviewed

2025-07-22 17:04:35 +00:00

thesimplekid (Migrated from github.com) left a comment

Nice work. I like that the sql is abstracted into a common place where we will not have to maintain two versions for sqlite and postgres. Though I have a question on this are we limited to sqlite feature set. The first one that comes to mint is row locks, to prevent race conditions in places like proof state updates we currently have the db locked in a transaction but in postgres we could use a row lock to only lock the specific proofs involved. Can we still do this?

crodas (Migrated from github.com) reviewed

2025-07-22 20:52:11 +00:00

crodas (Migrated from github.com) reviewed

2025-07-22 21:00:47 +00:00

crodas commented

2025-07-22 21:09:50 +00:00

(Migrated from github.com)

The first one that comes to mint is row locks, to prevent race conditions in places like proof state updates we currently have the db locked in a transaction but in postgres we could use a row lock to only lock the specific proofs involved. Can we still do this?

The idea is to use the correct SQL, even if the driver does not support that. The example is SELECT FOR UPDATE , which is standard, but SQLite does not support it, because semantically it makes no sense, since TRANSACTION locks the database exclusively. So any changes are rewritten by the driver

Although RAW SQL is written, it is vaguely parsed to extract placeholders, because we use named arguments for readability but some drivers use position placeholders.

The bits that are missing that can be a follow-up are:

Caching for SQL statements and placeholder parsing. It will parsed for all but none for the bind_vec
Add unit tests for all the expectations of the driver, specially the read and lock.

> The first one that comes to mint is row locks, to prevent race conditions in places like proof state updates we currently have the db locked in a transaction but in postgres we could use a row lock to only lock the specific proofs involved. Can we still do this? The idea is to use the correct SQL, even if the driver does not support that. The example is [SELECT FOR UPDATE ](https://github.com/cashubtc/cdk/pull/890/files#diff-3187b934449f9df8b872cdc495b489eade400dbc42232ea5320c8a7b991bb7caR171-R178), which is standard, but SQLite does not support it, because semantically it makes no sense, since TRANSACTION locks the database exclusively. So any changes are [rewritten by the driver](https://github.com/cashubtc/cdk/pull/890/files#diff-48690e0d6302c75c4c71b0f8f5df1a28817b54ef0bd3136dc0dcde576491b5f6R136) Although RAW SQL is written, it is [vaguely parsed ](https://github.com/cashubtc/cdk/pull/890/files#diff-d7c06cd47222a89473705c0d4f43a88e06b485988a15003a9a0164af122c58b6R73-R141)to extract placeholders, because we use named arguments for readability but some drivers [use position placeholders](https://github.com/cashubtc/cdk/pull/890/files#diff-d7c06cd47222a89473705c0d4f43a88e06b485988a15003a9a0164af122c58b6R166-R195). The bits that are missing that can be a follow-up are: * Caching for SQL statements and placeholder parsing. It will parsed for all but none for the `bind_vec` * Add unit tests for all the expectations of the driver, specially the read and lock.

asmogo (Migrated from github.com) reviewed

2025-07-25 11:58:42 +00:00

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

If the names match, we should remove the prefix from the names. Otherwise, this will cause all migrations to be rerun because they were moved to a new subdirectory.
Alternatively, we could remove the path from all pub static MIGRATIONS.

If the names match, we should remove the prefix from the names. Otherwise, this will cause all migrations to be rerun because they were moved to a new subdirectory. Alternatively, we could remove the path from all `pub static MIGRATIONS`.

asmogo (Migrated from github.com) requested changes

2025-07-25 12:27:31 +00:00

crates/cdk-sql-common/Cargo.toml

					
				@ -0,0 +25,4 @@

				tracing.workspace = true

				serde.workspace = true

				serde_json.workspace = true

				lightning-invoice.workspace = true

this is unused

crodas (Migrated from github.com) reviewed

2025-07-25 13:39:09 +00:00

crates/cdk-sql-common/Cargo.toml

					
				@ -0,0 +25,4 @@

				tracing.workspace = true

				serde.workspace = true

				serde_json.workspace = true

				lightning-invoice.workspace = true

great catch. I'll fix it.

crodas (Migrated from github.com) reviewed

2025-07-25 14:27:18 +00:00

crates/cdk-sql-common/Cargo.toml

					
				@ -0,0 +25,4 @@

				tracing.workspace = true

				serde.workspace = true

				serde_json.workspace = true

				lightning-invoice.workspace = true

Fixed in e1cc72ecff76d801e2299d548002e5a4a955d9c7

crodas (Migrated from github.com) reviewed

2025-07-25 14:28:32 +00:00

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

Isn't e1cc72ecff76d801e2299d548002e5a4a955d9c7 a better solution?

asmogo (Migrated from github.com) reviewed

2025-07-25 14:43:51 +00:00

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

That should do the job. Thanks 👍 I had something similar in mind like using the file_name() function on the path.

diff --git a/crates/cdk-sql-common/build.rs b/crates/cdk-sql-common/build.rs
--- a/crates/cdk-sql-common/build.rs	(revision 316538beb9aee2bf89637bcd1744e92f6d8e1c50)
+++ b/crates/cdk-sql-common/build.rs	(date 1753447116890)
@@ -17,15 +17,14 @@
         let skip_path = parent.to_str().unwrap_or_default().len();
         let dest_path = parent.join("migrations.rs");
         let mut out_file = File::create(&dest_path).expect("Failed to create migrations.rs");
-
-        let skip_name = migration_path.to_str().unwrap_or_default().len();
-
+        
+        
         writeln!(out_file, "/// @generated").unwrap();
         writeln!(out_file, "/// Auto-generated by build.rs").unwrap();
         writeln!(out_file, "pub static MIGRATIONS: &[(&str, &str)] = &[").unwrap();
 
         for path in &files {
-            let rel_name = &path.to_str().unwrap().replace("\\", "/")[skip_name + 1..]; // for Windows
+            let rel_name = &path.file_name().unwrap().to_str().unwrap();
             let rel_path = &path.to_str().unwrap().replace("\\", "/")[skip_path..]; // for Windows
             writeln!(
                 out_file,

That should do the job. Thanks 👍 I had something similar in mind like using the `file_name()` function on the path. ``` diff --git a/crates/cdk-sql-common/build.rs b/crates/cdk-sql-common/build.rs --- a/crates/cdk-sql-common/build.rs (revision 316538beb9aee2bf89637bcd1744e92f6d8e1c50) +++ b/crates/cdk-sql-common/build.rs (date 1753447116890) @@ -17,15 +17,14 @@ let skip_path = parent.to_str().unwrap_or_default().len(); let dest_path = parent.join("migrations.rs"); let mut out_file = File::create(&dest_path).expect("Failed to create migrations.rs"); - - let skip_name = migration_path.to_str().unwrap_or_default().len(); - + + writeln!(out_file, "/// @generated").unwrap(); writeln!(out_file, "/// Auto-generated by build.rs").unwrap(); writeln!(out_file, "pub static MIGRATIONS: &[(&str, &str)] = &[").unwrap(); for path in &files { - let rel_name = &path.to_str().unwrap().replace("\\", "/")[skip_name + 1..]; // for Windows + let rel_name = &path.file_name().unwrap().to_str().unwrap(); let rel_path = &path.to_str().unwrap().replace("\\", "/")[skip_path..]; // for Windows writeln!( out_file, ```

asmogo (Migrated from github.com) requested changes

2025-07-25 20:48:16 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

Im not sure why we need that still_valid here. It seems like we always create the resource in valid state true.
The timeout should be moved to the configuration.

Im not sure why we need that `still_valid` here. It seems like we always create the resource in valid state true. The timeout should be moved to the configuration.

crodas (Migrated from github.com) reviewed

2025-07-25 21:05:17 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

still_valid is used to signal the resource manager if the current resource is stale and it should be dropped (and a be eventually replaced with a new one).

This makes no sense for SQLite but it does for pgsql, to reconnect stale connections.

`still_valid` is used to signal the resource manager if the current resource is stale and it should be dropped (and a be eventually replaced with a new one). This makes no sense for SQLite but it does for pgsql, to reconnect stale connections.

crodas (Migrated from github.com) reviewed

2025-07-25 21:12:09 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

I acknowledge that the name is not very descriptive; a better name could be used. Any suggestions?

asmogo (Migrated from github.com) reviewed

2025-07-25 21:48:03 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

OK gotcha. That makes sense. I haven’t looked into the psql implementation yet.

Perhaps we should simply call it stale and negate the drop condition?

Do we need this as a parameter to the new_resource function?
New resources might always be valid (or not stale)

OK gotcha. That makes sense. I haven’t looked into the psql implementation yet. Perhaps we should simply call it `stale` and negate the drop condition? Do we need this as a parameter to the `new_resource` function? New resources might always be valid (or not stale)

crodas (Migrated from github.com) reviewed

2025-07-25 21:52:33 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

I love it. I will rename it and document the behaviour, thanks for the feedback

crodas (Migrated from github.com) reviewed

2025-07-26 00:38:16 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

is it better in cd066415c53ef2e0e9ac0c4eef3421bbeaa6c6b0 /cc @asmogo ?

thesimplekid (Migrated from github.com) requested changes

2025-07-26 13:43:16 +00:00

thesimplekid (Migrated from github.com) left a comment

Overall I think this looks good I am just getting a runtime error that needs to be addressed. I am getting the Error: Duplicate entry when i restart the mint. This happened when I did not have a mint db created one by starting the mint, then stopping the mint and attempting to restart.

Overall I think this looks good I am just getting a runtime error that needs to be addressed. I am getting the Error: Duplicate entry when i restart the mint. This happened when I did not have a mint db created one by starting the mint, then stopping the mint and attempting to restart. <img width="1880" height="185" alt="Image" src="https://github.com/user-attachments/assets/d478b5a9-c282-496a-9ed7-7cdbbe7df2b3" />

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

let basename = match name.split_once(['/', '\\']) {
    Some((prefix, basename)) => {
        if prefix != db_prefix {
            continue;
        }
        basename
    }
    None => name,
};

Our code style says we should use match here https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#if-let.

```suggestion let basename = match name.split_once(['/', '\\']) { Some((prefix, basename)) => { if prefix != db_prefix { continue; } basename } None => name, }; ``` Our code style says we should use match here https://github.com/cashubtc/cdk/blob/main/CODE_STYLE.md#if-let.

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

    /// If `stale` is ever set to TRUE it is assumed the resource is no longer valid and it will be

```suggestion /// If `stale` is ever set to TRUE it is assumed the resource is no longer valid and it will be ```

crates/cdk-sql-common/src/stmt.rs

					
				@ -0,0 +1,359 @@

				//! Stataments mod

if let Ok(mut cache) = self.cache.write() {
    if let Some((_, cached_sql)) = cache.get_mut(&original_sql) {
        *cached_sql = Some(sql.clone().into());
    }
} else {
    tracing::warn!("Failed to acquire write lock for SQL statement cache");
}

Just add some logging instead of ignoring the error

```suggestion if let Ok(mut cache) = self.cache.write() { if let Some((_, cached_sql)) = cache.get_mut(&original_sql) { *cached_sql = Some(sql.clone().into()); } } else { tracing::warn!("Failed to acquire write lock for SQL statement cache"); } ``` Just add some logging instead of ignoring the error

asmogo (Migrated from github.com) reviewed

2025-07-27 18:04:04 +00:00

asmogo (Migrated from github.com) left a comment

using path.file_name() in the builder:
Not sure if this has further implications. Current master also uses the file_name function (but with to_string_lossy).

diff --git a/crates/cdk-sql-common/build.rs b/crates/cdk-sql-common/build.rs
index 6b32a916..639710ac 100644
--- a/crates/cdk-sql-common/build.rs
+++ b/crates/cdk-sql-common/build.rs
@@ -17,15 +17,13 @@ fn main() {
         let skip_path = parent.to_str().unwrap_or_default().len();
         let dest_path = parent.join("migrations.rs");
         let mut out_file = File::create(&dest_path).expect("Failed to create migrations.rs");
-
-        let skip_name = migration_path.to_str().unwrap_or_default().len();
-
+        
         writeln!(out_file, "/// @generated").unwrap();
         writeln!(out_file, "/// Auto-generated by build.rs").unwrap();
         writeln!(out_file, "pub static MIGRATIONS: &[(&str, &str)] = &[").unwrap();
 
         for path in &files {
-            let rel_name = &path.to_str().unwrap().replace("\\", "/")[skip_name + 1..]; // for Windows
+            let rel_name = &path.file_name().unwrap().to_str().unwrap();
             let rel_path = &path.to_str().unwrap().replace("\\", "/")[skip_path..]; // for Windows
             writeln!(
                 out_file,
diff --git a/crates/cdk-sql-common/src/common.rs b/crates/cdk-sql-common/src/common.rs
index ccdd4cad..536a77e7 100644
--- a/crates/cdk-sql-common/src/common.rs
+++ b/crates/cdk-sql-common/src/common.rs
@@ -21,18 +21,8 @@ pub async fn migrate<C: DatabaseExecutor>(
 
     // Apply each migration if it hasn’t been applied yet
     for (name, sql) in migrations {
-        let basename = match name.split_once(['/', '\\']) {
-            Some((prefix, basename)) => {
-                if prefix != db_prefix {
-                    continue;
-                }
-                basename
-            }
-            None => name,
-        };
-
         let is_missing = query("SELECT name FROM migrations WHERE name = :name")?
-            .bind("name", basename)
+            .bind("name", name)
             .pluck(conn)
             .await?
             .is_none();

using `path.file_name()` in the builder: Not sure if this has further implications. Current [master](https://github.com/cashubtc/cdk/blob/main/crates/cdk-sqlite/build.rs#L26C29-L26C38) also uses the file_name function (but with `to_string_lossy`). ``` diff --git a/crates/cdk-sql-common/build.rs b/crates/cdk-sql-common/build.rs index 6b32a916..639710ac 100644 --- a/crates/cdk-sql-common/build.rs +++ b/crates/cdk-sql-common/build.rs @@ -17,15 +17,13 @@ fn main() { let skip_path = parent.to_str().unwrap_or_default().len(); let dest_path = parent.join("migrations.rs"); let mut out_file = File::create(&dest_path).expect("Failed to create migrations.rs"); - - let skip_name = migration_path.to_str().unwrap_or_default().len(); - + writeln!(out_file, "/// @generated").unwrap(); writeln!(out_file, "/// Auto-generated by build.rs").unwrap(); writeln!(out_file, "pub static MIGRATIONS: &[(&str, &str)] = &[").unwrap(); for path in &files { - let rel_name = &path.to_str().unwrap().replace("\\", "/")[skip_name + 1..]; // for Windows + let rel_name = &path.file_name().unwrap().to_str().unwrap(); let rel_path = &path.to_str().unwrap().replace("\\", "/")[skip_path..]; // for Windows writeln!( out_file, diff --git a/crates/cdk-sql-common/src/common.rs b/crates/cdk-sql-common/src/common.rs index ccdd4cad..536a77e7 100644 --- a/crates/cdk-sql-common/src/common.rs +++ b/crates/cdk-sql-common/src/common.rs @@ -21,18 +21,8 @@ pub async fn migrate<C: DatabaseExecutor>( // Apply each migration if it hasn’t been applied yet for (name, sql) in migrations { - let basename = match name.split_once(['/', '\\']) { - Some((prefix, basename)) => { - if prefix != db_prefix { - continue; - } - basename - } - None => name, - }; - let is_missing = query("SELECT name FROM migrations WHERE name = :name")? - .bind("name", basename) + .bind("name", name) .pluck(conn) .await? .is_none(); ```

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

removing the base name and using the name variable as bind parameter solves the issue mentioned in https://github.com/cashubtc/cdk/pull/890#pullrequestreview-3058149066. This was introduced due to a previous change request.
Maybe we should consider removing the path from the name while creating the MIGRATIONS in build.rs.
The path (database engine) is still referenced by file location.

removing the base name and using the name variable as bind parameter solves the issue mentioned in https://github.com/cashubtc/cdk/pull/890#pullrequestreview-3058149066. This was introduced due to a previous change request. Maybe we should consider removing the path from the name while creating the MIGRATIONS in build.rs. The path (database engine) is still referenced by file location.

asmogo (Migrated from github.com) reviewed

2025-07-27 18:06:59 +00:00

crates/cdk-sql-common/src/pool.rs

					
				@ -0,0 +27,4 @@

				/// Trait to manage resources

				pub trait ResourceManager: Debug {

				    /// The resource to be pooled

				    type Resource: Debug;

Yep. Thanks!

crodas (Migrated from github.com) reviewed

2025-07-28 17:26:36 +00:00

crates/cdk-sql-common/src/common.rs

					
				@ -0,0 +1,44 @@

				use crate::database::DatabaseExecutor;

Is it better in fc7adba0?

thesimplekid commented

2025-07-29 01:31:39 +00:00

(Migrated from github.com)

ACK fc7adba067b8aa518690ec56704211ab6f86b220

❤️ 1 🎉 1

thesimplekid (Migrated from github.com) reviewed

2025-07-29 12:54:08 +00:00

thesimplekid (Migrated from github.com) left a comment

@asmogo Anything outstanding in your view on this pr?

asmogo (Migrated from github.com) approved these changes

2025-07-29 19:26:55 +00:00

asmogo (Migrated from github.com) left a comment

ACK. Didn’t test https://github.com/cashubtc/cdk/pull/890/commits/0041c135f7872cb9dda7e68c1adfad8d013672c8 explicitly but it seems ok.

thesimplekid (Migrated from github.com) approved these changes

2025-07-29 19:32:04 +00:00

Sign in to join this conversation.

No reviewers

asmogo

thesimplekid

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

cashubtc/cdk!890

				`@ -0,0 +1,44 @@`
				`use crate::database::DatabaseExecutor;`

				`@ -0,0 +1,44 @@`
				`use crate::database::DatabaseExecutor;`

				`@ -0,0 +1,44 @@`
				`use crate::database::DatabaseExecutor;`

				`@ -0,0 +1,44 @@`
				`use crate::database::DatabaseExecutor;`

Introduce cdk-sql-common #890

Description

Notes to the reviewers

Suggested CHANGELOG Updates

CHANGED

ADDED

REMOVED

FIXED

Checklist

Introduce `cdk-sql-common` #890