From 76a121ecaf4c3b44b2824430c7a644d397433a38 Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Fri, 26 Aug 2022 17:58:42 -0400 Subject: [PATCH] sql: SHOW QUERIES doesn't interpolate placeholders Previously, SHOW QUERIES (and crdb_internal.node_queries, cluster_queries) interpolated placeholder values into the statement so that it was possible to see the placeholder values of a prepared statement. However, this was an expensive process that spends a lot of CPU for little reason, since the interpolation was happening in the hot path of every query. Now, we include the placeholder values as a separate array in these tables, to avoid the runtime interpolation costs. As a side effect of this change, the original comments in a query are now inclued in SHOW QUERIES and the two active queries tables. Release note (sql change): the query field in the crdb_internal.node_queries, crdb_internal.cluster_queries, and SHOW QUERIES commands no longer interpolates placeholders. The placeholders are now included as a SQL text array in the new placeholders column of those tables. Comments from queries are also now included in the aforementioned query tables. --- pkg/server/serverpb/status.proto | 3 +++ pkg/sql/conn_executor.go | 17 +++++++++++------ pkg/sql/conn_executor_exec.go | 18 ++++++++---------- pkg/sql/crdb_internal.go | 9 +++++++++ pkg/sql/delegate/show_queries.go | 1 + pkg/sql/exec_util.go | 19 +++++-------------- 6 files changed, 37 insertions(+), 30 deletions(-) diff --git a/pkg/server/serverpb/status.proto b/pkg/server/serverpb/status.proto index 54504e884ef1..6eb23ea0164d 100644 --- a/pkg/server/serverpb/status.proto +++ b/pkg/server/serverpb/status.proto @@ -905,6 +905,9 @@ message ActiveQuery { // field is only valid if the query is in the EXECUTING phase. bool is_full_scan = 10; + // The placeholders if any. + repeated string placeholders = 11; + } // Request object for ListSessions and ListLocalSessions. diff --git a/pkg/sql/conn_executor.go b/pkg/sql/conn_executor.go index c5797c07dd0a..82f6f640892f 100644 --- a/pkg/sql/conn_executor.go +++ b/pkg/sql/conn_executor.go @@ -3153,12 +3153,16 @@ func (ex *connExecutor) serialize() serverpb.Session { if query.hidden { continue } - ast, err := query.getStatement() - if err != nil { - continue + sqlNoConstants := truncateSQL(formatStatementHideConstants(query.stmt.AST)) + nPlaceholders := 0 + if query.placeholders != nil { + nPlaceholders = len(query.placeholders.Values) + } + placeholders := make([]string, nPlaceholders) + for i := range placeholders { + placeholders[i] = tree.AsStringWithFlags(query.placeholders.Values[i], tree.FmtPgwireText) } - sqlNoConstants := truncateSQL(formatStatementHideConstants(ast)) - sql := truncateSQL(ast.String()) + sql := truncateSQL(query.stmt.SQL) progress := math.Float64frombits(atomic.LoadUint64(&query.progressAtomic)) activeQueries = append(activeQueries, serverpb.ActiveQuery{ TxnID: query.txnID, @@ -3166,7 +3170,8 @@ func (ex *connExecutor) serialize() serverpb.Session { Start: query.start.UTC(), Sql: sql, SqlNoConstants: sqlNoConstants, - SqlSummary: formatStatementSummary(ast), + SqlSummary: formatStatementSummary(query.stmt.AST), + Placeholders: placeholders, IsDistributed: query.isDistributed, Phase: (serverpb.ActiveQuery_Phase)(query.phase), Progress: float32(progress), diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index ade924808a02..b7b868a895e5 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -301,15 +301,9 @@ func (ex *connExecutor) execStmtInOpenState( // doneAfterFunc will be allocated only when timeoutTicker is non-nil. var doneAfterFunc chan struct{} - // Early-associate placeholder info with the eval context, - // so that we can fill in placeholder values in our call to addActiveQuery, below. - if !ex.planner.EvalContext().HasPlaceholders() { - ex.planner.EvalContext().Placeholders = pinfo - } - var cancelQuery context.CancelFunc ctx, cancelQuery = contextutil.WithCancel(ctx) - ex.addActiveQuery(ast, formatWithPlaceholders(ast, ex.planner.EvalContext()), queryID, cancelQuery) + ex.addActiveQuery(parserStmt, pinfo, queryID, cancelQuery) // Make sure that we always unregister the query. It also deals with // overwriting res.Error to a more user-friendly message in case of query @@ -2046,13 +2040,17 @@ func (ex *connExecutor) enableTracing(modes []string) error { // addActiveQuery adds a running query to the list of running queries. func (ex *connExecutor) addActiveQuery( - ast tree.Statement, rawStmt string, queryID clusterunique.ID, cancelQuery context.CancelFunc, + stmt parser.Statement, + placeholders *tree.PlaceholderInfo, + queryID clusterunique.ID, + cancelQuery context.CancelFunc, ) { - _, hidden := ast.(tree.HiddenFromShowQueries) + _, hidden := stmt.AST.(tree.HiddenFromShowQueries) qm := &queryMeta{ txnID: ex.state.mu.txn.ID(), start: ex.phaseTimes.GetSessionPhaseTime(sessionphase.SessionQueryReceived), - rawStmt: rawStmt, + stmt: stmt, + placeholders: placeholders, phase: preparing, isDistributed: false, isFullScan: false, diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index d8a7ee973a36..8899b81670ce 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -1758,6 +1758,7 @@ CREATE TABLE crdb_internal.%s ( user_name STRING, -- the user running the query start TIMESTAMP, -- the start time of the query query STRING, -- the SQL code of the query + placeholders STRING[], -- the placeholders, if any, passed for the query client_address STRING, -- the address of the client that issued the query application_name STRING, -- the name of the application as per SET application_name distributed BOOL, -- whether the query is running distributed @@ -1890,6 +1891,13 @@ func populateQueriesTable( if err != nil { return err } + placeholders := tree.NewDArray(types.String) + placeholders.Array = make(tree.Datums, 0, len(query.Placeholders)) + for _, v := range query.Placeholders { + if err := placeholders.Append(tree.NewDString(v)); err != nil { + return err + } + } if err := addRow( tree.NewDString(query.ID), txnID, @@ -1898,6 +1906,7 @@ func populateQueriesTable( tree.NewDString(session.Username), ts, tree.NewDString(query.Sql), + placeholders, tree.NewDString(session.ClientAddress), tree.NewDString(session.ApplicationName), isDistributedDatum, diff --git a/pkg/sql/delegate/show_queries.go b/pkg/sql/delegate/show_queries.go index ee69334ea547..56d1c54a2955 100644 --- a/pkg/sql/delegate/show_queries.go +++ b/pkg/sql/delegate/show_queries.go @@ -26,6 +26,7 @@ SELECT user_name, start, query, + placeholders, client_address, application_name, distributed, diff --git a/pkg/sql/exec_util.go b/pkg/sql/exec_util.go index 5a9e2ff8dc3e..3c1d5cd44c8a 100644 --- a/pkg/sql/exec_util.go +++ b/pkg/sql/exec_util.go @@ -1933,10 +1933,11 @@ type queryMeta struct { // The timestamp when this query began execution. start time.Time - // The string of the SQL statement being executed. This string may - // contain sensitive information, so it must be converted back into - // an AST and dumped before use in logging. - rawStmt string + // The SQL statement being executed. + stmt parser.Statement + + // The placeholders that the query was executed with if any. + placeholders *tree.PlaceholderInfo // States whether this query is distributed. Note that all queries, // including those that are distributed, have this field set to false until @@ -1971,16 +1972,6 @@ func (q *queryMeta) cancel() { q.cancelQuery() } -// getStatement returns a cleaned version of the query associated -// with this queryMeta. -func (q *queryMeta) getStatement() (tree.Statement, error) { - parsed, err := parser.ParseOne(q.rawStmt) - if err != nil { - return nil, err - } - return parsed.AST, nil -} - // SessionDefaults mirrors fields in Session, for restoring default // configuration values in SET ... TO DEFAULT (or RESET ...) statements. type SessionDefaults map[string]string