Merge pull request #124268 from yuzefovich/backport23.2-123793-123794

release-23.2: sql: improve env collection in stmt bundles
cockroachdb · May 20, 2024 · 7cec751 · 7cec751
2 parents 93deb8f + 69d118c
commit 7cec751
Show file tree

Hide file tree

Showing 9 changed files with 294 additions and 191 deletions.
diff --git a/pkg/sql/explain_bundle.go b/pkg/sql/explain_bundle.go
diff --git a/pkg/sql/explain_bundle_test.go b/pkg/sql/explain_bundle_test.go
@@ -24,7 +24,9 @@ import (
 	"time"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/kv"
 	"github.com/cockroachdb/cockroach/pkg/kv/kvserver"
+	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
@@ -597,3 +599,50 @@ SELECT a || $1 FROM t WHERE k = ($2 - $10);
 		require.Equal(t, tc.numPlaceholders, p)
 	}
 }
+
+// TestExplainBundleEnv is a sanity check that all SET and SET CLUSTER SETTING
+// statements in the env.sql file of the bundle are valid.
+func TestExplainBundleEnv(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	srv, sqlDB, db := serverutils.StartServer(t, base.TestServerArgs{})
+	defer srv.Stopper().Stop(ctx)
+	s := srv.ApplicationLayer()
+
+	execCfg := s.ExecutorConfig().(ExecutorConfig)
+	sd := NewInternalSessionData(ctx, execCfg.Settings, "test")
+	internalPlanner, cleanup := NewInternalPlanner(
+		"test",
+		kv.NewTxn(ctx, db, srv.NodeID()),
+		username.RootUserName(),
+		&MemoryMetrics{},
+		&execCfg,
+		sd,
+	)
+	defer cleanup()
+	p := internalPlanner.(*planner)
+	c := makeStmtEnvCollector(ctx, p, s.InternalExecutor().(*InternalExecutor))
+
+	var sb strings.Builder
+	require.NoError(t, c.PrintSessionSettings(&sb, &s.ClusterSettings().SV, true /* all */))
+	vars := strings.Split(sb.String(), "\n")
+	for _, line := range vars {
+		_, err := sqlDB.ExecContext(ctx, line)
+		if err != nil {
+			words := strings.Split(line, " ")
+			t.Fatalf("%v: probably need to add %q into 'sessionVarNeedsQuotes' map", err, words[1])
+		}
+	}
+
+	sb.Reset()
+	require.NoError(t, c.PrintClusterSettings(&sb, true /* all */))
+	vars = strings.Split(sb.String(), "\n")
+	for _, line := range vars {
+		_, err := sqlDB.ExecContext(ctx, line)
+		if err != nil {
+			t.Fatalf("unexpectedly couldn't execute %s: %v", line, err)
+		}
+	}
+}
diff --git a/pkg/sql/instrumentation.go b/pkg/sql/instrumentation.go
@@ -662,7 +662,7 @@ func (ih *instrumentationHelper) Finish(
 				planString = "-- plan elided due to gist matching"
 			}
 			bundle = buildStatementBundle(
-				bundleCtx, ih.explainFlags, cfg.DB, ie.(*InternalExecutor),
+				bundleCtx, ih.explainFlags, cfg.DB, p, ie.(*InternalExecutor),
 				stmtRawSQL, &p.curPlan, planString, trace, placeholders, res.ErrAllowReleased(),
 				payloadErr, retErr, &p.extendedEvalCtx.Settings.SV, ih.inFlightTraceCollector,
 			)

diff --git a/pkg/sql/logictest/testdata/logic_test/pg_catalog b/pkg/sql/logictest/testdata/logic_test/pg_catalog
@@ -2974,7 +2974,7 @@ allow_ordinal_column_references                            off                 N
 allow_role_memberships_to_change_during_transaction        off                 NULL  user     NULL      off                 off
 alter_primary_region_super_region_override                 off                 NULL  user     NULL      off                 off
 application_name                                           ·                   NULL  user     NULL      ·                   ·
-avoid_buffering                                            off                 NULL  user     NULL      false               false
+avoid_buffering                                            off                 NULL  user     NULL      off                 off
 backslash_quote                                            safe_encoding       NULL  user     NULL      safe_encoding       safe_encoding
 bytea_output                                               hex                 NULL  user     NULL      hex                 hex
 check_function_bodies                                      on                  NULL  user     NULL      on                  on
@@ -3085,7 +3085,7 @@ optimizer_use_provided_ordering_fix                        on                  N
 optimizer_use_trigram_similarity_optimization              off                 NULL  user     NULL      off                 off
 optimizer_use_virtual_computed_column_stats                off                 NULL  user     NULL      off                 off
 override_multi_region_zone_config                          off                 NULL  user     NULL      off                 off
-parallelize_multi_key_lookup_joins_enabled                 off                 NULL  user     NULL      false               false
+parallelize_multi_key_lookup_joins_enabled                 off                 NULL  user     NULL      off                 off
 password_encryption                                        scram-sha-256       NULL  user     NULL      scram-sha-256       scram-sha-256
 pg_trgm.similarity_threshold                               0.3                 NULL  user     NULL      0.3                 0.3
 prefer_lookup_joins_for_fks                                off                 NULL  user     NULL      off                 off
@@ -3131,7 +3131,7 @@ transaction_rows_read_log                                  0                   N
 transaction_rows_written_err                               0                   NULL  user     NULL      0                   0
 transaction_rows_written_log                               0                   NULL  user     NULL      0                   0
 transaction_status                                         NoTxn               NULL  user     NULL      NoTxn               NoTxn
-transaction_timeout                                        0                   NULL  user     NULL      0                   0
+transaction_timeout                                        0                   NULL  user     NULL      0s                  0s
 troubleshooting_mode                                       off                 NULL  user     NULL      off                 off
 unbounded_parallel_scans                                   off                 NULL  user     NULL      off                 off
 unconstrained_non_covering_index_scan_enabled              off                 NULL  user     NULL      off                 off

diff --git a/pkg/sql/logictest/testdata/logic_test/set b/pkg/sql/logictest/testdata/logic_test/set
@@ -912,3 +912,7 @@ SHOW application_name
 ·
 
 subtest end
+
+# Regression test for incorrectly marking this variable as boolean.
+statement ok
+SET copy_num_retries_per_batch = 5;