feat: put read replicas to use#1012
Conversation
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
There was a problem hiding this comment.
There are some opinionated changes from my end in this file, so feel free to ask for a revert.
We basically had defined wrapper functions for the actual NIF functions, which added basic runtime validation, basically aiming to be the public representation of the underlying NIF (see statements vs statement_types). I have extended that pattern for the new parse & parse_to_json functions, made the NIFs to be private, such that now it exposes the public variants only.
I feel it's a cleaner layout, let me know your thoughts.
There was a problem hiding this comment.
This is my main point of discussion: how conservative should we be?
The current implementation allows for all read-only SELECT statements, and read-only BEGIN statements (transaction start), but stays conservative on function calls. Function calls in general shouldn't be write-safe, but should we have an allowlist for in-built functions like SUM, COUNT, AVG?
Chose to stay conservative for now, as it's safer to be. WDYT?
There was a problem hiding this comment.
I should explain myself here:
I decided to call libpg_query's C function directly for the parse function (pg_query_parse) because the rust wrapper doesn't expose a JSON serializer yet, expecting us to deal with protobuf directly. And to clarify, serde::Serialize derives WAS added in their codebase (pganalyze/pg_query.rs@123d448), which would let us do serde_json::to_string(&result.protobuf) directly, but is yet to be released upstream. Once released, we can refactor to Rust-only.
Or we can pin the crate to the above GitHub commit now as well. I just didn't want to mess with a stable dep. WDYT?
|
@v0idpwn @mentels I have implemented a module for classifying read-safe SQL queries as of now. This will be the first step where we decide when exactly to route, before we implement the actual routing itself. Will move to that implementation once I have feedback on this and a direction for the routing. |
|
Hi, @Snehil-Shah. It will take us some time to actually review this, since we have other priorities. |
|
@v0idpwn Totally fine, thanks for the heads up. I'll keep iterating on this in the meantime. |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
- earlier logic only checked the first returned tenant for active state, it should simply just return only active tenants - also a drive by clarity improvement for adding a no_users error Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
| @spec maybe_checkin(:transaction | :session | :proxy, Data.db_connection()) :: | ||
| Data.db_connection() | ||
| defp maybe_checkin(:transaction, nil), do: nil | ||
|
|
||
| defp maybe_checkin(:transaction, pool, {_, db_pid, _}) do | ||
| defp maybe_checkin(:transaction, {pool, db_pid, _}) do | ||
| Process.unlink(db_pid) | ||
| :poolboy.checkin(pool, db_pid) | ||
| nil | ||
| end | ||
|
|
||
| defp maybe_checkin(:session, _, db_connection), do: db_connection | ||
| defp maybe_checkin(:proxy, _, db_connection), do: db_connection | ||
| defp maybe_checkin(:session, db_connection), do: db_connection | ||
| defp maybe_checkin(:proxy, db_connection), do: db_connection |
There was a problem hiding this comment.
db_connection already has the pool it was sourced from, and we were incorrectly passing the entire map of pools in cluster mode before. I just decided to remove the explicit pool argument. The function, imo, reads better now as "checkin the given connection back to its source pool".
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
| @doc """ | ||
| Returns `true` if the SQL query is read-safe. | ||
| """ | ||
| @spec read_safe?(String.t()) :: boolean() |
There was a problem hiding this comment.
One thing to flag here is that this is only a guarantee at the AST level. If their DB has some really weird configuration (TIL: I found out about these rare cases today itself), like views with function calls in their definition, so every read from the view calls a function.. or someone made a custom operator symbol of their own that does a write operation (it would appear as any other operator like + etc).. or someone wrote an RLS policy that checks by writing data somewhere on every read (ik, doesn't make sense). All these cases are very unusual but undetectable at an AST level.
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
I went ahead and added the routing logic for clustered tenants. It routes read-safe queries to randomly picked read replicas. I have left various comments highlighting my decisions and questions throughout. Once the implementation is validated, I'll move to telemetry and some integration tests for this. |
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
| Telem.pool_checkout_time( | ||
| System.monotonic_time(:microsecond) - start, | ||
| data.id, | ||
| same_box, | ||
| replica_type | ||
| ) |
There was a problem hiding this comment.
Should this metric include the time spent in the actual pool selection process? (query parsing, classification). Should that be a separate metric?
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
| @spec client_query_time(integer(), Supavisor.id(), boolean(), :read | :write) :: :ok | nil | ||
| def client_query_time(start, Supavisor.id() = id, proxy, query_type) do | ||
| telemetry_execute( | ||
| [:supavisor, :client, :query, :stop], | ||
| %{duration: System.monotonic_time() - start}, | ||
| Map.put(id_to_tags(id), :proxy, proxy) | ||
| id_to_tags(id) | ||
| |> Map.put(:proxy, proxy) | ||
| |> Map.put(:query_type, query_type) | ||
| ) |
There was a problem hiding this comment.
I think this can be a cool metric to include. Can be insightful for read vs write query comparisons.
Signed-off-by: Snehil Shah <snehilshah.989@gmail.com>
|
@v0idpwn I have added integration tests and I think this PR is now ready for review. This only implements routing for simple queries in transaction mode to begin with. I have started working on extended queries too, but I would limit this PR to simple queries only, so we can work on easy-to-review chunks incrementally. Also, no hurries :) |
2 participants
What kind of change does this PR introduce?
Aims to put read replicas to use.
What is the current behavior?
Cluster routing (the postgres.cluster.alias connection path) is broken, and doesn't work even if the cluster has one replica.
What is the new behavior?
Additional context
We had some precedence in this very repo in #162, which was removed in a future refactor. But it had a naive logic of routing all select statements to any random replica.