Add Sqlite based PaginatedKVStore impl. #23

G8XSU · 2024-11-14T17:30:35Z

Add Sqlite based PaginatedKVStore impl.

For now we have different table and different DB for this.

Once we upstream this trait, we should be able to reuse at least the read and remove methods for KVStore and PaginatedKVStore. For write and list, we can either have separate implementations in each trait or introduce minor logic deduplication.

Mainly adapts the existing implementation for sql_lite in ldk-node for PaginatedKVStore i.e. support for:

Paginated list operations using next-page-token
Time based descending ordering of list keys.

G8XSU · 2024-11-26T19:31:55Z

Rebased on top of main, adjusted tests and table acc. to paginated store.

G8XSU · 2024-11-26T19:36:53Z

Once we upstream this trait, we should be able to reuse at least the read and remove methods for KVStore and PaginatedKVStore. For write and list, we can either have separate implementations in each trait or introduce minor logic deduplication.

jkczyz

Limited mostly to a diff against upstream.

jkczyz · 2024-11-28T03:24:40Z

ldk-server/src/io/sqlite_store/mod.rs

+// The maximum number of keys retrieved per page in paginated list operation.
+const LIST_KEYS_MAX_PAGE_SIZE: i32 = 100;


Don't have a strong opinion, but should we consider something smaller?

Since these are not mobile nodes, I think 100 is very small set of records for any server deployment.
We can change it to 50 if you had like, whatever value we choose, it is not a big deal since we can always adjust this without any api breaking changes.
A lower number also means higher RTT latency overhead.

jkczyz · 2024-11-28T03:27:49Z

ldk-server/src/io/sqlite_store/mod.rs

+// The current SQLite `user_version`, which we can use if we'd ever need to do a schema migration.
+const SCHEMA_USER_VERSION: u16 = 1;


Should we use 2 to match upstream? If this eventually implements both traits, it would be nice if downgrading from paginated was possible. Or at very least to avoid confusion with two different versions defined. Or if not possible, should we use 3 instead?

This one uses a separate db hence the separate schema version.
I think the way to upstream/migrate would be we introduce new table in upstream and at that time change the schema version of that db. After that, we can stop relying on this store and start using paginated queries from upstream apis.

ldk-server/src/io/sqlite_store/mod.rs

ldk-server/src/io/utils.rs

G8XSU · 2024-12-02T21:50:15Z

ldk-server/src/io/sqlite_store/mod.rs

+		let locked_conn = self.connection.lock().unwrap();
+
+		let sql = format!(
+			"SELECT key FROM {} WHERE primary_namespace=:primary_namespace AND secondary_namespace=:secondary_namespace AND key > :page_token ORDER BY creation_time DESC LIMIT :page_size",


there is a bug in pagination that needs to be fixed, we need an order by on key as well i think.

page_token/ordering/pagination needs to account for both creation_time and key.

Fixed in latest commit.

ldk-server/src/io/sqlite_store/mod.rs

jkczyz · 2024-12-04T15:55:29Z

ldk-server/src/io/utils.rs

-										PrintableString(primary_namespace), PrintableString(secondary_namespace), PrintableString(key));
+				"Failed to {} {}/{}/{}: primary namespace may not be empty if a non-empty secondary namespace is given.",
+				operation,
+				PrintableString(primary_namespace), PrintableString(secondary_namespace), PrintableString(key));


Surprised rustfmt doesn't move the closing parenthesis down to a new line. Maybe that confused it?

I think depends on whether it can fit all args in single line or not, otherwise it would have placed them on separate line each.
Moved parenthesis to new line manually.

jkczyz · 2024-12-04T17:08:24Z

