Conversation
|
I've assigned @tankyleo as a reviewer! |
|
Oops, looks like the new |
tankyleo
left a comment
There was a problem hiding this comment.
Thanks again sorry for the delay, feel free to rebase.
ba56010 to
b4c596c
Compare
|
|
||
| let statement = client.prepare(query).await?; | ||
| self.statements.insert(query, statement.clone()); | ||
| Ok(statement) |
There was a problem hiding this comment.
Reusing the statement here can cause postgres to choose and reuse a generic plan that can perform poorly with varying parameters on our list queries.
From this documentation: https://www.postgresql.org/docs/16/sql-prepare.html
"A prepared statement can be executed with either a generic plan or a custom plan. A generic plan is the same across all executions, while a custom plan is generated for a specific execution using the parameter values given in that call. Use of a generic plan avoids planning overhead, but in some situations a custom plan will be much more efficient to execute because the planner can make use of knowledge of the parameter values. (Of course, if the prepared statement has no parameters, then this is moot and a generic plan is always used.)
By default (that is, when plan_cache_mode is set to auto), the server will automatically choose whether to use a generic or custom plan for a prepared statement that has parameters. The current rule for this is that the first five executions are done with custom plans and the average estimated cost of those plans is calculated. Then a generic plan is created and its estimated cost is compared to the average custom-plan cost. Subsequent executions use the generic plan if its cost is not so much higher than the average custom-plan cost as to make repeated replanning seem preferable."
Let me know what you think.
There was a problem hiding this comment.
did some tests locally, changing the plan_cache_mode to either custom or generic was markedly slower (~10-40%, and like 10x slower for generic on list endpoints), since we have to re-plan each query each time. My guess is that in most normal VSS instances, the query planner will do the right thing.
I think the actual problem is the global UNIQUE index on sort_order BIGSERIAL, which very rarely leads the query planner astray (as in the example you posted elsewhere). BIGSERIAL already creates a SEQUENCE object which guarantees the sort_order is unique, without adding an additional per-insert cost of maintaining the UNIQUE index.
It really doesn't make sense to me to maintain a global index on sort_order, since different values are meaningless across different namespaces. In any VSS server with >1 user or >1 namespace, I don't see how the sort_order UNIQUE index ever helps outside of some extreme edge case.
IMO we should just remove the UNIQUE index unless there's some real pressing reason to keep it.
So I can `POSTGRES_ENDPOINT='postgresql://%2Frun%2Fuser%2F1000' cargo test` against my user-local, socket-activated dev postgres.
Without this change, we would re-parse and re-plan each query, potentially multiple times per request (!). Instead, maintain a small prepared statement cache per connection. We'll lazily prepare a query statement the first time we execute it, then use the prepared statement thereafter. This change reduces latency by ~20-60% on my small benchmark suite, but is particularly impactful on batch conditional updates, where we were previously re-parsing and re-planning the put query for each item in the batch. This change improved throughput there by ~1.5x (~4.8k/s -> 12.3k/s).
b4c596c to
e028e0d
Compare
Without this change, we would re-parse and re-plan each query, potentially multiple times per request (!). For simple queries, this appears to add ~100-200 us overhead. More for more complex queries.
Instead, maintain a small prepared statement cache per connection. We'll lazily prepare a query statement the first time we execute it, then use the prepared statement thereafter.
This change reduces latency by ~20-60% on my small benchmark suite, but is particularly impactful on batch conditional updates, where we were previously re-parsing and re-planning the
putquery for each item in the batch. This change improved throughput there by ~2.5x (~4.8k/s -> 12.3k/s).Using const
&'static strqueries is really more a stylistic preference on my part. I think it adds a nice roadblock to prevent people from accidentally addinglet stmt = format!("..", untrusted_user_input)-> SQL injection.Migrations and other admin queries are still uncached, since they're usually only executed once.