Merge #145893 #153995 #154990

145893: isession: create internal session r=jeffswenson a=jeffswenson

This introduces the InternalSession. The immediate motivation for this
is to improve the performance of LDR. LDR currently depends on the
internal executor. Every statement executed with the internal executor
creates its own connExecutor instance. This prevents LDR from taking
advantage of prepared statements and generic query plans.

Long term, the idea is to have InternalSession replace the internal
executor. But it was developed as a stand alone conn executor wrapper in
order to minimize the risk of breaking existing clients of the internal
executor.

One key design feature of the InternalSession is the state machine was
moved from a separate goroutine to the goroutine that is requesting the
statement execution. This makes CPU profiles for internal processes much
easier to read because it is obvious which call stack is using the SQL
layer. It may also help with integrating LDR's SQL CPU usage with
elastic admission control since the CPU time is accounted to the calling
goroutine.

Release note: none
Informs: #148310

153995: sql/hints: add per-node cache for system.statement_hints r=DrewKimball a=DrewKimball

#### sql/hints: change StatementHint protobuf struct to union

This commit makes a few changes to the protobuf message used for
serializing statement hints in the `system.statement_hints` table to
prepare for future commits/PRs. No release note or migrations are
necessary, since nothing is currently store nor read from the table.

Epic: None

Release note: None

#### sql/hints: add per-node cache for system.statement_hints

This commit adds a per-node cache for the `system.statement_hints` table,
similar to the table stats cache. The cache tracks the set of all statement
fingerprint hashes that have hints in a map kept up-to-date via rangefeed.
This allows queries to quickly determine whether they have hints, minimizing
the overhead for unhinted queries. Hints are kept in a separate
limited-capacity LRU cache. The LRU cache size can be configured by setting
the `sql.hints.statement_hints_cache_size` cluster setting.

Fixes #148160

Release note: None

#### sql/hints: wait for initial scan during server startup

Server startup will now block until the initial scan of the
`system.statement_hints` table for the hints cache has finished.
This ensures that the node will not accept queries until `hintedHashes`
is initialized, which means that queries without hints will not have
to perform system table reads.

It is possible for the initial scan to fail, in which case server startup
will unblock while the scan restarts. In this rare error scenario,
queries will have to read from the system table on the first execution
even if they don't have hints until the scan finishes. This is
considered preferable to blocking startup across retries, since the
`hintedHashes` map is only an optimization.

Epic: None

Release note: None

#### sql/hints: add tests for statement hints cache

This commit adds a set of tests for the behavior of the hint cache.
The tests verify that the cache is correctly populated in response
to existing hints as well as new insertions and deletions.

Epic: None

Release note: None

154990: sql/schemachanger: add dml injection tests for vector indexes r=fqazi a=fqazi

Previously, we didn't have DML injection tests for creating vector
indexes or changing the primary key on tables with vector indexes. We
now add these.

This patch also addresses one more issue in the secondary index recreation logic that can lead to selecting the wrong target index ID, when index chains are being folded.


Informs: #144443
Release notes: none

Co-authored-by: Jeff Swenson <[email protected]>
Co-authored-by: Drew Kimball <[email protected]>
Co-authored-by: Faizan Qazi <[email protected]>

Author: 4 people

.github/CODEOWNERS

@@ -70,6 +70,7 @@
 /pkg/sql/execute.go                    @cockroachdb/sql-queries-prs
 /pkg/sql/executor_statement_metrics.go @cockroachdb/obs-prs
 /pkg/sql/flowinfra/          @cockroachdb/sql-queries-prs
+/pkg/sql/hints/              @cockroachdb/sql-queries-prs
 /pkg/sql/physicalplan/       @cockroachdb/sql-queries-prs
 /pkg/sql/row*                @cockroachdb/sql-queries-prs
 /pkg/sql/control_job*        @cockroachdb/sql-queries-prs @cockroachdb/jobs-prs

build/bazelutil/nogo_config.json

