fix(cross_shard_disabled): provide more specific error msg when dealing with unknown sharding key - #1365
fix(cross_shard_disabled): provide more specific error msg when dealing with unknown sharding key#1365jkaczman wants to merge 6 commits into
Conversation
…ng with unknown sharding key
Co-authored-by: Lev Kokotov <levkk@users.noreply.github.com>
| let query = context.client_request.query()?; | ||
| let error = ErrorResponse::cross_shard_disabled(query.as_ref().map(|q| q.query())); | ||
|
|
||
| let error = if let Some(sharding_key) = context.params.get("pgdog.sharding_key") |
There was a problem hiding this comment.
I think this is fine for now. A better solution could be to check this inside the parser:
- If it returns
Shard::All - and
pgdog.sharding_keyis set in params (those are available in the context, see ) - and cross-shard queries are disabled (this may be currently missing from the context but can be easily added I think: )
then the parser can return this error, which we can catch and send to the client.
The reason this is better is because it avoids connecting to Postgres when we know we can't serve that query because of the missing sharding key mapping.
There was a problem hiding this comment.
Another reason why the parser could be a better place for this is because it'll also catch the comment directive:
/* pgdog_sharding_key: 25 */ SELECT * FROM users;while this check will only catch the pgdog.sharding_key SET parameter.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
bcc4437 to
a8a56ce
Compare
a8a56ce to
6c8dbac
Compare
levkk
left a comment
There was a problem hiding this comment.
Awesome! We are going to need to organize the checks a little bit next week; it's getting a bit complex. But this looks good!
Provides a specific error message "unknown sharding key was specified" as opposed to "cross-shard queries are disabled" when a query:
Fixes #1163