Skip to content

kv: support computation of index field#228

Merged
nrc merged 1 commit into
mainfrom
npry/kv.compute_idx
Jun 17, 2026
Merged

kv: support computation of index field#228
nrc merged 1 commit into
mainfrom
npry/kv.compute_idx

Conversation

@npry

@npry npry commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Allow specifying a closure to compute indices for a given value. By default, it just returns Some(value.clone()) (the current behavior), but you can return None if the value doesn't have a key for that index (such as Node::disco_key, which is optional) or multiple values if they exist and you know they're unique (such as the peer's tailnet ipv4 and ipv6, which are necessarily disjoint).

@npry npry force-pushed the npry/kv.compute_idx branch 2 times, most recently from fe6e39f to a3d6cbf Compare June 7, 2026 00:13
@npry npry marked this pull request as ready for review June 7, 2026 00:14

@nrc nrc left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good, thanks! Couple of questions inline

Comment thread ts_kv_store/src/schema.rs
/// computed by the specified closure, which is expected to return
/// `impl IntoIterator<Item = I>` (where `I` is the index key type). Most commonly, this
/// will likely be `Option`: you might want to return `None` in some cases if the value has
/// an optional field or an index key can't be produced.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is the use of IntoIterator future-proofing for multiple secondary keys per primary key (which I hadn't thought of, but I guess might be useful as well as multiple primary keys per secondary)? If not, then could we use Option rather than IntoIterator?

@npry npry Jun 9, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The salient case in Peers is that there's an index on both the tailnet IPv4 and IPv6, which are both represented naturally as just IpAddr and are disjoint, so they can live in the same index. It's not like it's terribly onerous to just split them and use Option — if you'd prefer that I'm fine with it until we see a compelling need for a large or variable number of provably unique index keys

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So there's one index IpAddr -> Peer where there might be 0, 1, or 2 IpAddrs per peer?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's always 2 in this case

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We discussed this in a meeting last week. The use case is that the index is IpAddr -> Peer where each Peer has two IpAddr fields (4 and 6). The use of iterator supports this (or arbitrary numbers of foreign keys). Option is used for the 0 or 1 case and is simply converted into an iterator. In the future we want to support a foreign key mapping to multiple values as well as multiple keys mapping to one value, so we might need to revisit this design as part of that (for the peer use case, having separate indexes for ipv4 and 6 would also work but would be a bit less optimal).

Comment thread ts_kv_store/src/schema.rs
@npry npry force-pushed the npry/kv.compute_idx branch 4 times, most recently from 39b98e6 to 0a2f293 Compare June 15, 2026 07:30
@nrc nrc mentioned this pull request Jun 16, 2026
11 tasks
@npry npry force-pushed the npry/kv.compute_idx branch from 0a2f293 to df131ff Compare June 16, 2026 17:20
Signed-off-by: Nathan Perry <nathan@tailscale.com>
Change-Id: I2f06ff801e32df800336175bc31d7b956a6a6964
@npry npry force-pushed the npry/kv.compute_idx branch from df131ff to 091fd97 Compare June 16, 2026 20:05
@nrc nrc merged commit de40ca6 into main Jun 17, 2026
28 checks passed
@nrc nrc deleted the npry/kv.compute_idx branch June 17, 2026 01:06
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.

2 participants