@@ -144,6 +144,7 @@
             "pkg/sql/contention/event_store.go": "flagged by linter, should be evaluated",
             "pkg/sql/distsql_running.go": "flagged by linter, should be evaluated",
             "pkg/sql/flowinfra/flow_registry.go": "flagged by linter, should be evaluated",
+            "pkg/sql/hints/hint_cache.go": "flagged by linter, should be evaluated",
             "pkg/sql/idxusage/local_idx_usage_stats.go": "flagged by linter, should be evaluated",
             "pkg/sql/importer/import_processor.go": "flagged by linter, should be evaluated",
             "pkg/sql/importer/import_processor_planning.go": "flagged by linter, should be evaluated",

pkg/BUILD.bazel

@@ -506,12 +506,14 @@ ALL_TESTS = [
     "//pkg/sql/gcjob/gcjobnotifier:gcjobnotifier_test",
     "//pkg/sql/gcjob:gcjob_test",
     "//pkg/sql/gcjob_test:gcjob_test_test",
+    "//pkg/sql/hints:hints_test",
     "//pkg/sql/idxrecommendations:idxrecommendations_test",
     "//pkg/sql/idxusage:idxusage_test",
     "//pkg/sql/importer:importer_test",
     "//pkg/sql/inspect:inspect_test",
     "//pkg/sql/inverted:inverted_disallowed_imports_test",
     "//pkg/sql/inverted:inverted_test",
+    "//pkg/sql/isession:isession_test",
     "//pkg/sql/lex:lex_disallowed_imports_test",
     "//pkg/sql/lex:lex_test",
     "//pkg/sql/lexbase:lexbase_test",
@@ -2066,6 +2068,7 @@ GO_TARGETS = [
     "//pkg/sql/gcjob:gcjob_test",
     "//pkg/sql/gcjob_test:gcjob_test_test",
     "//pkg/sql/hints:hints",
+    "//pkg/sql/hints:hints_test",
     "//pkg/sql/idxrecommendations:idxrecommendations",
     "//pkg/sql/idxrecommendations:idxrecommendations_test",
     "//pkg/sql/idxusage:idxusage",
@@ -2076,6 +2079,8 @@ GO_TARGETS = [
     "//pkg/sql/inspect:inspect_test",
     "//pkg/sql/inverted:inverted",
     "//pkg/sql/inverted:inverted_test",
+    "//pkg/sql/isession:isession",
+    "//pkg/sql/isession:isession_test",
     "//pkg/sql/isql:isql",
     "//pkg/sql/lex:lex",
     "//pkg/sql/lex:lex_test",

pkg/ccl/schemachangerccl/sctestbackupccl/backup_base_generated_test.go

@@ -242,9 +242,11 @@ go_library(
         "//pkg/sql/flowinfra",
         "//pkg/sql/gcjob",
         "//pkg/sql/gcjob/gcjobnotifier",
+        "//pkg/sql/hints",
         "//pkg/sql/idxusage",
         "//pkg/sql/importer",
         "//pkg/sql/inspect",
+        "//pkg/sql/isession",
         "//pkg/sql/isql",
         "//pkg/sql/lexbase",
         "//pkg/sql/optionalnodeliveness",

pkg/server/BUILD.bazel

@@ -101,6 +101,7 @@ import (
 	_ "github.com/cockroachdb/cockroach/pkg/sql/gcjob"    // register jobs declared outside of pkg/sql
 	_ "github.com/cockroachdb/cockroach/pkg/sql/importer" // register jobs/planHooks declared outside of pkg/sql
 	_ "github.com/cockroachdb/cockroach/pkg/sql/inspect"  // register job and planHook declared outside of pkg/sql
+	_ "github.com/cockroachdb/cockroach/pkg/sql/isession" // register isession constructor hook
 	"github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	_ "github.com/cockroachdb/cockroach/pkg/sql/schemachanger/scjob" // register jobs declared outside of pkg/sql

pkg/server/server.go

@@ -83,6 +83,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/execinfrapb"
 	"github.com/cockroachdb/cockroach/pkg/sql/flowinfra"
 	"github.com/cockroachdb/cockroach/pkg/sql/gcjob/gcjobnotifier"
+	"github.com/cockroachdb/cockroach/pkg/sql/hints"
 	"github.com/cockroachdb/cockroach/pkg/sql/idxusage"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/optionalnodeliveness"
@@ -1039,7 +1040,10 @@ func newSQLServer(ctx context.Context, cfg sqlServerArgs) (*SQLServer, error) {
 			cfg.stopper,
 		),
 
-		QueryCache:                 querycache.New(cfg.QueryCacheSize),
+		QueryCache: querycache.New(cfg.QueryCacheSize),
+		StatementHintsCache: hints.NewStatementHintsCache(
+			cfg.clock, cfg.rangeFeedFactory, cfg.stopper, codec, cfg.internalDB, cfg.Settings,
+		),
 		VecIndexManager:            vecIndexManager,
 		RowMetrics:                 &rowMetrics,
 		InternalRowMetrics:         &internalRowMetrics,
@@ -1775,6 +1779,9 @@ func (s *SQLServer) preStart(
 	if err := s.execCfg.TableStatsCache.Start(ctx, s.execCfg.Codec, s.execCfg.RangeFeedFactory); err != nil {
 		return err
 	}
+	if err = s.execCfg.StatementHintsCache.Start(ctx, s.execCfg.SystemTableIDResolver); err != nil {
+		return err
+	}
 
 	scheduledlogging.Start(
 		ctx, stopper, s.execCfg.InternalDB, s.execCfg.Settings,

pkg/server/server_sql.go

@@ -145,6 +145,7 @@ go_library(
         "instrumentation.go",
         "internal.go",
         "internal_result_channel.go",
+        "internal_session.go",
         "inverted_filter.go",
         "inverted_join.go",
         "job_exec_context.go",
@@ -441,6 +442,7 @@ go_library(
         "//pkg/sql/faketreeeval",
         "//pkg/sql/flowinfra",
         "//pkg/sql/gcjob/gcjobnotifier",
+        "//pkg/sql/hints",
         "//pkg/sql/idxrecommendations",
         "//pkg/sql/idxusage",
         "//pkg/sql/inverted",

pkg/sql/BUILD.bazel

@@ -5433,9 +5433,9 @@ var (
 		},
 	)
 
-	statementHintsComputeExpr = "fnv64(fingerprint)"
-
-	StatementHintsTable = makeSystemTable(
+	statementHintsComputeExpr                = "fnv64(fingerprint)"
+	StatementHintsHashIndexID descpb.IndexID = 2
+	StatementHintsTable                      = makeSystemTable(
 		StatementHintsTableSchema,
 		systemTable(
 			catconstants.StatementHintsTableName,
@@ -5465,7 +5465,7 @@ var (
 			},
 			descpb.IndexDescriptor{
 				Name:                "hash_idx",
-				ID:                  2,
+				ID:                  StatementHintsHashIndexID,
 				Unique:              false,
 				Version:             descpb.StrictIndexColumnIDGuaranteesVersion,
 				KeyColumnNames:      []string{"hash"},

pkg/sql/catalog/systemschema/system.go

@@ -3503,9 +3503,9 @@ var retryableMinTimestampBoundUnsatisfiableError = errors.Newf(
 	"retryable MinTimestampBoundUnsatisfiableError",
 )
 
-// errIsRetryable is true if the error is a client-visible retry error
+// ErrIsRetryable is true if the error is a client-visible retry error
 // or the error is a special error that is handled internally and retried.
-func errIsRetryable(err error) bool {
+func ErrIsRetryable(err error) bool {
 	return errors.HasInterface(err, (*pgerror.ClientVisibleRetryError)(nil)) ||
 		errors.Is(err, retryableMinTimestampBoundUnsatisfiableError) ||
 		descs.IsTwoVersionInvariantViolationError(err)
@@ -3563,7 +3563,7 @@ func (ex *connExecutor) makeErrEvent(err error, stmt tree.Statement) (fsm.Event,
 		}
 	}
 
-	retryable := errIsRetryable(err)
+	retryable := ErrIsRetryable(err)
 	if retryable {
 		var rc rewindCapability
 		var canAutoRetry bool