Skip to content

Paginate by creation time instead of key order #96

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
benthecarman wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Paginate by creation time instead of key order #96

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ integration with different cloud providers. As long as the API contract is imple
effortlessly switch between different instances of VSS.

VSS ships with a PostgreSQL implementation by default and can be hosted in your favorite infrastructure/cloud provider
(AWS/GCP) and its backend storage can be switched with some other implementation for KeyValueStore if needed.
(AWS/GCP). The backend storage can be switched with another implementation, but it must support ordering by creation
time, a simple key-value store is not sufficient.
Copy link
Copy Markdown
Contributor

@tnull tnull Apr 20, 2026

Choose a reason for hiding this comment

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

No, we don't sort by creation time anymore. Might be good to be more accurate here, too.


### Project Status

Expand Down
4 changes: 3 additions & 1 deletion proto/vss.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,16 @@ message ListKeyVersionsRequest {
// Server response for `ListKeyVersions` API.
message ListKeyVersionsResponse {

// Fetched keys and versions.
// Fetched keys and versions, ordered by creation time (newest first).
Copy link
Copy Markdown
Contributor

@tnull tnull Apr 16, 2026

Choose a reason for hiding this comment

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

Remind me, is there any particular reason we need to use time for this? Wouldn't we get around the need for a tie breaker if we'd simply use a monotonically increasing atomic counter instead (e.g., a Postgres BIGSERIAL column) ? Then we'd be certain than each entry has a unique value?

Copy link
Copy Markdown
Contributor

@tnull tnull Apr 16, 2026

Choose a reason for hiding this comment

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

Claude:

 Monotonic counter vs. creation_time

  Your intuition is right — a monotonically increasing counter (e.g., a Postgres BIGSERIAL column) would be strictly simpler here. The current
  approach has to deal with:

  - Tie-breaking: The compound condition (created_at < $3 OR (created_at = $3 AND key > $4)) in the SQL query
  - A composite page token: Encoding both the timestamp and the key (0:<micros>:<key>)
  - A composite index: (user_token, store_id, created_at DESC, key ASC) INCLUDE (version)
  - A dedicated test just for the tie-breaking behavior

  With an auto-incrementing counter (say row_id BIGSERIAL), all of that collapses to:
  - WHERE row_id < $cursor ORDER BY row_id DESC LIMIT $N
  - Page token = just the counter value
  - Simple single-column addition to the index
  - No tie-breaking needed since values are unique by definition

  The only argument for creation_time would be if it carried semantic meaning the client cares about (e.g., "show me keys created after X"). But
  looking at the ListKeyVersionsResponse, created_at isn't exposed to the client — it's purely an internal ordering mechanism. So you're paying the
   complexity tax of timestamps without getting the semantic benefit.

Copy link
Copy Markdown
Contributor

@tankyleo tankyleo Apr 16, 2026

Choose a reason for hiding this comment

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

Thank you tnull for raising this point. I did some back-and-forth with claude, and yea this is interesting !

My one question at this point is backwards compat. Do you have thoughts on this point ? I'm thinking if keys created before this commit don't have strict creation order, this is OK. Perhaps we can use the VSS version you described above to encourage people to upgrade once we start relying on PaginatedKVStore in LDK Node.

@benthecarman let me know what you think.

Copy link
Copy Markdown
Contributor

@tankyleo tankyleo Apr 16, 2026

Choose a reason for hiding this comment

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

@tnull I ping you on the response above in case it helps bubble this up in your inbox :)

Copy link
Copy Markdown
Contributor Author

@benthecarman benthecarman Apr 16, 2026

Choose a reason for hiding this comment

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

Ah, I didn't know we could use BIGSERIAL for non-primary keys (in sqlite you can't).

Yeah this might be better. On backwards compat, we still should be able to backfill the column by sorting by creation time, however, the migration that claude generated for this is pretty big/ugly.

Copy link
Copy Markdown
Contributor

@tnull tnull Apr 20, 2026

Choose a reason for hiding this comment

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

Thank you tnull for raising this point. I did some back-and-forth with claude, and yea this is interesting !

My one question at this point is backwards compat. Do you have thoughts on this point ? I'm thinking if keys created before this commit don't have strict creation order, this is OK. Perhaps we can use the VSS version you described above to encourage people to upgrade once we start relying on PaginatedKVStore in LDK Node.

@benthecarman let me know what you think.

I think the backfill should still handle it for the most part? But yeah, apart from that I think it might be good to lean on the versioning byte for this going forward, and make sure we publish vss-server v0.1 (with protocol versioning support) prior to LDK Node v0.8?

Copy link
Copy Markdown
Contributor

@tnull tnull Apr 20, 2026

Choose a reason for hiding this comment

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

These docs are now inaccurate, no?

Copy link
Copy Markdown
Contributor

@tankyleo tankyleo Apr 20, 2026

Choose a reason for hiding this comment

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

