Merge #152882 #153419 #154654 #154676 #154677 #154679 #154698 #154711 #154733 #154742

152882: sql: fix placeholder type checking r=mgartner a=mgartner

Previously, an binary expression with two placeholders could be
incorrectly typed in a prepared statement. The issue could occur when
one placeholder had an `UNKNOWN` type hint and one with a non-ambiguous
type hint, e.g., `INT`, `DECIMAL`, etc. The `UNKNOWN` type hint could
take precedence over the non-ambiguous type. This could cause incorrect
results in some cases.

The bug has been fixed by preferring non-ambiguous type hints over
ambiguous ones.

Fixes #152664

Release note (bug fix): A bug in type-checking placeholders with
`UNKNOWN` types has been fixed. It could cause incorrect results in some
cases.


153419: sql/stats: merge all arbitrary partial and full table statistics r=Uzair5162 a=Uzair5162

This commit generalizes partial and full stat merging in the stats cache to support partial stats collected over any range of values in a column. Non-extreme partial stats are treated as covering a single span, and extreme partials continue to separate the lower and upper extreme buckets as before.

Additionally, merged stats now apply all partial stats collected after the latest full stat, in order of creation. Previously, we could only merge partial stats collected at the extremes of the index, and would only merge the latest one.

Part of: #93998

Release note (sql change): The optimizer can now use table statistics that merge the latest full statistic with all newer partial statistics, including those over arbitrary constraints over a single span.

154654: restore: deflake TestRestoreCheckpointing r=msbutler a=kev-cao

The current `TestRestoreCheckpointing` waits for job progress to be updated by sleeping for a duration greater than the checkpoint interval. While this works the vast majority of the time, if the test cluster is overloaded, it is not sufficient and we can end up resuming the job before the progress was updated. This results in more spans being processed than expected.

This commit updates the test to instead check the contents of the job progress checkpoint and wait until the checkpoint contains all processed spans from before the pause.

Fixes: #153848

Release note: None

154676: stmtdiagnostics: don't create statement dignostics request for txn di… r=kyle-a-wong a=kyle-a-wong

…agnostics

Previously, `innerInsertStatementDiagnostics` was creating new statement diagnostics requests when a transaction diagnostics bundle was being created. Now, these new statement requests will no longer be created.

Epic: CRDB-53541
Release note: None

154677: sql: remove canModifySchema interface r=mgartner a=mgartner

The `canModifySchema` interface has been removed. The `CanModifySchema`
function now has special logic for the two non-DDL statements that
modify the schema: `DISCARD` and `SET ZONE CONFIG`. Additionally,
`CanModifySchema` now returns early for DML statements—a very minor
optimization to avoid an unnecessary invocation of the
`StatementReturnType` method which can chase additional pointers for
`RETURNING` nodes of `INSERT` and `UPDATE` statements.

Release note: None


154679: sql: move bufferable session var lookup to init-time r=mgartner a=mgartner

Lookups for bufferable session vars are now performed once at init-time
rather than every time `(*connExecutor).reportSessionDataChanges` is
called.

Release note: None


154698: workflows: update names of GitHub Action runner groups r=rail a=rickystewart

These are the new-style names that will make it easier to migrate to Ubuntu 22.04+ when the time comes.

154711: sql: update stats for descriptor table r=rafiss a=rafiss

This is needed in order to enforce the
sql.schema.approx_max_object_count cluster setting, which relies on optimizer table statistics to find the count.

Since the schemachanger uses the KV API to write to the descriptor table, we need to explicitly notify the stats refresher when it should compute new stats for the table.

informs: #148286
Release note: None

###  schematelemetry: update object count gauge using table stats

In order to support larger scales of object counts, we switch away from
using a full table scan on system.descriptors in order to update the
object count gauge.

Instead, we use table stats on system.descriptor now. The
schematelemetry job is updated so it notifies the stats refresher to
keep the stats up to date. Note that the stats refresher is also
notified with a partial count already.

Epic CRDB-48806
Release note: None

