Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
b3cd70f to
0df2c6b
Compare
| }; | ||
|
|
||
| // Recreate the table to ensure the correct PRIMARY KEY regardless of migration history. | ||
| // Tables migrated from v1 have PK (primary_namespace, key) only — missing |
There was a problem hiding this comment.
Huh, I'm confused, why is this true? We explictly have
// Add new 'secondary_namespace' column
let sql = format!(
"ALTER TABLE {}
ADD secondary_namespace TEXT DEFAULT \"\" NOT NULL;",
kv_table_name
);in migrate_v1_to_v2? Am I missing something, or is this Claude hallucinating?
There was a problem hiding this comment.
In v1 to v2 is added but its not a part of the primary key so we can't use ON CONFLICT on writes for just updating the value. Before we could use replace but now we want to keep the created_at date. This rewrites the table to make it a part of the primary key, otherwise we need to do a query inside to look up the created_at date when replacing. Could add a unique index instead but this felt cleaner
src/io/sqlite_store/migrations.rs
Outdated
|
|
||
| tx.execute( | ||
| &format!( | ||
| "CREATE TABLE {} ( |
There was a problem hiding this comment.
Honestly not sure if we need all that recreation logic, why can't we just do:
let sql = format!(
"ALTER TABLE {}
ADD created_at INTEGER NOT NULL DEFAULT 0,
kv_table_name
);
?
tnull
left a comment
There was a problem hiding this comment.
Thanks! Can we also extend the persistence_backwards_compatibility test to setup an ldk_node_070::Node and check we can continue to upgrade from it, too? (Could be refactored into do_persistence_backwards_compatibility taking the necessary parameters to DRY up the test logic)
Implement rust-lightning's PaginatedKVStore/PaginatedKVStoreSync traits on SqliteStore with a v2→v3 schema migration that adds a created_at column for tracking insertion order. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
0df2c6b to
143d16f
Compare
|
Added test |
Closes #804
Implement rust-lightning's PaginatedKVStore/PaginatedKVStoreSync traits
on SqliteStore with a v2→v3 schema migration that adds a created_at
column for tracking insertion order.