Our implementation of this contract changed, but the contract remains the same, I think this is still accurate. Same for the other comments below.

// Even though this API reuses the `KeyValue` struct, the `value` sub-field will not be set by the server.
repeated KeyValue key_versions = 1;

// `next_page_token` is a pagination token, used to retrieve the next page of results.
// Use this value to query for next-page of paginated `ListKeyVersions` operation, by specifying
// this value as the `page_token` in the next request.
//
// Following AIP-158 (https://google.aip.dev/158):
//
// If `next_page_token` is empty (""), then the "last page" of results has been processed and
Copy link
Copy Markdown
Contributor

@tnull tnull Apr 8, 2026

Choose a reason for hiding this comment

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

No, I think we were intentionally following protobuf's/Google's best practices here. https://google.aip.dev/158 states:

  • Response messages for collections should define a string next_page_token field, providing the user with a page token that may be used to retrieve the next page.
    • The field containing pagination results should be the first field in the message and have a field number of 1. It should be a repeated field containing a list of resources constituting a single page of results.
    • If the end of the collection has been reached, the next_page_token field must be empty. This is the only way to communicate "end-of-collection" to users.
    • If the end of the collection has not been reached (or if the API can not determine in time), the API must provide a next_page_token.

IMO would be good to revert and include this context in the docs.

Copy link
Copy Markdown
Contributor Author

@benthecarman benthecarman Apr 8, 2026

Choose a reason for hiding this comment

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

Okay updated

// there is no more data to be retrieved.
//
Expand Down
98 changes: 78 additions & 20 deletions rust/api/src/kv_store_tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,8 @@ macro_rules! define_kv_store_tests {
create_test!(list_should_honour_page_size_and_key_prefix_if_provided);
create_test!(list_should_return_zero_global_version_when_global_versioning_not_enabled);
create_test!(list_should_limit_max_page_size);
create_test!(list_should_return_results_ordered_by_creation_time);
create_test!(list_should_paginate_by_creation_time_with_prefix);
};
}

Expand Down Expand Up @@ -393,12 +395,11 @@ pub trait KvStoreTestSuite {
},
};

if current_page.key_versions.is_empty() {
Comment thread
tnull marked this conversation as resolved.
break;
}

all_key_versions.extend(current_page.key_versions);
next_page_token = current_page.next_page_token;
match current_page.next_page_token {
Some(token) if !token.is_empty() => next_page_token = Some(token),
Comment thread
tnull marked this conversation as resolved.
_ => break,
}
}

if let Some(k1_response) = all_key_versions.iter().find(|kv| kv.key == "k1") {
Expand Down Expand Up @@ -444,13 +445,12 @@ pub trait KvStoreTestSuite {
},
};

if current_page.key_versions.is_empty() {
break;
}

assert!(current_page.key_versions.len() <= page_size as usize);
all_key_versions.extend(current_page.key_versions);
next_page_token = current_page.next_page_token;
match current_page.next_page_token {
Some(token) if !token.is_empty() => next_page_token = Some(token),
_ => break,
}
}

let unique_keys: std::collections::HashSet<String> =
Expand Down Expand Up @@ -490,12 +490,11 @@ pub trait KvStoreTestSuite {
Some(next_page_token) => ctx.list(Some(next_page_token), None, None).await?,
};

if current_page.key_versions.is_empty() {
break;
}

all_key_versions.extend(current_page.key_versions);
next_page_token = current_page.next_page_token;
match current_page.next_page_token {
Some(token) if !token.is_empty() => next_page_token = Some(token),
_ => break,
}
}

let unique_keys: std::collections::HashSet<String> =
Expand All @@ -506,6 +505,66 @@ pub trait KvStoreTestSuite {
Ok(())
}

async fn list_should_return_results_ordered_by_creation_time() -> Result<(), VssError> {
Copy link
Copy Markdown
Contributor

@tnull tnull Apr 20, 2026

Choose a reason for hiding this comment

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

Same here and below, the test names seems inaccurate now that we don't sort by time?

let kv_store = Self::create_store().await;
let ctx = TestContext::new(&kv_store);

ctx.put_objects(Some(0), vec![kv("z_first", "v1", 0)]).await?;
ctx.put_objects(Some(1), vec![kv("a_third", "v1", 0)]).await?;
ctx.put_objects(Some(2), vec![kv("m_second", "v1", 0)]).await?;

let page = ctx.list(None, None, None).await?;
assert_eq!(page.global_version, Some(3));
let keys: Vec<&str> = page.key_versions.iter().map(|kv| kv.key.as_str()).collect();

// Results should be in reverse creation order (newest first), not alphabetical.
assert_eq!(keys, vec!["m_second", "a_third", "z_first"]);
Comment thread
benthecarman marked this conversation as resolved.

Ok(())
}