154733: changefeedccl: fix slow acquisition log message r=andyyang890 a=stevendanna

Previously,

    have been waiting rangefeed attempting to acquire changefeed quota (buffer=5.000049088s)

Now,

    have been waiting 5.000049088s attempting to acquire changefeed quota (buffer=rangefeed)

It would be nice if the buffer name `rangefeed` was something that indicated that this a buffer that lives in the changefeed, but changing the name impacts the metrics which doesn't seem worth it.

Epic: none
Release note: None

154742: dev-inf: Use pull_request_target for Claude Code review action r=rickystewart a=ajstorm

Previously, the Claude Code PR Review GitHub Action used the pull_request trigger, which runs in the context of the PR branch. This prevents GitHub from injecting OIDC tokens when the PR comes from a forked repository, causing authentication failures with Google Cloud's Workload Identity Federation.

This change updates the workflow to use pull_request_target instead, which runs in the context of the base repository. This allows OIDC tokens to be injected even for fork PRs, enabling authentication with Google Cloud for Vertex AI access.

Our standard git flow requires developers to create PRs from forks, so this change is necessary for the action to function in our development workflow. The workflow is safe to run with pull_request_target because it does not check out the PR branch - all PR content is fetched safely via the GitHub API using 'gh pr diff' and 'gh pr view'.

Release note: None

Epic: None

Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Uzair Ahmad <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: Kyle Wong <[email protected]>
Co-authored-by: Ricky Stewart <[email protected]>
Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Steven Danna <[email protected]>
Co-authored-by: Adam Storm <[email protected]>

Author: 9 people

.github/workflows/github-actions-essential-ci.yml

@@ -54,7 +54,7 @@ concurrency:
   cancel-in-progress: true
 jobs:
   acceptance:
-    runs-on: [self-hosted, basic_big_runner_group]
+    runs-on: [self-hosted, ubuntu_big_2004]
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4
@@ -75,7 +75,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   check_generated_code:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4
@@ -90,7 +90,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   docker_image_amd64:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4
@@ -111,7 +111,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   examples_orms:
-    runs-on: [self-hosted, basic_big_runner_group]
+    runs-on: [self-hosted, ubuntu_big_2004]
     timeout-minutes: 120
     steps:
       - uses: actions/checkout@v4
@@ -132,7 +132,7 @@ jobs:
         run: ./cockroach/build/github/cleanup-engflow-keys.sh
         if: always()
   lint:
-    runs-on: [self-hosted, basic_big_runner_group]
+    runs-on: [self-hosted, ubuntu_big_2004]
     timeout-minutes: 120
     steps:
       - uses: actions/checkout@v4
@@ -156,7 +156,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   local_roachtest:
-    runs-on: [self-hosted, basic_big_runner_group]
+    runs-on: [self-hosted, ubuntu_big_2004]
     timeout-minutes: 120
     steps:
       - uses: actions/checkout@v4
@@ -181,7 +181,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   local_roachtest_fips:
-    runs-on: [self-hosted, basic_runner_group_fips]
+    runs-on: [self-hosted, ubuntu_2004_fips]
     timeout-minutes: 120
     steps:
       - uses: actions/checkout@v4
@@ -206,7 +206,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   linux_amd64_build:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4
@@ -227,7 +227,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   linux_amd64_fips_build:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 60
     steps:
       - uses: actions/checkout@v4
@@ -248,7 +248,7 @@ jobs:
         run: ./build/github/cleanup-engflow-keys.sh
         if: always()
   unit_tests:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 120
     steps:
       - uses: actions/checkout@v4

.github/workflows/microbenchmarks-ci.yaml

@@ -17,7 +17,7 @@ env:
 jobs:
   base:
     name: build merge base
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 30
     if: ${{ !contains(github.event.pull_request.labels.*.name, 'X-skip-perf-check') }}
     outputs:
@@ -33,7 +33,7 @@ jobs:
           pkg: ${{ env.PACKAGE }}
   head:
     name: build head
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 30
     if: ${{ !contains(github.event.pull_request.labels.*.name, 'X-skip-perf-check') }}
     steps:
