db-based sharding key lookup_query mapping - #1279
Conversation
|
|
||
| fn validate_sharded_tables(&self) -> Result<(), Error> { | ||
| for table in &self.config.sharded_tables { | ||
| if let Some(query) = &table.lookup_query |
There was a problem hiding this comment.
nit: we should use pg_parse_raw::parse here:
- will validate that the query is syntactically valid
- doesn't rely on string interpolation
| // so lookup queries run on the source, which has the complete | ||
| // mapping, and fill the destination's cache, which the row | ||
| // sharder reads. | ||
| let pending = self.copy.pending_lookups(); |
| /// Execute a query with text-format parameters using the extended | ||
| /// protocol and return all rows. Parameters are bound by the server, | ||
| /// so values don't need to be escaped. | ||
| pub async fn fetch_all_params<T: From<DataRow>>( |
There was a problem hiding this comment.
refactor: we should change fetch_all and consequently execute_checked and execute to accept a generic type that resolves to either a Vec<Query> or Vec<"extended protocol request">. The protocol handling below is brittle and needs to re-use existing logic.
| if !pending.is_empty() { | ||
| match lookup::resolve(cluster, pending).await { | ||
| Ok(()) => { | ||
| let router_context = RouterContext::new( |
There was a problem hiding this comment.
nit: I'd probably construct a new router here instead of relying on internals and calling take_pending_lookups() twice.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
| system_catalogs: SystemCatalogsBehavior, | ||
| /// Cached sharding key lookup query results. Scoped here so the | ||
| /// cache is recreated, i.e. emptied, on config reload. | ||
| lookup_cache: LookupCache, |
There was a problem hiding this comment.
I lost track of this one. We want to double check that it lives on the Cluster, i.e., we get a new version of this when we call reload_from_existing or reload.
|
Looks pretty good! I need to read it some more, but architecturally it's sound. Just a few nits around code maintenability / quality, but nothing major/blocking I think. |
| // can't be resolved fails the statement: routing by the | ||
| // untranslated value could put it on the wrong shard. | ||
| if result.is_ok() { | ||
| let pending = self.router.take_pending_lookups(); |
There was a problem hiding this comment.
These state modifications are hard to track. I think we introduced this pattern and should not do it anymore. Maybe we can return these as part of the Route? That one is created for each request, so no concerns about leaking state between calls.
| pub fn parse(&mut self, context: RouterContext) -> Result<Command, Error> { | ||
| // Pending lookups are per-statement; don't leak them into | ||
| // the next statement if this parser is reused. | ||
| self.pending_lookups.clear(); |
There was a problem hiding this comment.
Same comment re: Route as above. We shouldn't leak this between calls like this in the first place.
| } | ||
| }; | ||
|
|
||
| self.pending_lookups.extend(pending_lookups); |
There was a problem hiding this comment.
Maybe move this to QueryParserContext? A new one is created with each request, so data between calls won't leak.
| // The terminator must ship after every data row; | ||
| // hold it back while rows are deferred. | ||
| if !self.deferred.is_empty() { | ||
| self.deferred.push(DeferredRow { |
There was a problem hiding this comment.
Maybe we make this method async instead? I get the defer pattern, but it sure makes things hard to follow. I'm pretty sure (i.e., not sure at all) that this method only gets called in async contexts anyway.
| fn translate_sharding_key(&mut self, table: &ShardedTable, value: &str) -> Option<Arc<str>> { | ||
| let query = table.lookup_query.as_deref()?; | ||
|
|
||
| match self.schema.tables.lookup_cache().get(query, value) { |
There was a problem hiding this comment.
This should really be a lookup by ShardedTable (name, schema, database is the fully-qualified lookup). It's possible (but unlikely) to have two sharded databases with the same lookup query, and different lookup tables, so this could route things incorrectly.
| /// on the source cluster, which has the complete mapping while the | ||
| /// destination is still syncing, and fills the destination's cache, | ||
| /// which the row sharder reads. | ||
| pub(crate) async fn resolve_with( |
There was a problem hiding this comment.
Need to think about this one. Nothing wrong with it, but a lot of that code is duplicated, and I'm thinking we should either move it or add a function to Cluster to execute a query against a shard using round robin.
| #[derive(Debug, Default, Clone)] | ||
| pub struct LookupCache { | ||
| // Keyed by lookup query. | ||
| cache: Arc<DashMap<String, Translations>>, |
There was a problem hiding this comment.
Another thing that came up lately is we really should have memory-bound/unit-bound caching for this kind of stuff. Maybe use https://github.com/moka-rs/moka? We otherwise have the lru crate for a simpler LRU cache, which we could also use here, albeit it won't be sharded, so maybe that's why moka would be a better choice.
|
Oh and before I forget. Metrics! We need to track lookup queries: cache hits and misses and actual cache size + evictions. That one is gonna be an important metric of performance. For cache misses, we should track the latency of lookups. For those, I would follow the cluster-global metrics pattern pgdog/pgdog/src/backend/pool/cluster.rs Line 506 in b2a61dd ... which should be renamed to |

Query-based sharding key lookup for
[[sharded_tables]]Implements the lazy per-key lookup design: a sharded table config can declare a
lookup query that translates an extracted sharding key value into the value
that actually gets hashed. Query returns a row: the result is hashed, and the
translation is cached, keyed by the value. No row: the statement fails with a
retryable error, and nothing is cached.
Semantics
value is translated through the cache first; the result is hashed with the
table's configured hasher and data_type to pick the shard.
one shard, round-robin), then routes with the translated value. The value is
bound with the extended protocol, so it's never spliced into SQL text.
the untranslated value, which could place data on the wrong shard. The main
way this happens in practice is traffic for a key whose row hasn't been
inserted yet; because absence isn't cached, the first statement after the
row appears translates correctly. The query should therefore return a row
for every valid value, including values that translate to themselves
(e.g.
COALESCE(parent_tenant_id, id)gives root tenants a self-row).SchemaCachepattern:it's recreated, and therefore emptied, on config reload. Since only
translations are cached and rows are looked up on first use, a reload is
only needed to pick up changes to existing rows.
mokaand sized by a newsharding_lookup_cache_sizesetting in[general](bytes, default 64MB,per cluster). The weigher counts key and value bytes plus an approximation
of per-entry overhead. Entries are keyed by the sharded table (schema,
name, column) and the value, so identical queries on different tables
can't collide. An evicted translation is simply looked up again on its
next use; the resolve loops retry bounded in case an entry is evicted
between routing attempts.
the statement with a retryable error.
Validation
lookup_queryis validated with the SQL parser at config load: it must besyntactically valid, a single statement, and reference exactly one parameter,
$1. The validation runs in thepgdogcrate, matched to whichever parserthe build uses; linking
pg_raw_parseintopgdog-configunconditionallyturned out to corrupt
pg_querybuilds through duplicate libpg_query Csymbols. The table the query reads must be omnisharded, i.e. hold the same
data on all shards, since the query runs on one shard picked round-robin;
this part is documented rather than validated, because the config doesn't
name the table.
Notes
untranslated value silently splits a tenant family across shards the moment
a lookup races a row insert, and a loud, retryable error seemed strictly
better; happy to flip it (or make it a per-table option) if you prefer the
identity behavior. In my use-case we would have the ability to ensure we
can insert the row before we start using the new sharding key id.
on their own:
ServerRequest, which letsexecute,execute_checkedandfetch_alltake either simple-protocol queries or an extended-protocolbatch (anonymous Parse + Bind + Execute + Sync, parameters bound by the
server), and
Cluster::fetch_all_round_robin, which runs a parameterizedquery on one shard picked round-robin.
are decoded to their text form and translated like text values, reusing the
existing binary decoding in
sharding::Value.resharding a lookup-translated table places rows correctly.
CopyParser::shardis async and resolves missing keys inline, in row order, through the same
resolver routed queries use. Replicated statements resolve their lookups in
StreamContextthe same way. Both text and binary COPY formats are supported, sothe default
resharding_copy_formatworks unchanged. During data sync,lookup queries run on the source cluster, which has the complete mapping
while the destination is still syncing, and fill the destination's cache;
table sync order therefore doesn't matter. During the streaming phase the
destination answers lookups, which works because data sync completed and
the stream itself keeps the mapping current; a mapping row and rows keyed
by it written in the same source transaction would fail the stream, which
the insert-the-mapping-first application pattern avoids.
statement before erroring, since absence isn't cached. Complete mappings
(a row per valid value) avoid this entirely.