Merge #150866 #151366 #151429 #151504 #151578 #151608 #151667

150866: sql: harden logging logic in an error case r=yuzefovich a=yuzefovich

We just saw a sentry report where we hit a nil pointer with `planner.curPlan.instrumentationHelper` being nil when trying to log a statement. This happened after we hit an error in `handleAOST`, so we short-circuited "dispatch to execution engine" part, meaning that the plan is left uninitialized. Previously, in eac1971 we attempted to handle such scenarios by tracking a boolean indicating whether the dispatch has occurred, but in this case we marked the boolean "true" (meaning it has occurred), yet we short-circuited due to an error.

This commit refactors how we ensure that the plan is initialized by simply checking whether the corresponding fields are set or not. In turn, this exposed the fact that we previously didn't unset `planner.curPlan` when resetting the planner, which is now fixed.

I spent some time trying to come up with a repro but didn't succeed, so there is no regression test.

Fixes: #150717.

Release note: None

151366: sql: improve stmt bundles a bit r=yuzefovich a=yuzefovich

This commit addresses two minor issues around the stmt bundles:
- previously, we would always include the value of `direct_columnar_scans_enabled` session variable even when it's not changed from the default. This was a bug where we overrode the value of `skip` boolean based on whether crdb_test build tag was set.
- it also hardens `debug sb recreate` command to ignore `cluster.preserve_downgrade_option` if it's set - it has no effect on the execution, yet it can break recreation on a new enough binary.

Epic: None
Release note: None

151429: allkeywords: replace switch with map in GetKeywordID r=yuzefovich a=yuzefovich

Long time ago in 74f630c we switched the implementation of keyword ID lookup in lexer from the map to a large generated switch. At that time it was more performant, but with the new swiss table implementation of hash maps in Go I was curious to see whether the map approach is now faster, and it turns out to be so, so this commit switches back to the map lookup.

```
name                   old time/op    new time/op    delta
Parse/simple-8           12.4µs ± 0%    12.1µs ± 0%  -2.05%  (p=0.000 n=8+9)
Parse/string-8           18.2µs ± 0%    18.0µs ± 0%  -1.24%  (p=0.000 n=10+9)
Parse/tpcc-delivery-8    25.0µs ± 0%    24.5µs ± 0%  -1.73%  (p=0.000 n=10+10)
Parse/account-8          32.9µs ± 0%    32.2µs ± 0%  -2.14%  (p=0.000 n=10+10)

name                   old alloc/op   new alloc/op   delta
Parse/simple-8           3.56kB ± 0%    3.56kB ± 0%    ~     (all equal)
Parse/string-8           3.83kB ± 0%    3.83kB ± 0%    ~     (all equal)
Parse/tpcc-delivery-8    6.14kB ± 0%    6.14kB ± 0%    ~     (all equal)
Parse/account-8          11.2kB ± 0%    11.2kB ± 0%    ~     (all equal)

name                   old allocs/op  new allocs/op  delta
Parse/simple-8             22.0 ± 0%      22.0 ± 0%    ~     (all equal)
Parse/string-8             26.0 ± 0%      26.0 ± 0%    ~     (all equal)
Parse/tpcc-delivery-8      41.0 ± 0%      41.0 ± 0%    ~     (all equal)
Parse/account-8            75.0 ± 0%      75.0 ± 0%    ~     (all equal)
```

Epic: None
Release note: None

151504: opt: fix incorrect filter elimination in join reordering r=mgartner a=mgartner

A bug in join reordering which incorrectly eliminated filters has been
fixed. The join reordering logic could sometimes eliminate filters of
the form `a=a` when constructing new inner-joins. It incorrectly assumed
these filters to be redundant because a column is always equivalent to
itself. However, `a=a` is a null-rejecting filter, so it can only be
omitted if `a` is guaranteed to be non-null by something else, like
another filter or a `NOT NULL` constraint.