@@ -46,7 +46,7 @@ jobs:
           ref: head
           pkg: ${{ env.PACKAGE }}
   run-group-1:
-    runs-on: [self-hosted, basic_microbench_runner_group]
+    runs-on: [self-hosted, ubuntu_2004_microbench]
     timeout-minutes: 60
     needs: [base, head]
     steps:
@@ -59,7 +59,7 @@ jobs:
           pkg: ${{ env.PACKAGE }}
           group: 1
   run-group-2:
-    runs-on: [self-hosted, basic_microbench_runner_group]
+    runs-on: [self-hosted, ubuntu_2004_microbench]
     timeout-minutes: 60
     needs: [base, head]
     steps:
@@ -72,7 +72,7 @@ jobs:
           pkg: ${{ env.PACKAGE }}
           group: 2
   compare:
-    runs-on: [self-hosted, basic_runner_group]
+    runs-on: [self-hosted, ubuntu_2004]
     timeout-minutes: 30
     permissions:
         contents: read

.github/workflows/pr-analyzer-threestage.yml

@@ -1,7 +1,7 @@
 name: Claude Code PR Review
 
 on:
-  pull_request:
+  pull_request_target:
     types: [synchronize, ready_for_review, reopened, labeled]
 
 jobs:

pkg/BUILD.bazel

