From 477d1e84a6f6e734a758d30564d4aad2a01a672d Mon Sep 17 00:00:00 2001 From: Marylia Gutierrez Date: Wed, 10 Aug 2022 13:07:24 -0400 Subject: [PATCH 1/2] sql: fix log spam during idx recommendation if txn was closed Previously, if a transaction (from planner.txn) was already closed, due to one-phase commit optimization or an error, we were still trying to generate a recommendation, causing log spam. This commit now checks if we have a transaction open that can be used, otherwise it won't try to generate recommendation. This commit also updates the cache, even if the recommendation generations fails, this way we won't keep trying constantly, and instead will wait for the next hour to try again. Fixes #85875 Release note: None --- pkg/sql/conn_executor_exec.go | 9 +- .../idx_recommendations_cache_test.go | 83 ++++++++++++------- pkg/sql/instrumentation.go | 30 +++---- 3 files changed, 70 insertions(+), 52 deletions(-) diff --git a/pkg/sql/conn_executor_exec.go b/pkg/sql/conn_executor_exec.go index 6a569888fb48..c279c15deed2 100644 --- a/pkg/sql/conn_executor_exec.go +++ b/pkg/sql/conn_executor_exec.go @@ -1197,8 +1197,13 @@ func (ex *connExecutor) dispatchToExecutionEngine( populateQueryLevelStats(ctx, planner) - // Set index recommendations so it can be saved on statement statistics. - planner.instrumentation.SetIndexRecommendations(ctx, ex.server.idxRecommendationsCache, planner) + // The transaction (from planner.txn) may already have been committed at this point, + // due to one-phase commit optimization or an error. Since we use that transaction + // on the optimizer, check if is still open before generating index recommendations. + if planner.txn.IsOpen() { + // Set index recommendations, so it can be saved on statement statistics. + planner.instrumentation.SetIndexRecommendations(ctx, ex.server.idxRecommendationsCache, planner) + } // Record the statement summary. This also closes the plan if the // plan has not been closed earlier. diff --git a/pkg/sql/idxrecommendations/idx_recommendations_cache_test.go b/pkg/sql/idxrecommendations/idx_recommendations_cache_test.go index f2565b0a855e..76ec199c9e5a 100644 --- a/pkg/sql/idxrecommendations/idx_recommendations_cache_test.go +++ b/pkg/sql/idxrecommendations/idx_recommendations_cache_test.go @@ -38,44 +38,63 @@ func TestIndexRecommendationsStats(t *testing.T) { testConn := sqlutils.MakeSQLRunner(sqlConn) testConn.Exec(t, "CREATE DATABASE idxrectest") testConn.Exec(t, "USE idxrectest") - testConn.Exec(t, "CREATE TABLE t ( k INT PRIMARY KEY, v INT, FAMILY \"primary\" (k, v))") - testConn.Exec(t, "CREATE TABLE t1 (k INT, i INT, f FLOAT, s STRING)") - testConn.Exec(t, "CREATE TABLE t2 (k INT, i INT, s STRING)") - testConn.Exec(t, "CREATE UNIQUE INDEX existing_t1_i ON t1(i)") + minExecutions := 5 + var recommendations string - testCases := []struct { - stmt string - fingerprint string - recommendations string - }{ - { - stmt: "SELECT * FROM t WHERE v > 123", - fingerprint: "SELECT * FROM t WHERE v > _", - recommendations: "{\"creation : CREATE INDEX ON t (v);\"}", - }, - { - stmt: "SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE t1.i > 3 AND t2.i > 3", - fingerprint: "SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE (t1.i > _) AND (t2.i > _)", - recommendations: "{\"replacement : CREATE UNIQUE INDEX ON t1 (i) STORING (k); DROP INDEX t1@existing_t1_i;\"," + - "\"creation : CREATE INDEX ON t2 (i) STORING (k);\"}", - }, - } + t.Run("index recommendations generated", func(t *testing.T) { + testConn.Exec(t, "CREATE TABLE t ( k INT PRIMARY KEY, v INT, FAMILY \"primary\" (k, v))") + testConn.Exec(t, "CREATE TABLE t1 (k INT, i INT, f FLOAT, s STRING)") + testConn.Exec(t, "CREATE TABLE t2 (k INT, i INT, s STRING)") + testConn.Exec(t, "CREATE UNIQUE INDEX existing_t1_i ON t1(i)") - var recommendations string - for i := 0; i < 8; i++ { - for _, tc := range testCases { - testConn.Exec(t, tc.stmt) + testCases := []struct { + stmt string + fingerprint string + recommendations string + }{ + { + stmt: "SELECT * FROM t WHERE v > 123", + fingerprint: "SELECT * FROM t WHERE v > _", + recommendations: "{\"creation : CREATE INDEX ON t (v);\"}", + }, + { + stmt: "SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE t1.i > 3 AND t2.i > 3", + fingerprint: "SELECT t1.k FROM t1 JOIN t2 ON t1.k = t2.k WHERE (t1.i > _) AND (t2.i > _)", + recommendations: "{\"replacement : CREATE UNIQUE INDEX ON t1 (i) STORING (k); DROP INDEX t1@existing_t1_i;\"," + + "\"creation : CREATE INDEX ON t2 (i) STORING (k);\"}", + }, + } + + for i := 0; i < (minExecutions + 2); i++ { + for _, tc := range testCases { + testConn.Exec(t, tc.stmt) + rows := testConn.QueryRow(t, "SELECT index_recommendations FROM CRDB_INTERNAL.STATEMENT_STATISTICS "+ + " WHERE metadata ->> 'db' = 'idxrectest' AND metadata ->> 'query'=$1", tc.fingerprint) + rows.Scan(&recommendations) + + expected := tc.recommendations + if i < minExecutions { + expected = "{}" + } + require.Equal(t, expected, recommendations) + } + } + }) + + t.Run("index recommendations not generated for one-phase commit "+ + "optimization statement", func(t *testing.T) { + testConn.Exec(t, "CREATE TABLE t3 (k INT PRIMARY KEY)") + + for i := 0; i < (minExecutions + 2); i++ { + testConn.Exec(t, `INSERT INTO t3 VALUES($1)`, i) rows := testConn.QueryRow(t, "SELECT index_recommendations FROM CRDB_INTERNAL.STATEMENT_STATISTICS "+ - " WHERE metadata ->> 'db' = 'idxrectest' AND metadata ->> 'query'=$1", tc.fingerprint) + " WHERE metadata ->> 'db' = 'idxrectest' AND metadata ->> 'query' = 'INSERT INTO t3 VALUES ($1)'") rows.Scan(&recommendations) - expected := tc.recommendations - if i < 5 { - expected = "{}" - } - require.Equal(t, expected, recommendations) + require.Equal(t, "{}", recommendations) } - } + + }) } func TestFormatIdxRecommendations(t *testing.T) { diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go index 60ec3283a675..a293758fb781 100644 --- a/pkg/sql/instrumentation.go +++ b/pkg/sql/instrumentation.go @@ -684,6 +684,8 @@ func (ih *instrumentationHelper) SetIndexRecommendations( opc.reset(ctx) stmtType := opc.p.stmt.AST.StatementType() + reset := false + var recommendations []string if idxRec.ShouldGenerateIndexRecommendation( ih.fingerprint, ih.planGist.Hash(), @@ -713,25 +715,17 @@ func (ih *instrumentationHelper) SetIndexRecommendations( err = opc.makeQueryIndexRecommendation(ctx) if err != nil { log.Warningf(ctx, "unable to generate index recommendations: %s", err) - } else { - idxRec.UpdateIndexRecommendations( - ih.fingerprint, - ih.planGist.Hash(), - planner.SessionData().Database, - stmtType, - ih.indexRecommendations, - true, - ) } } - } else { - ih.indexRecommendations = idxRec.UpdateIndexRecommendations( - ih.fingerprint, - ih.planGist.Hash(), - planner.SessionData().Database, - stmtType, - []string{}, - false, - ) + reset = true + recommendations = ih.indexRecommendations } + ih.indexRecommendations = idxRec.UpdateIndexRecommendations( + ih.fingerprint, + ih.planGist.Hash(), + planner.SessionData().Database, + stmtType, + recommendations, + reset, + ) } From 5391bb2e440f6cbeacd5bfde2e2bb61525827549 Mon Sep 17 00:00:00 2001 From: Aditya Maru Date: Wed, 10 Aug 2022 15:40:19 -0400 Subject: [PATCH 2/2] sql: use the correct txn when creating external connection We need to use the planners' txn when creating an external connection. Using a DB().Txn here was an oversight. Release note: None --- pkg/sql/create_external_connection.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pkg/sql/create_external_connection.go b/pkg/sql/create_external_connection.go index af34adf6547a..86ad966d4c13 100644 --- a/pkg/sql/create_external_connection.go +++ b/pkg/sql/create_external_connection.go @@ -15,7 +15,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/cloud/externalconn" "github.com/cockroachdb/cockroach/pkg/clusterversion" - "github.com/cockroachdb/cockroach/pkg/kv" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" @@ -111,9 +110,7 @@ func (p *planner) createExternalConnection( // Create the External Connection and persist it in the // `system.external_connections` table. - return params.ExecCfg().DB.Txn(params.ctx, func(ctx context.Context, txn *kv.Txn) error { - return ex.Create(params.ctx, params.ExecCfg().InternalExecutor, params.p.User(), txn) - }) + return ex.Create(params.ctx, params.ExecCfg().InternalExecutor, params.p.User(), p.Txn()) } func (c *createExternalConectionNode) Next(_ runParams) (bool, error) { return false, nil }