Merge #151362 #151774 #151949

151362: sql: gate system database behind session variable r=angles-n-daemons a=angles-n-daemons

It today isn't quite clear which internal objects are safe for external usage. For example, selecting from crdb_internal.zones is considered safe, while deleting from system.jobs is not.

To address this, we're adding a gate protecting some of these internals, as well as auditing around their usage so that we can better see how the internal state of the database may have been modified.

The start of this effort is this PR, which adds an gate on the privilege check for system descriptors. This gate is simple now, it checks whether the caller is internal or if the override session variable is set to access "unsafe internals".

It should be noted that allowance is the default to start, so that extensive testing can be done on this feature before its rolled out more widely.

Fixes: #149594 
Fixes: #151333
Epic: CRDB-24527

Release note (sql change): Adds a new session variable `allow_unsafe_internals` and corresponding cluster setting (`sql.defaults.allow_unsafe_internals`) which controls access to the `system` database.

151774: kv,sql: explicitly set IsReverse on the Header r=yuzefovich a=yuzefovich

When processing batches that touch multiple ranges, the DistSender needs
to know whether to iterate across those ranges in the forward or reverse
manner (i.e. ASC or DESC for range keys). Currently, it only uses the
reverse direction if it finds at least one ReverseScan request in the
batch, and it uses the forward direction otherwise. This goes against
the needs of SQL which might issue point Gets when performing a SQL
revscan operation, and currently in such a scenario we would hit an
error (instead of returning the results in incorrect order).

This commit fixes the issue by introducing `IsReverse` boolean on the
batch header to explicitly indicate the direction for range iteration.
It seems reasonable that the caller should be explicit about this, and
we also add a validation that the boolean is set correctly (meaning that
it should be `false` when only forward range requests are present and
`true` when only reverse range requests are present).

In order to simplify the story a bit, the DistSender will no longer
allow batches that have both forward and reverse range requests.
Previously, this limitation only applied to the batches with limits, but
now it's extended to be unconditional. SQL never issues such batches, so
the limitation seems acceptable. This limitation required some updates
to the existing tests, including KVNemesis to not generate batches that
are now disallowed.

See also 619f395 for some related context.

Given that the new header field is only examined on the KV client, the
change can be backported with no mixed-version concerns.

Fixes: #146637.

Release note (bug fix): Previously, CockroachDB could hit an error
`ERROR: span with results after resume span...` when evaluating some
queries with ORDER BY ... DESC in an edge case. The bug has been present
since about v22.1 and is now fixed.

151949: sql: prep migration for sampled_query and sampled_transaction events r=kyle-a-wong a=kyle-a-wong

In v26.1, sampled_query and sampled_transaction events will be moved
from the TELEMETRY logging channel to the SQL_EXEC logging channel.

This commit gates this migration under the cluster setting:
`log.channel_compatibility_mode.enabled` and will log these events
to the SQL_EXEC channel if this setting is set to false. Users can
set this setting to false in their clusters to validate, test, and
identify potential downstream impacts to their logging setups and
pipelines.

