Merge #151774 #151949 #152080

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

152080: clusterversion: bump PreviousVersion r=RaduBerinde a=RaduBerinde

Bump PreviousVersion to 25.3, add
cockroach-go-testserver-25.3 tests.

This is part of the [M.3 checklist](https://github.com/cockroachdb/cockroach/blob/master/pkg/clusterversion/README.md#m3-enable-upgrade-tests)
(the roachtest fixtures were already done in #150712).

- [x] Update `PreviousRelease` constant to 25.3.

- [x] Verify the logic in `supportsSkipUpgradeTo`
  (`pkg/cmd/roachtest/roachtestutil/mixedversion/mixedversion.go`) is correct
  for the new release.

- [x] Add `cockroach-go-testserver-25.3` logictest config and add it to
  `cockroach-go-testserver-configs`. Update the visibility for
  `cockroach_predecessor_version` in `pkg/sql/logictest/BUILD.bazel`. Run
  `./dev gen` and fix up any tests that fail (using a draft PR helps).

- [x] Check that all gates for the previous release are identical on the
  `master` and release branch.

Epic: REL-2736
Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>
Co-authored-by: Kyle Wong <37189875+kyle-a-wong@users.noreply.github.com>
Co-authored-by: Bhaskarjyoti Bora <bhaskar.bora@cockroachlabs.com>

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

@@ -508,6 +508,7 @@ ALL_TESTS = [
     "//pkg/sql/logictest/tests/5node-disk:5node-disk_test",
     "//pkg/sql/logictest/tests/5node:5node_test",
     "//pkg/sql/logictest/tests/cockroach-go-testserver-25.2:cockroach-go-testserver-25_2_test",
+    "//pkg/sql/logictest/tests/cockroach-go-testserver-25.3:cockroach-go-testserver-25_3_test",
     "//pkg/sql/logictest/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/logictest/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/logictest/tests/fakedist:fakedist_test",
@@ -2054,6 +2055,7 @@ GO_TARGETS = [
     "//pkg/sql/logictest/tests/5node-disk:5node-disk_test",
     "//pkg/sql/logictest/tests/5node:5node_test",
     "//pkg/sql/logictest/tests/cockroach-go-testserver-25.2:cockroach-go-testserver-25_2_test",
+    "//pkg/sql/logictest/tests/cockroach-go-testserver-25.3:cockroach-go-testserver-25_3_test",
     "//pkg/sql/logictest/tests/fakedist-disk:fakedist-disk_test",
     "//pkg/sql/logictest/tests/fakedist-vec-off:fakedist-vec-off_test",
     "//pkg/sql/logictest/tests/fakedist:fakedist_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/cli/testdata/declarative-rules/deprules

@@ -1,6 +1,6 @@
 dep
 ----
-debug declarative-print-rules 1000025.2 dep
+debug declarative-print-rules 1000025.3 dep
 deprules
 ----
 - name: 'CheckConstraint transitions to ABSENT uphold 2-version invariant: PUBLIC->VALIDATED'

pkg/clusterversion/cockroach_versions.go

@@ -283,7 +283,7 @@ const MinSupported Key = V25_2
 
 // PreviousRelease is the logical cluster version of the previous release (which must
 // have at least an RC build published).
-const PreviousRelease Key = V25_2
+const PreviousRelease Key = V25_3
 
 // V25_4 is a placeholder that will eventually be replaced by the actual 25.4
 // version Key, but in the meantime it points to the latest Key. The placeholder

pkg/crosscluster/logical/logical_replication_job_test.go

@@ -2889,7 +2889,7 @@ func TestGetWriterType(t *testing.T) {
 
 	t.Run("immediate-mode", func(t *testing.T) {
 		st := cluster.MakeTestingClusterSettingsWithVersions(
-			clusterversion.V25_2.Version(),
+			clusterversion.V25_3.Version(),
 			clusterversion.PreviousRelease.Version(),
 			true, /* initializeVersion */
 		)

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