sql: include placeholder values in SHOW QUERIES #66689
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, 2 of 2 files at r2.
Reviewable status:complete! 1 of 0 LGTMs obtained (waiting on @matthewtodd)
pkg/sql/conn_executor_exec.go, line 737 at r2 (raw file):
d, err := placeholder.Eval(evalCtx) if err != nil { // fall back to the default behavior if something goes wrong
super nit: Comments should be sentences.
pkg/sql/show_test.go, line 636 at r2 (raw file):
} // #61569 sql: placeholder values for prepared statements should be visible in SHOW QUERIES
My understanding of the party line is that you shouldn't need to follow github issues to understand the context. It's okay to reference them but I think it's unusual to lead with them. Also, sentences for comments (sorry, I know it's nitpicky).
pkg/sql/show_test.go, line 654 at r2 (raw file):
// Only observe queries when we're in an application session, // to limit concurrent access to the recordedQueries map. if session.ApplicationName == "application" {
nit: define "application" as a constant and then use it here and elsewhere -- then, in the queries, you can use it as a placeholder?
There was a problem hiding this comment.
Also, 👋 and welcome!
Reviewable status:
This allows test code to make decisions based on, for example, the session id or the application name. Release note: none
There was a problem hiding this comment.
👋 Thanks for looking, @ajwerner!
Reviewable status:
pkg/sql/conn_executor_exec.go, line 737 at r2 (raw file):
Previously, ajwerner wrote…
super nit: Comments should be sentences.
Done.
There was a problem hiding this comment.
This is awesome! 🎉
Reviewable status:
pkg/sql/show_test.go, line 742 at r4 (raw file):
actual := recordedQueries[test.statement] if actual != test.expected {
nit: I think we can use require.Equal here
Resolves #61569 Given this test script, running against a `cockroach demo` cluster: ```ruby require "pg" c = PG::Connection.new(host: "localhost", port: 26257, user: "root") c.prepare("test", "SELECT pg_sleep(20), upper($1)") c.exec_prepared("test", ["hello"]) c.close ``` Here's what we would see before this change, in particular the $1 as the argument to `upper()`: ``` > select query from [show queries] where application_name like '%run%'; query ---------------------------------- SELECT pg_sleep(20), upper($1) (1 row) ``` And here's what we now see, the string 'hello' filled in: ``` > select query from [show queries] where application_name like '%run%'; query --------------------------------------- SELECT pg_sleep(20), upper('hello') (1 row) ``` Release note (sql change): The SHOW QUERIES command was extended for prepared statements to show the actual values in use at query time, rather than the previous $1, $2, etc., placeholders. We expect showing these values will greatly improve the experience of debugging slow queries.
There was a problem hiding this comment.
Reviewable status:
pkg/sql/show_test.go, line 742 at r4 (raw file):
Previously, Azhng (Archer Zhang) wrote…
nit: I think we can use
require.Equalhere
Awesome, I was wondering about that! Done.
|
bors r+ |
|
Build succeeded: |
Resolves #61569
Given this test script, running against a
cockroach democluster:Here's what we would see before this change, in particular the $1 as the argument to
upper():And here's what we now see, the string 'hello' filled in:
Release note (sql change): The SHOW QUERIES command was extended for prepared statements to show the actual values in use at query time, rather than the previous $1, $2, etc., placeholders.
We expect showing these values will greatly improve the experience of debugging slow queries.