async fn list_should_paginate_by_creation_time_with_prefix() -> Result<(), VssError> {
let kv_store = Self::create_store().await;
let ctx = TestContext::new(&kv_store);

// Insert prefixed keys in reverse-alphabetical order with a page_size of 1
// to force multiple pages and verify cross-page ordering.
ctx.put_objects(Some(0), vec![kv("pfx_z", "v1", 0)]).await?;
ctx.put_objects(Some(1), vec![kv("pfx_a", "v1", 0)]).await?;
ctx.put_objects(Some(2), vec![kv("other", "v1", 0)]).await?;
ctx.put_objects(Some(3), vec![kv("pfx_m", "v1", 0)]).await?;

let mut next_page_token: Option<String> = None;
let mut all_keys: Vec<String> = Vec::new();

loop {
let current_page = match next_page_token.take() {
None => {
let page = ctx.list(None, Some(1), Some("pfx_".to_string())).await?;
assert_eq!(page.global_version, Some(4));
page
},
Some(token) => {
let page = ctx.list(Some(token), Some(1), Some("pfx_".to_string())).await?;
assert!(page.global_version.is_none());
page
},
};

assert!(current_page.key_versions.len() <= 1);
all_keys.extend(current_page.key_versions.into_iter().map(|kv| kv.key));
match current_page.next_page_token {
Some(token) if !token.is_empty() => next_page_token = Some(token),
Comment thread
benthecarman marked this conversation as resolved.
_ => break,
}
}

// Should get prefixed keys in reverse creation order (newest first), excluding "other".
assert_eq!(all_keys, vec!["pfx_m", "pfx_a", "pfx_z"]);

Ok(())
}

async fn list_should_limit_max_page_size() -> Result<(), VssError> {
let kv_store = Self::create_store().await;
let ctx = TestContext::new(&kv_store);
Expand All @@ -524,16 +583,15 @@ pub trait KvStoreTestSuite {
None => ctx.list(None, None, None).await?,
Some(next_page_token) => ctx.list(Some(next_page_token), None, None).await?,
};
if current_page.key_versions.is_empty() {
break;
}

assert!(
current_page.key_versions.len() < vss_arbitrary_page_size_max as usize,
"Page size exceeds the maximum allowed size"
);
all_key_versions.extend(current_page.key_versions);
next_page_token = current_page.next_page_token;
match current_page.next_page_token {
Some(token) if !token.is_empty() => next_page_token = Some(token),
_ => break,
}
}

assert_eq!(all_key_versions.len(), total_kv_objects as usize);
Expand Down
4 changes: 3 additions & 1 deletion rust/api/src/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -212,14 +212,16 @@ pub struct ListKeyVersionsRequest {
#[allow(clippy::derive_partial_eq_without_eq)]
#[derive(Clone, PartialEq, ::prost::Message)]
pub struct ListKeyVersionsResponse {
/// Fetched keys and versions.
/// Fetched keys and versions, ordered by creation time (newest first).
Copy link
Copy Markdown
Contributor

@tnull tnull Apr 20, 2026

Choose a reason for hiding this comment

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

Also inaccurate now.

/// Even though this API reuses the `KeyValue` struct, the `value` sub-field will not be set by the server.
#[prost(message, repeated, tag = "1")]
pub key_versions: ::prost::alloc::vec::Vec<KeyValue>,
/// `next_page_token` is a pagination token, used to retrieve the next page of results.
/// Use this value to query for next-page of paginated `ListKeyVersions` operation, by specifying
/// this value as the `page_token` in the next request.
///
/// Following AIP-158 (<https://google.aip.dev/158>):
///
/// If `next_page_token` is empty (""), then the "last page" of results has been processed and
/// there is no more data to be retrieved.
///
Expand Down
2 changes: 2 additions & 0 deletions rust/impls/src/migrations.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@ pub(crate) const MIGRATIONS: &[&str] = &[
PRIMARY KEY (user_token, store_id, key)
);",
"ALTER TABLE vss_db DROP CONSTRAINT IF EXISTS vss_db_store_id_check;",
"ALTER TABLE vss_db ADD COLUMN IF NOT EXISTS sort_order BIGSERIAL NOT NULL UNIQUE;",
"CREATE INDEX IF NOT EXISTS idx_vss_db_sort_order ON vss_db (user_token, store_id, sort_order DESC) INCLUDE (key, version);",
];
#[cfg(test)]
pub(crate) const DUMMY_MIGRATION: &str = "SELECT 1 WHERE FALSE;";
1 change: 1 addition & 0 deletions rust/impls/src/postgres/sql/v0_create_vss_db.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ CREATE TABLE vss_db (
version bigint NOT NULL,
created_at TIMESTAMP WITH TIME ZONE,
last_updated_at TIMESTAMP WITH TIME ZONE,
sort_order BIGSERIAL NOT NULL UNIQUE,
PRIMARY KEY (user_token, store_id, key)
);
Loading
Loading