Epic: [CRDB-53410](https://cockroachlabs.atlassian.net/browse/CRDB-53410)
Part of: [CRDB-53412](https://cockroachlabs.atlassian.net/browse/CRDB-53412)
Release note (ops change): sampled_query and sampled_transaction
events will be moved to the SQL_EXEC channel in 26.1. In order to
test the impact of these changes, users can set the setting:
`log.channel_compatibility_mode.enabled` to false. Note that this
will cause these logs to start logging in the SQL_EXEC channel so
this shouldn't be tested in a production environment.

----
This is a stacked PR, only the last commit in this PR should be reviewed

Co-authored-by: Brian Dillmann <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>

Author: 4 people

docs/generated/eventlog.md

@@ -3374,6 +3374,11 @@ Fields in this struct should be updated in sync with apps_stats.proto.
 An event of type `sampled_query` is the SQL query event logged to the telemetry channel. It
 contains common SQL event/execution details.
 
+Note: in version 26.1, these events will be moved to the `SQL_EXEC` channel.
+To test compatability before this, set the cluster setting
+`log.channel_compatibility_mode.enabled` to false. This will send the
+events to `SQL_EXEC` instead of `TELEMETRY`.
+
 
 | Field | Description | Sensitive |
 |--|--|--|
@@ -3482,6 +3487,11 @@ contains common SQL event/execution details.
 
 An event of type `sampled_transaction` is the event logged to telemetry at the end of transaction execution.
 
+Note: in version 26.1, these events will be moved to the `SQL_EXEC` channel.
+To test compatability before this, set the cluster setting
+`log.channel_compatibility_mode.enabled` to false. This will send the
+events to `SQL_EXEC` instead of `TELEMETRY`.
+
 
 | Field | Description | Sensitive |
 |--|--|--|

pkg/BUILD.bazel

@@ -662,6 +662,7 @@ ALL_TESTS = [
     "//pkg/sql/ttl/ttljob:ttljob_test",
     "//pkg/sql/types:types_disallowed_imports_test",
     "//pkg/sql/types:types_test",
+    "//pkg/sql/unsafesql:unsafesql_test",
     "//pkg/sql/vecindex/cspann/memstore:memstore_test",
     "//pkg/sql/vecindex/cspann/quantize:quantize_test",
     "//pkg/sql/vecindex/cspann/utils:utils_test",
@@ -2397,6 +2398,8 @@ GO_TARGETS = [
     "//pkg/sql/ttl/ttlschedule:ttlschedule",
     "//pkg/sql/types:types",
     "//pkg/sql/types:types_test",
+    "//pkg/sql/unsafesql:unsafesql",
+    "//pkg/sql/unsafesql:unsafesql_test",
     "//pkg/sql/vecindex/cspann/commontest:commontest",
     "//pkg/sql/vecindex/cspann/memstore:memstore",
     "//pkg/sql/vecindex/cspann/memstore:memstore_test",

pkg/ccl/telemetryccl/BUILD.bazel

@@ -20,6 +20,7 @@ go_test(
         "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/server",
+        "//pkg/settings",
         "//pkg/sql",
         "//pkg/sql/sem/tree",
         "//pkg/sql/sqltestutils",

pkg/ccl/telemetryccl/telemetry_logging_test.go

@@ -13,6 +13,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"regexp"
+	"slices"
 	"strings"
 	"testing"
 
@@ -21,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/cloud/nodelocal"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/settings"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
@@ -155,20 +157,29 @@ type expectedSampleQueryEvent struct {
 }
 
 type telemetrySpy struct {
-	t *testing.T
+	t  *testing.T
+	sv *settings.Values
 
 	sampledQueries    []eventpb.SampledQuery
 	sampledQueriesRaw []logpb.Entry
 	recoveryEvents    []eventpb.RecoveryEvent
 }
 
+func (l *telemetrySpy) channelsToIntercept() []log.Channel {
+	if log.ShouldMigrateEvent(l.sv) {
+		return []log.Channel{logpb.Channel_TELEMETRY, logpb.Channel_SQL_EXEC}
+	}
+
+	return []log.Channel{logpb.Channel_TELEMETRY}
+}
+
 func (l *telemetrySpy) Intercept(entry []byte) {
 	var rawLog logpb.Entry
 	if err := json.Unmarshal(entry, &rawLog); err != nil {
 		l.t.Errorf("failed unmarshaling %s: %s", entry, err)
 	}
 
-	if rawLog.Channel != logpb.Channel_TELEMETRY {
+	if !slices.Contains(l.channelsToIntercept(), rawLog.Channel) {
 		return
 	}
 
@@ -204,12 +215,6 @@ func TestBulkJobTelemetryLogging(t *testing.T) {
 
 	ctx := context.Background()
 
-	spy := &telemetrySpy{
-		t: t,
-	}
-	cleanup := log.InterceptWith(ctx, spy)
-	defer cleanup()
-
 	st := logtestutils.StubTime{}
 	sqm := logtestutils.StubQueryStats{}
 	sts := logtestutils.StubTracingStatus{}
@@ -229,6 +234,15 @@ func TestBulkJobTelemetryLogging(t *testing.T) {
 			ExternalIODir: dir,
 		},
 	})
+
+	spy := &telemetrySpy{
+		t:  t,
+		sv: &testCluster.Server(0).ClusterSettings().SV,
+	}
+
+	cleanup := log.InterceptWith(ctx, spy)
+	defer cleanup()
+
 	sqlDB := testCluster.ServerConn(0)
 	defer func() {
 		testCluster.Stopper().Stop(context.Background())

pkg/kv/batch.go

@@ -365,6 +365,8 @@ func (b *Batch) AddRawRequest(reqs ...kvpb.Request) {
 			*kvpb.IncrementRequest,
 			*kvpb.DeleteRequest:
 			numRows = 1
+		case *kvpb.ReverseScanRequest:
+			b.Header.IsReverse = true
 		}
 		b.appendReqs(args)
 		b.initResult(1 /* calls */, numRows, raw, nil)
@@ -735,6 +737,7 @@ func (b *Batch) scan(
 	str kvpb.KeyLockingStrengthType,
 	dur kvpb.KeyLockingDurabilityType,
 ) {
+	b.Header.IsReverse = isReverse
 	begin, err := marshalKey(s)
 	if err != nil {
 		b.initResult(0, 0, notRaw, err)

pkg/kv/db.go

@@ -499,6 +499,7 @@ func (db *DB) scan(
 	if maxRows > 0 {
 		b.Header.MaxSpanRequestKeys = maxRows
 	}
+	b.Header.IsReverse = isReverse
 	b.scan(begin, end, isReverse, str, dur)
 	r, err := getOneResult(db.Run(ctx, b), b)
 	return r.Rows, err