ldk-server/src/io/sqlite_store/mod.rs

 		let sql = format!(
-			"SELECT key FROM {} WHERE primary_namespace=:primary_namespace AND secondary_namespace=:secondary_namespace AND key > :page_token ORDER BY creation_time DESC LIMIT :page_size",
+			"SELECT key, creation_time FROM {} WHERE primary_namespace=:primary_namespace AND secondary_namespace=:secondary_namespace \
+			AND ( creation_time < :creation_time_token OR (creation_time = :creation_time_token AND key > :key_token) ) \


Not sure that I understand why we need creation_time_token. Shouldn't the new ORDER BY suffice?

It will not suffice, here is an example :

Input:

Data:

Key Creation Time

A 1

C 2

B 3

E 4

D 5

Page Size: 2

Paginated Output should be:

First Page: D, E. , page_token: E, 4

Second Page: B, C, page_token: C, 2

Third Page: A, page_token: None

Behavior Without a Creation Time Token:

Output: D, E, page_token: E

Reason: Since there is no key greater than E, no additional pages are available.

Main reason this happens is because we prioritize ordering by creation_time for pagination.
And we can't just use creation_time for pagination as there might be 2 keys with the same creation_time resulting in duplicate or missed results.

Ah, right. That makes sense. Thanks for explaining. Can the condition be reduced to the following?

creation_time <= :creation_time_token AND key > :key_token

Though that would unnecessarily preform a key comparison when not needed, so maybe best to avoid?

No it can't be reduced to that, that too will lead to wrong pagination results in the same example.

Note: Keys after page-1 are less than E

Gotcha. Yeah, I see now.

jkczyz

LGTM. Please squash.

jkczyz · 2024-12-04T20:12:11Z

ldk-server/src/io/sqlite_store/mod.rs

 		let sql = format!(
-			"SELECT key FROM {} WHERE primary_namespace=:primary_namespace AND secondary_namespace=:secondary_namespace AND key > :page_token ORDER BY creation_time DESC LIMIT :page_size",
+			"SELECT key, creation_time FROM {} WHERE primary_namespace=:primary_namespace AND secondary_namespace=:secondary_namespace \
+			AND ( creation_time < :creation_time_token OR (creation_time = :creation_time_token AND key > :key_token) ) \


Gotcha. Yeah, I see now.

…tion.

G8XSU · 2024-12-04T21:29:12Z

Squashes fixups!

G8XSU requested a review from jkczyz November 14, 2024 18:10

jkczyz reviewed Nov 28, 2024

View reviewed changes

G8XSU commented Dec 2, 2024

View reviewed changes

G8XSU requested a review from jkczyz December 3, 2024 19:56

jkczyz reviewed Dec 4, 2024

View reviewed changes

G8XSU requested a review from jkczyz December 4, 2024 18:37

G8XSU mentioned this pull request Dec 4, 2024

Ldk-Server Milestone 3 #11

Open

9 tasks

jkczyz reviewed Dec 4, 2024

View reviewed changes

G8XSU added 2 commits December 4, 2024 13:28

Fix PaginatedKVStore interface to account for time ordering in pagina…

52a63e9

…tion.

Add Sqlite based PaginatedKVStore impl.

7eab8bf

jkczyz approved these changes Dec 4, 2024

View reviewed changes

G8XSU merged commit 31026d6 into lightningdevkit:main Dec 4, 2024
1 check passed

		// The maximum number of keys retrieved per page in paginated list operation.
		const LIST_KEYS_MAX_PAGE_SIZE: i32 = 100;

		// The current SQLite `user_version`, which we can use if we'd ever need to do a schema migration.
		const SCHEMA_USER_VERSION: u16 = 1;

Add Sqlite based PaginatedKVStore impl. #23

Add Sqlite based PaginatedKVStore impl. #23

Uh oh!

Conversation

G8XSU commented Nov 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

G8XSU commented Nov 26, 2024

Uh oh!

G8XSU commented Nov 26, 2024

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Input:

Paginated Output should be:

Behavior Without a Creation Time Token:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU Dec 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU commented Dec 4, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

G8XSU commented Nov 14, 2024 •

edited

Loading

G8XSU Dec 2, 2024 •

edited

Loading

G8XSU Dec 4, 2024 •

edited

Loading

G8XSU Dec 4, 2024 •

edited

Loading