There is no release note because, as far as we know, this bug is not
possible to hit in production. The only reproduction we have requires
optimization rules to be disabled, which can only happen in tests.

Fixes #146507

Release note: None


151578: rowexec: fix JoinReader benchmarks r=yuzefovich a=yuzefovich

We recently added an assertion that the lookup join with "equality columns are key" property never fetches more than one lookup row for each input row. In a couple of existing benchmarks this fact was violated, so this commit fixes the issue. Namely, previously we set it to indicate that we want cross-range parallelism, but now we actually have a separate boolean on the processor spec, which makes the fix simple.

Fixes: #151296.
Fixes: #151299.

Release note: None

151608: backup: refactor ValidateEndTimeAndTruncate r=jeffswenson a=kev-cao

This patch refactors `ValidateEndTimeAndTruncate` to zip together the slices of URIs, manifests, and locality information into a single slice. This allows for easier manipulation of the slices instead of treating them as separate identities.

Epic: None

Release note: None

151667: sqlstats: fix insights benchmark bugs r=kyle-a-wong a=dhartunian

- Transactions slice length was incorrectly initialized.
- Transactions needed to set a UUID in order to get observed by the `observeTransaction` function and actually trigger the code that's being benchmarked.
- Indexing into the `transactions` slice in the benchmark was incorrectly using `idx` instead of `i` and grabbing empty objects.

Resolves: #151300
Epic: None
Release note: None

Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Marcus Gartner <[email protected]>
Co-authored-by: Kevin Cao <[email protected]>
Co-authored-by: David Hartunian <[email protected]>

Author: 5 people

cpu.out

@@ -875,11 +875,69 @@ func GetLocalityInfo(
 	return info, nil
 }
 
+type BackupTreeEntry struct {
+	// The URI to the backup manifest.
+	Uri string
+	// The contents of the backup Manifest.
+	Manifest backuppb.BackupManifest
+	// The locality information for the backup.
+	LocalityInfo jobspb.RestoreDetails_BackupLocalityInfo
+}
+
+// zipBackupTreeEntries zips the default URIs, main backup manifests, and
+// locality information into a slice of backupChainEntry structs. It assumes
+// that the passed in slices are of the same length and that each entry at the
+// same index in each slice corresponds to the same backup.
+func zipBackupTreeEntries(
+	defaultURIs []string,
+	mainBackupManifests []backuppb.BackupManifest,
+	localityInfo []jobspb.RestoreDetails_BackupLocalityInfo,
+) ([]BackupTreeEntry, error) {
+	if len(defaultURIs) != len(mainBackupManifests) || len(defaultURIs) != len(localityInfo) {
+		return nil, errors.AssertionFailedf(
+			"length mismatch: defaultURIs %d, mainBackupManifests %d, localityInfo %d",
+			len(defaultURIs), len(mainBackupManifests), len(localityInfo),
+		)
+	}
+
+	entries := make([]BackupTreeEntry, len(mainBackupManifests))
+	for i := range mainBackupManifests {
+		entries[i] = BackupTreeEntry{
+			Uri:          defaultURIs[i],
+			Manifest:     mainBackupManifests[i],
+			LocalityInfo: localityInfo[i],
+		}
+	}
+
+	return entries, nil
+}
+
+// unzipBackupTreeEntries unzips a slice of backupChainEntry structs into
+// slices of their constituent parts: default URIs, main backup manifests, and
+// locality info and returns the individual slices.
+func unzipBackupTreeEntries(
+	entries []BackupTreeEntry,
+) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo) {
+	defaultURIs := make([]string, len(entries))
+	mainBackupManifests := make([]backuppb.BackupManifest, len(entries))
+	localityInfo := make([]jobspb.RestoreDetails_BackupLocalityInfo, len(entries))
+	for i := range entries {
+		defaultURIs[i] = entries[i].Uri
+		mainBackupManifests[i] = entries[i].Manifest
+		localityInfo[i] = entries[i].LocalityInfo
+	}
+	return defaultURIs, mainBackupManifests, localityInfo
+}
+
 // ValidateEndTimeAndTruncate checks that the requested target time, if
 // specified, is valid for the list of incremental backups resolved, truncating
 // the results to the backup that contains the target time.
 // The method also performs additional sanity checks to ensure the backups cover
 // the requested time.
