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
cockroachdb · Aug 10, 2022 · 477d1e8 · 477d1e8
1 parent 65fe1a5
commit 477d1e8
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 52 deletions.
diff --git 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
@@ -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
@@ -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,
+	)
 }