Migrate INTEGER/BIGINT columns storing u64 values to TEXT#1954
Conversation
crodas
force-pushed
the
experiment/store-u64-sql-db
branch
3 times, most recently
from
89a0769 to
eecb267
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1954 +/- ##
==========================================
+ Coverage 65.13% 65.17% +0.03%
==========================================
Files 329 330 +1
Lines 56769 56899 +130
==========================================
+ Hits 36978 37083 +105
- Misses 19791 19816 +25 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thesimplekid
left a comment
thesimplekid
left a comment
There was a problem hiding this comment.
It seems this is still i64 should be u64, there might be others? https://github.com/crodas/cdk/blob/eecb267cff1768f00872a961afeaa5cc766992e8/crates/cdk-sql-common/src/wallet/mod.rs#L1801
| .unwrap_or(0); | ||
| .collect::<Result<Vec<_>, _>>()? | ||
| .into_iter() | ||
| .sum(); |
There was a problem hiding this comment.
This can also overflow we should always used checked math.
| -- Only amount and fee columns are migrated; timestamps and indices stay as INTEGER/BIGINT | ||
|
|
||
| -- proof | ||
| ALTER TABLE proof ALTER COLUMN amount TYPE TEXT USING amount::TEXT; |
There was a problem hiding this comment.
For these columns we could add a check that they are numbers even as text. Though I'm also okay to just relay on rust for that.
There was a problem hiding this comment.
Is there a way to enforce the SQL level? I'm not aware of that. Our rust codebase is quite flexible, though, when reading a number off the database
There was a problem hiding this comment.
I think it could be done with a check constraint on the column that probably uses a regex.
Is there a way to enforce the SQL level? I'm not aware of that. Our rust codebase is quite flexible, though, when reading a number off the database
It would fail at the row to struct level though I think even if it passes here. Which i think it probably okay, since we're also in control of what gets put in and we know thats a number?
There was a problem hiding this comment.
I think it could be done with a check constraint on the column that probably uses a regex.
I will explore this, it's a good idea.
It would fail at the row to struct level though I think even if it passes here. Which i think it probably okay, since we're also in control of what gets put in and we know thats a number?
It is explicit what kind of number should come from the storage layer, regardless of how it was stored. There are two main goals: never lose precision, and never ever do math outside of Rust.
crodas
force-pushed
the
experiment/store-u64-sql-db
branch
2 times, most recently
from
e851296 to
dfd0c57
Compare
|
Per the dev call we said it would be good to see benchmarks of performance before changing to text. Specifically #1152 was introduced to resolve performance bottlenecks we should keep in mind. |
crodas
force-pushed
the
experiment/store-u64-sql-db
branch
from
dfd0c57 to
a8c5947
Compare
load_from_mint holds fetch_lock then calls load_from_db which tries to acquire it again. Tokio Mutex is not reentrant so this deadlocks. Made load_from_db private and removed the lock since both callers already gate on is_populated.
u64 cannot be faithfully represented as SQL INTEGER/BIGINT (signed i64). The storage layer performs no math on these columns, so TEXT is safe. Migrates amount, fee, expiry, and similar value columns to TEXT across all databases (mint, wallet, auth SQLite/PostgreSQL, and Supabase). Time columns (created_time, updated_at, timestamp, etc.) are left as INTEGER/BIGINT since they fit within i64 range. Rust code updated: From<u64> for Value converts to Text, all .to_i64() calls replaced with .to_u64(), and as i64 casts removed for value fields.
The named parameter parser was incorrectly treating `::` (PostgreSQL type cast operator) as the start of a named parameter. Skip `::` and emit it as raw SQL instead.
Move amount summation out of SQL queries and into Rust code. This avoids relying on database-specific numeric behavior (e.g. INTEGER overflow, type coercion differences between SQLite and PostgreSQL) and keeps all monetary math in Rust where it benefits from checked arithmetic via the Amount type. Changes: - Add keyset_amounts module to compute per-keyset totals in Rust - Rewrite completed_operations, proofs, and signatures queries to return raw rows instead of SUM aggregates - Update wallet storage queries to sum in Rust TODO: stream amounts from storage to avoid loading full result sets into memory; propose moving all math outside the storage layer entirely. Do all math outside of the storage layer.
Remove MethodTimer module, all timer usage in wallet DB operations, and lock timing warnings in AsyncSqlite. These were temporary diagnostics for investigating connection pool starvation.
Replace std::sync::Condvar with tokio::sync::Notify in the connection pool. The blocking Condvar::wait_timeout was starving the tokio runtime when pool size=1 (in-memory SQLite), causing 5s cascading timeouts on concurrent ops.
The pool.get() method returns a future that must be awaited before calling .is_ok() or .expect() on the result.
crodas
force-pushed
the
experiment/store-u64-sql-db
branch
from
a8c5947 to
18ef26b
Compare
2 participants
Description
Cashu amounts are u64, but neither SQLite nor PostgreSQL has an unsigned 64-bit integer type. Both INTEGER and BIGINT are signed, so values above
i64::MAXsilently overflow when cast to i64. This PR stores all amount-like columns as TEXT instead, eliminating the risk of truncation entirely.Once columns become TEXT, PostgreSQL refuses to do arithmetic on them; expressions like total_issued + EXCLUDED.total_issued simply fail. Even with SQLite, casting large text values back to integers for math is unreliable. Rather than patching the SQL with ever more complex
CAST/COALESCEchains, this PR moves all arithmetic into Rust. Each keyset amounts update now reads the current value, adds the delta with checked_add (which returns an explicit error on overflow instead of wrapping silently), and writes the result back. The same applies to SUM() aggregations — amounts are fetched as individual rows and summed in Rust.A secondary fix handles PostgreSQL's
::cast syntax (column::TYPE), which the SQL parser was misinterpreting as a named bind parameter.As a next step, amounts should be streamed from storage rather than loaded as full result sets into memory, and all math should be performed in the mint or wallet, outside the storage layer.
This PR depends on #1957 and #1808
Notes to the reviewers
Suggested CHANGELOG Updates
CHANGED
ADDED
REMOVED
FIXED
Checklist
just quick-checkbefore committingcrates/cdk-ffi)