@@ -2339,6 +2339,7 @@ GO_TARGETS = [
     "//pkg/sql/sem/plpgsqltree:plpgsqltree",
     "//pkg/sql/sem/semenumpb:semenumpb",
     "//pkg/sql/sem/transform:transform",
+    "//pkg/sql/sem/tree/datumrange:datumrange",
     "//pkg/sql/sem/tree/evalgen:evalgen",
     "//pkg/sql/sem/tree/evalgen:evalgen_lib",
     "//pkg/sql/sem/tree/treebin:treebin",

pkg/backup/backup_test.go

@@ -1301,8 +1301,12 @@ func TestRestoreCheckpointing(t *testing.T) {
 	defer jobs.TestingSetProgressThresholds()()
 
 	// totalEntries represents the number of entries to appear in the persisted frontier.
-	totalEntries := 7
-	entriesBeforePause := 4
+	const totalEntries = 7
+	const entriesBeforePause = 4
+	processedSpans := struct {
+		syncutil.Mutex
+		spans roachpb.Spans
+	}{}
 	entriesCount := 0
 	var alreadyPaused atomic.Bool
 	postResumeCount := 0
@@ -1313,7 +1317,7 @@ func TestRestoreCheckpointing(t *testing.T) {
 	knobs := base.TestingKnobs{
 		DistSQL: &execinfra.TestingKnobs{
 			BackupRestoreTestingKnobs: &sql.BackupRestoreTestingKnobs{
-				RunAfterProcessingRestoreSpanEntry: func(_ context.Context, _ *execinfrapb.RestoreSpanEntry) error {
+				RunAfterProcessingRestoreSpanEntry: func(_ context.Context, entry *execinfrapb.RestoreSpanEntry) error {
 					//  Because the restore processor has several workers that
 					//  concurrently send addsstable requests and because all workers will
 					//  wait on the lock below, when one flush gets blocked on the
@@ -1325,12 +1329,20 @@ func TestRestoreCheckpointing(t *testing.T) {
 					//  checking if the job was paused in each request before it began
 					//  waiting for the lock.
 					wasPausedBeforeWaiting := alreadyPaused.Load()
+
 					mu.Lock()
 					defer mu.Unlock()
 					if entriesCount == entriesBeforePause {
 						close(waitForProgress)
 						<-blockDBRestore
+					} else if entriesCount < entriesBeforePause {
+						// We save all spans from before the pause to ensure that they have
+						// been checkpointed and saved in the job progress.
+						processedSpans.Lock()
+						defer processedSpans.Unlock()
+						processedSpans.spans = append(processedSpans.spans, entry.Span)
 					}
+
 					entriesCount++
 					if wasPausedBeforeWaiting {
 						postResumeCount++
@@ -1373,8 +1385,25 @@ func TestRestoreCheckpointing(t *testing.T) {
 	// Pause the job after some progress has been logged.
 	<-waitForProgress
 
-	// To ensure that progress gets persisted, sleep well beyond the test only job update interval.
-	time.Sleep(time.Second)
+	// To ensure that progress has been persisted, we wait until all processed
+	// spans from before the pause are stored in the job progress.
+	testutils.SucceedsSoon(t, func() error {
+		jobProgress := jobutils.GetJobProgress(t, sqlDB, jobID)
+		checkpointedSpans := jobProgress.GetRestore().Checkpoint
+		checkpointedSpanGroup := roachpb.SpanGroup{}
+		for _, span := range checkpointedSpans {
+			checkpointedSpanGroup.Add(span.Span)
+		}
+
+		processedSpans.Lock()
+		defer processedSpans.Unlock()
+		for _, span := range processedSpans.spans {
+			if !checkpointedSpanGroup.Encloses(span) {
+				return errors.Newf("span %s was processed but not saved in job progress yet")
+			}
+		}
+		return nil
+	})
 
 	sqlDB.Exec(t, `PAUSE JOB $1`, &jobID)
 	jobutils.WaitForJobToPause(t, sqlDB, jobID)

pkg/ccl/changefeedccl/kvevent/blocking_buffer.go

@@ -523,13 +523,16 @@ func logSlowAcquisition(
 	return func(ctx context.Context, poolName string, r quotapool.Request, start time.Time) func() {
 		shouldLog := logSlowAcquire.ShouldLog()
 		if shouldLog {
-			log.Changefeed.Warningf(ctx, "have been waiting %s attempting to acquire changefeed quota (buffer=%s)", redact.SafeString(bufType),
-				timeutil.Since(start))
+			log.Changefeed.Warningf(ctx, "have been waiting %s attempting to acquire changefeed quota (buffer=%s)",
+				timeutil.Since(start),
+				redact.SafeString(bufType))
 		}
 
 		return func() {
 			if shouldLog {
-				log.Changefeed.Infof(ctx, "acquired changefeed quota after %s (buffer=%s)", timeutil.Since(start), redact.SafeString(bufType))
+				log.Changefeed.Infof(ctx, "acquired changefeed quota after %s (buffer=%s)",
+					timeutil.Since(start),
+					redact.SafeString(bufType))
 			}
 		}
 	}

pkg/sql/catalog/descs/collection.go

@@ -273,21 +273,17 @@ func (tc *Collection) IsVersionBumpOfUncommittedDescriptor(id descpb.ID) bool {
 	return tc.uncommitted.versionBumpOnly[id]
 }
 
-// HasUncommittedNewOrDroppedDescriptors returns true if the collection contains
-// any uncommitted descriptors that are newly created or dropped.
-func (tc *Collection) HasUncommittedNewOrDroppedDescriptors() bool {
-	isNewDescriptor := false
-	err := tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error {
+// CountUncommittedNewOrDroppedDescriptors returns the number of uncommitted
+// descriptors that are newly created or dropped.
+func (tc *Collection) CountUncommittedNewOrDroppedDescriptors() int {
+	count := 0
+	_ = tc.uncommitted.iterateUncommittedByID(func(desc catalog.Descriptor) error {
 		if desc.GetVersion() == 1 || desc.Dropped() {
-			isNewDescriptor = true
-			return iterutil.StopIteration()
+			count++
 		}
 		return nil
 	})
-	if err != nil {
-		return false
-	}
-	return isNewDescriptor
+	return count
 }
 
 // HasUncommittedTypes returns true if the Collection contains uncommitted

pkg/sql/catalog/schematelemetry/BUILD.bazel

@@ -12,6 +12,7 @@ go_library(
     deps = [
         "//pkg/jobs",
         "//pkg/jobs/jobspb",
+        "//pkg/keys",
         "//pkg/scheduledjobs",
         "//pkg/security/username",
         "//pkg/server/telemetry",

pkg/sql/catalog/schematelemetry/schema_telemetry_job.go

@@ -7,9 +7,11 @@ package schematelemetry
 
 import (
 	"context"
+	"math"
 
 	"github.com/cockroachdb/cockroach/pkg/jobs"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
+	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
@@ -63,6 +65,31 @@ func (t schemaTelemetryResumer) Resume(ctx context.Context, execCtx interface{})
 	if k := p.ExecCfg().SchemaTelemetryTestingKnobs; k != nil {
 		knobs = *k
 	}
+	// Notify the stats refresher to update the system.descriptors table stats,
+	// and update the object count in schema changer metrics.
+	err := p.ExecCfg().InternalDB.DescsTxn(ctx, func(ctx context.Context, txn descs.Txn) error {
+		desc, err := txn.Descriptors().ByIDWithLeased(txn.KV()).Get().Table(ctx, keys.DescriptorTableID)
+		if err != nil {
+			return err
+		}
+		p.ExecCfg().StatsRefresher.NotifyMutation(desc, math.MaxInt64 /* rowCount */)
+
+		// Note: This won't be perfectly up-to-date, but it will make sure the
+		// metric gets updated periodically. It also gets updated after every
+		// schema change.
+		tableStats, err := p.ExecCfg().TableStatsCache.GetTableStats(ctx, desc, nil /* typeResolver */)
+		if err != nil {
+			return err
+		}
+		if len(tableStats) > 0 {
+			// Use the row count from the most recent statistic.
+			p.ExecCfg().SchemaChangerMetrics.ObjectCount.Update(int64(tableStats[0].RowCount))
+		}
+		return nil
+	})
+	if err != nil {
+		return errors.Wrap(err, "failed to notify stats refresher to update system.descriptors table stats")
+	}
 
 	// Outside of tests, scan the catalog tables AS OF SYSTEM TIME slightly in the
 	// past. Schema telemetry is not latency-sensitive to the point where a few

pkg/sql/conn_executor.go

@@ -4169,11 +4169,10 @@ func (ex *connExecutor) txnStateTransitionsApplyWrapper(
 			if err := ex.waitForInitialVersionForNewDescriptors(cachedRegions); err != nil {
 				return advanceInfo{}, err
 			}
-		}
-		if ex.extraTxnState.descCollection.HasUncommittedNewOrDroppedDescriptors() {
+
 			execCfg := ex.planner.ExecCfg()
 			if err := UpdateDescriptorCount(ex.Ctx(), execCfg, execCfg.SchemaChangerMetrics); err != nil {
-				log.Dev.Warningf(ex.Ctx(), "failed to scan descriptor table: %v", err)
+				log.Dev.Warningf(ex.Ctx(), "failed to update descriptor count metric: %v", err)
 			}
 		}
 		fallthrough
@@ -4572,6 +4571,16 @@ func (ex *connExecutor) notifyStatsRefresherOfNewTables(ctx context.Context) {
 			ex.planner.execCfg.StatsRefresher.NotifyMutation(desc, math.MaxInt32 /* rowsAffected */)
 		}
 	}
+	if cnt := ex.extraTxnState.descCollection.CountUncommittedNewOrDroppedDescriptors(); cnt > 0 {
+		// Notify the refresher of a mutation on the system.descriptor table.
+		// We conservatively assume that any transaction which creates or
+		desc, err := ex.extraTxnState.descCollection.ByIDWithLeased(ex.planner.txn).Get().Table(ctx, keys.DescriptorTableID)
+		if err != nil {
+			log.Dev.Warningf(ctx, "failed to fetch descriptor table to refresh stats: %v", err)
+			return
+		}
+		ex.planner.execCfg.StatsRefresher.NotifyMutation(desc, cnt)
+	}
 }
 
 func (ex *connExecutor) getDescIDGenerator() eval.DescIDGenerator {