+//
+// TODO (kev-cao): Refactor this function to accept/return the BackupTreeEntry
+// type and update surrounding caller logic to use it. This will allow us to
+// refactor out the zipping and unzipping logic.
 func ValidateEndTimeAndTruncate(
 	defaultURIs []string,
 	mainBackupManifests []backuppb.BackupManifest,
@@ -888,71 +946,70 @@ func ValidateEndTimeAndTruncate(
 	includeSkipped bool,
 	includeCompacted bool,
 ) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo, error) {
+	backupEntries, err := zipBackupTreeEntries(defaultURIs, mainBackupManifests, localityInfo)
+	if err != nil {
+		return nil, nil, nil, err
+	}
+
 	if !includeCompacted {
-		defaultURIs, mainBackupManifests, localityInfo = skipCompactedBackups(
-			defaultURIs, mainBackupManifests, localityInfo,
-		)
+		backupEntries = skipCompactedBackups(backupEntries)
 	}
 
 	if endTime.IsEmpty() {
 		if includeSkipped {
-			return defaultURIs, mainBackupManifests, localityInfo, nil
+			uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
+			return uris, manifests, locality, nil
 		}
-		uris, manifests, locality, err := ElideSkippedLayers(defaultURIs, mainBackupManifests, localityInfo)
-		if err != nil {
-			return nil, nil, nil, err
-		}
-		if err := validateContinuity(manifests); err != nil {
+		backupEntries = elideSkippedLayers(backupEntries)
+		if err := validateContinuity(backupEntries); err != nil {
 			return nil, nil, nil, err
 		}
+		uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
 		return uris, manifests, locality, nil
 	}
-	for i, b := range mainBackupManifests {
+	for i, b := range backupEntries {
 		// Find the backup that covers the requested time.
-		if !(b.StartTime.Less(endTime) && endTime.LessEq(b.EndTime)) {
+		if !(b.Manifest.StartTime.Less(endTime) && endTime.LessEq(b.Manifest.EndTime)) {
 			continue
 		}
 
 		// Ensure that the backup actually has revision history.
-		if !endTime.Equal(b.EndTime) {
-			if b.MVCCFilter != backuppb.MVCCFilter_All {
+		if !endTime.Equal(b.Manifest.EndTime) {
+			if b.Manifest.MVCCFilter != backuppb.MVCCFilter_All {
 				const errPrefix = "invalid RESTORE timestamp: restoring to arbitrary time requires that BACKUP for requested time be created with 'revision_history' option."
 				if i == 0 {
 					return nil, nil, nil, errors.Errorf(
 						errPrefix+" nearest backup time is %s",
-						timeutil.Unix(0, b.EndTime.WallTime).UTC(),
+						timeutil.Unix(0, b.Manifest.EndTime.WallTime).UTC(),
 					)
 				}
 				return nil, nil, nil, errors.Errorf(
 					errPrefix+" nearest BACKUP times are %s or %s",
 					timeutil.Unix(0, mainBackupManifests[i-1].EndTime.WallTime).UTC(),
-					timeutil.Unix(0, b.EndTime.WallTime).UTC(),
+					timeutil.Unix(0, b.Manifest.EndTime.WallTime).UTC(),
 				)
 			}
 			// Ensure that the revision history actually covers the requested time -
 			// while the BACKUP's start and end might contain the requested time for
 			// example if start time is 0 (full backup), the revision history was
 			// only captured since the GC window. Note that the RevisionStartTime is
 			// the latest for ranges backed up.
-			if endTime.LessEq(b.RevisionStartTime) {
+			if endTime.LessEq(b.Manifest.RevisionStartTime) {
 				return nil, nil, nil, errors.Errorf(
 					"invalid RESTORE timestamp: BACKUP for requested time only has revision history"+
-						" from %v", timeutil.Unix(0, b.RevisionStartTime.WallTime).UTC(),
+						" from %v", timeutil.Unix(0, b.Manifest.RevisionStartTime.WallTime).UTC(),
 				)
 			}
 		}
 		if includeSkipped {
-			return defaultURIs[:i+1], mainBackupManifests[:i+1], localityInfo[:i+1], nil
-		}
-		uris, manifests, locality, err := ElideSkippedLayers(
-			defaultURIs[:i+1], mainBackupManifests[:i+1], localityInfo[:i+1],
-		)
-		if err != nil {
-			return nil, nil, nil, err
+			uris, manifests, locality := unzipBackupTreeEntries(backupEntries[:i+1])
+			return uris, manifests, locality, nil
 		}
-		if err := validateContinuity(manifests); err != nil {
+		backupEntries = elideSkippedLayers(backupEntries[:i+1])
+		if err := validateContinuity(backupEntries); err != nil {
 			return nil, nil, nil, err
 		}
+		uris, manifests, locality := unzipBackupTreeEntries(backupEntries)
 		return uris, manifests, locality, nil
 	}
 
@@ -962,68 +1019,58 @@ func ValidateEndTimeAndTruncate(
 }
 
 // skipCompactedBackups removes any compacted backups from the list of
-// backups and returns the updated list of URIs, manifests, and locality info.
+// backups and returns the updated list backup entries.
 //
 // NOTE: This function modifies the underlying memory of the slices passed in.
-func skipCompactedBackups(
-	defaultURIs []string,
-	manifests []backuppb.BackupManifest,
-	localityInfo []jobspb.RestoreDetails_BackupLocalityInfo,
-) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo) {
-	for i := len(manifests) - 1; i >= 0; i-- {
-		if manifests[i].IsCompacted {
-			defaultURIs = slices.Delete(defaultURIs, i, i+1)
-			manifests = slices.Delete(manifests, i, i+1)
-			localityInfo = slices.Delete(localityInfo, i, i+1)
+func skipCompactedBackups(backupEntries []BackupTreeEntry) []BackupTreeEntry {
+	for i := len(backupEntries) - 1; i >= 0; i-- {
+		if backupEntries[i].Manifest.IsCompacted {
+			backupEntries = slices.Delete(backupEntries, i, i+1)
 		}
 	}
-	return defaultURIs, manifests, localityInfo
+	return backupEntries
 }
 
 // validateContinuity checks that the backups are continuous.
-func validateContinuity(manifests []backuppb.BackupManifest) error {
-	if len(manifests) == 0 {
+func validateContinuity(backupEntries []BackupTreeEntry) error {
+	if len(backupEntries) == 0 {
 		return errors.AssertionFailedf("an empty chain of backups cannot cover an end time")
 	}
-	for i := range len(manifests) - 1 {
-		if !manifests[i].EndTime.Equal(manifests[i+1].StartTime) {
+	for i := range len(backupEntries) - 1 {
+		if !backupEntries[i].Manifest.EndTime.Equal(backupEntries[i+1].Manifest.StartTime) {
 			return errors.AssertionFailedf(
 				"backups are not continuous: %dth backup ends at %+v, %dth backup starts at %+v",
-				i, manifests[i].EndTime,
-				i+1, manifests[i+1].StartTime,
+				i, backupEntries[i].Manifest.EndTime,
+				i+1, backupEntries[i+1].Manifest.StartTime,
 			)
 		}
 	}
 	return nil
 }
 
-// ElideSkippedLayers removes backups that are skipped in the backup chain and
+// elideSkippedLayers removes backups that are skipped in the backup chain and
 // ensures only backups that will be used in the restore are returned.
 //
 // Note: This assumes that the provided backups are sorted in increasing order
 // by end time, and then sorted in increasing order by start time to break ties.
-func ElideSkippedLayers(
-	uris []string, backups []backuppb.BackupManifest, loc []jobspb.RestoreDetails_BackupLocalityInfo,
-) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo, error) {
-	uris, backups, loc = elideDuplicateEndTimes(uris, backups, loc)
-	i := len(backups) - 1
+func elideSkippedLayers(backupEntries []BackupTreeEntry) []BackupTreeEntry {
+	backupEntries = elideDuplicateEndTimes(backupEntries)
+	i := len(backupEntries) - 1
 	for i > 0 {
 		// Find j such that backups[j] is parent of backups[i].
 		j := i - 1
-		for j >= 0 && !backups[i].StartTime.Equal(backups[j].EndTime) {
+		for j >= 0 && !backupEntries[i].Manifest.StartTime.Equal(backupEntries[j].Manifest.EndTime) {
 			j--
 		}
 		// If there are backups between i and j, remove them.
 		// If j is less than 0, then no parent was found so nothing to skip.
 		if j != i-1 && j >= 0 {
-			uris = slices.Delete(uris, j+1, i)
-			backups = slices.Delete(backups, j+1, i)
-			loc = slices.Delete(loc, j+1, i)
+			backupEntries = slices.Delete(backupEntries, j+1, i)
 		}
 		// Move up to check the chain from j now.
 		i = j
 	}
-	return uris, backups, loc, nil
+	return backupEntries
 }
 
 // elideDuplicateEndTimes ensures that backups in a list of backups are
@@ -1034,24 +1081,21 @@ func ElideSkippedLayers(
 // by end time, and then sorted in increasing order by start time to break ties.
 // This is the case for backups being returned by storage clients due to us
 // encoding backup paths in a way that ensures this order.
-func elideDuplicateEndTimes(
-	uris []string, backups []backuppb.BackupManifest, loc []jobspb.RestoreDetails_BackupLocalityInfo,
-) ([]string, []backuppb.BackupManifest, []jobspb.RestoreDetails_BackupLocalityInfo) {
-	for i := range len(backups) - 1 {
+func elideDuplicateEndTimes(backupEntries []BackupTreeEntry) []BackupTreeEntry {
+	for i := range len(backupEntries) - 1 {
 		j := i + 1
 		// Find j such that backups[j] no longer shares the same end time as
 		// backups[i].
-		for j < len(backups) && backups[i].EndTime.Equal(backups[j].EndTime) {
+		for j < len(backupEntries) &&
+			backupEntries[i].Manifest.EndTime.Equal(backupEntries[j].Manifest.EndTime) {
 			j++
 		}
 		// If there exists backups between i and j, remove them.
 		if j > i+1 {
-			uris = slices.Delete(uris, i+1, j)
-			backups = slices.Delete(backups, i+1, j)
-			loc = slices.Delete(loc, i+1, j)
+			backupEntries = slices.Delete(backupEntries, i+1, j)
 		}
 	}
-	return uris, backups, loc
+	return backupEntries
 }
 
 // GetBackupIndexAtTime returns the index of the latest backup in

pkg/backup/backupinfo/manifest_handling.go

@@ -179,6 +179,12 @@ func runBundleRecreate(cmd *cobra.Command, args []string) (resErr error) {
 		initStmts := strings.Split(strings.Join(lines, "\n"), "SET CLUSTER SETTING")
 		for i := 1; i < len(initStmts); i++ {
 			initStmts[i] = "SET CLUSTER SETTING " + initStmts[i]
+			if strings.Contains(initStmts[i], "cluster.preserve_downgrade_option") {
+				// This cluster setting can prevent the bundle from being
+				// recreated on a new enough binary, so we'll skip it.
+				initStmts = append(initStmts[:i], initStmts[i+1:]...)
+				i--
+			}
 		}
 		// All stmts before the first SET CLUSTER SETTING are SET stmts. We need
 		// to handle 'SET database = ' stmt separately if found - the target

pkg/cli/statement_bundle.go

@@ -694,11 +694,6 @@ func (ex *connExecutor) execStmtInOpenState(
 		_ = p.FormatAstAsRedactableString(stmt.AST, p.extendedEvalCtx.Annotations)
 	}
 
-	// This flag informs logging decisions.
-	// Some statements are not dispatched to the execution engine and need
-	// some special plan initialization for logging.
-	dispatchToExecEngine := false
-
 	var logErr error
 	defer func() {
 		// Do not log if this is an eventTxnCommittedDueToDDL event. In that case,
@@ -708,10 +703,12 @@ func (ex *connExecutor) execStmtInOpenState(
 			return
 		}
 
-		// If we did not dispatch to the execution engine, we need to initialize
-		// the plan here.
-		if !dispatchToExecEngine {
+		if p.curPlan.stmt == nil || p.curPlan.instrumentation == nil {
+			// We short-circuited before we could initialize some fields that
+			// are needed for logging, so do that here.
 			p.curPlan.init(&p.stmt, &p.instrumentation)
+		}
+		if logErr == nil {
 			if p, ok := retPayload.(payloadWithError); ok {
 				logErr = p.errorCause()
 			}
@@ -932,8 +929,6 @@ func (ex *connExecutor) execStmtInOpenState(
 		return nil, nil, nil
 	}
 
-	dispatchToExecEngine = true
-
 	// Check if we need to auto-commit the transaction due to DDL.
 	if ev, payload := ex.maybeAutoCommitBeforeDDL(ctx, ast); ev != nil {
 		return ev, payload, nil
@@ -1707,11 +1702,6 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 		_ = p.FormatAstAsRedactableString(vars.stmt.AST, p.extendedEvalCtx.Annotations)
 	}
 
-	// This flag informs logging decisions.
-	// Some statements are not dispatched to the execution engine and need
-	// some special plan initialization for logging.
-	dispatchToExecEngine := false
-
 	defer processCleanupFunc(func() {
 		// Do not log if this is an eventTxnCommittedDueToDDL event. In that case,
 		// the transaction is committed, and the current statement is executed
@@ -1720,10 +1710,12 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 			return
 		}
 
-		// If we did not dispatch to the execution engine, we need to initialize
-		// the plan here.
-		if !dispatchToExecEngine {
+		if p.curPlan.stmt == nil || p.curPlan.instrumentation == nil {
+			// We short-circuited before we could initialize some fields that
+			// are needed for logging, so do that here.
 			p.curPlan.init(&p.stmt, &p.instrumentation)
+		}
+		if vars.logErr == nil {
 			if p, ok := retPayload.(payloadWithError); ok {
 				vars.logErr = p.errorCause()
 			}
@@ -1977,8 +1969,6 @@ func (ex *connExecutor) execStmtInOpenStateWithPausablePortal(
 		return nil, nil, nil
 	}
 
-	dispatchToExecEngine = true
-
 	// Check if we need to auto-commit the transaction due to DDL.
 	if ev, payload := ex.maybeAutoCommitBeforeDDL(ctx, vars.ast); ev != nil {
 		return ev, payload, nil

pkg/sql/conn_executor_exec.go

@@ -1132,7 +1132,9 @@ func (c *stmtEnvCollector) PrintSessionSettings(w io.Writer, sv *settings.Values
 		case "direct_columnar_scans_enabled":
 			// In test builds we might randomize some setting defaults, so
 			// we need to ignore them to make the tests deterministic.
-			skip = buildutil.CrdbTestBuild
+			if buildutil.CrdbTestBuild {
+				skip = true
+			}
 		case "role":
 			// If a role is set, we comment it out in env.sql. Otherwise, running
 			// 'debug sb recreate' will fail with a non-existent user/role error.