Skip to content

Paginated sqlite#807

Open
benthecarman wants to merge 2 commits intolightningdevkit:mainfrom
benthecarman:paginated-sqlite
Open

Paginated sqlite#807
benthecarman wants to merge 2 commits intolightningdevkit:mainfrom
benthecarman:paginated-sqlite

Conversation

@benthecarman
Copy link
Contributor

@benthecarman benthecarman commented Feb 25, 2026

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.

@benthecarman benthecarman requested a review from tnull February 25, 2026 00:35
@ldk-reviews-bot
Copy link

ldk-reviews-bot commented Feb 25, 2026

👋 Thanks for assigning @tnull as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

};

// Recreate the table to ensure the correct PRIMARY KEY regardless of migration history.
// Tables migrated from v1 have PK (primary_namespace, key) only — missing
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@benthecarman benthecarman Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


tx.execute(
&format!(
"CREATE TABLE {} (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
	);

?

Copy link
Collaborator

@tnull tnull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@benthecarman
Copy link
Contributor Author

Added test

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Implement PaginatedKVStore for SqliteStore

3 participants