Merge #150641 #151849 #153734 #153815

150641: backup: remove useless fields in telemetry events r=kev-cao a=msbutler

Epic: none

Release note: none

151849: plpgsql: fix handling of annotations for DO blocks r=yuzefovich a=yuzefovich

**plpgsql: fix handling of annotations for DO blocks**

This commit should fix for good annotations handling for DO blocks that
was recently attempted to be fixed in 3b8ab38.
(That commit is partially reversed too).

The general issue is that in the PLpgSQL parser we use an ephemeral SQL
parser instance to process SQL statements. However, when processing the
DO block stmts we need to preserve the annotations state between each
stmt. This is now achieved by extending the SQL parser infrastructure to
give an option to override the initial annotations index as well as keep
on reusing the counter across multiple stmts. This allows us to
correctly count the number of annotations that we need for the whole DO
block during the initial parsing, and then we use the allocated
annotations container to set the items in the optbuild.

Fixes: #151848.

Release note (bug fix): Previously, CockroachDB node could crash when
executing DO stmts when they contain user-defined types (possibly
non-existing) in non-default configuration (additional logging like the
one controlled via `sql.log.all_statements.enabled` cluster setting needed
to be enabled). This bug was introduced in 25.1 release and is now fixed.

**tree: avoid index-of-bounds panics in GetAnnotation in production**

We've recently seen two panics when trying to access the Annotations
container and hitting the out-of-bounds condition. The annotations are
used to provide additional information about unresolved object names, so
if we simply return `nil` on an invalid access, we'll keep on using the
unresolved object name, which seems like a better option than process
crash. In test builds we'll keep on crashing.

Release note: None

153734: builtins: deflake TestGetSSTableMetricsSingleNode r=xinhaoz a=jbowens

Refactor and deflake TestGetSSTableMetricsSingleNode.

Fixes: #151742
Epic: none
Release note: none

153815: roachtest: delake pcr roachtests that offset reader tenant system tables r=jeffswenson a=msbutler

In this patch, we restart the reader tenant after the poller job bootstraps the reader catalog, to refresh the namespace cache. This is necessary because the reader tenant namespace cache believes the the priv table has id 100052 instead of the newly ingested id 52 from the source.

Fixes #153555

Epic: none

Release note: none

Co-authored-by: Michael Butler <[email protected]>
Co-authored-by: Yahor Yuzefovich <[email protected]>
Co-authored-by: Jackson Owens <[email protected]>

Author: 4 people

docs/generated/eventlog.md

@@ -3421,25 +3421,20 @@ logged whenever a BACKUP and RESTORE job completes or fails.
 | `TargetScope` | TargetScope is the largest scope of the targets that the user is backing up or restoring based on the following order: table < schema < database < full cluster. | no |
 | `IsMultiregionTarget` | IsMultiregionTarget is true if any of the targets contain objects with multi-region primitives. | no |
 | `TargetCount` | TargetCount is the number of targets the in the BACKUP/RESTORE. | no |
-| `DestinationSubdirType` | DestinationSubdirType is - latest: if using the latest subdir - standard: if using a date-based subdir - custom: if using a custom subdir that's not date-based | no |
 | `DestinationStorageTypes` | DestinationStorageTypes are the types of storage that the user is backing up to or restoring from. | no |
 | `DestinationAuthTypes` | DestinationAuthTypes are the types of authentication methods that the user is using to access the destination storage. | no |
 | `IsLocalityAware` | IsLocalityAware indicates if the BACKUP or RESTORE is locality aware. | no |
-| `AsOfInterval` | AsOfInterval is the time interval in nanoseconds between the statement timestamp and the timestamp resolved by the AS OF SYSTEM TIME expression. The interval is expressed in nanoseconds. | no |
 | `WithRevisionHistory` | WithRevisionHistory is true if the BACKUP includes revision history. | no |
 | `HasEncryptionPassphrase` | HasEncryptionPassphrase is true if the user provided an encryption passphrase to encrypt/decrypt their backup. | no |
 | `KMSType` | KMSType is the type of KMS the user is using to encrypt/decrypt their backup. | no |
 | `KMSCount` | KMSCount is the number of KMS the user is using. | no |
 | `Options` | Options contain all the names of the options specified by the user in the BACKUP or RESTORE statement. For options that are accompanied by a value, only those with non-empty values will be present.<br><br>It's important to note that there are no option values anywhere in the event payload. Future changes to telemetry should refrain from adding values to the payload unless they are properly redacted. | no |
-| `DebugPauseOn` | DebugPauseOn is the type of event that the restore should pause on for debugging purposes. Currently only "error" is supported. | no |
 | `JobID` | JobID is the ID of the BACKUP/RESTORE job. | no |
 | `ResultStatus` | ResultStatus indicates whether the job succeeded or failed. | no |
 | `ErrorText` | ErrorText is the text of the error that caused the job to fail. | partially |
 | `RecurringCron` | RecurringCron is the crontab for the incremental backup. | no |
 | `FullBackupCron` | FullBackupCron is the crontab for the full backup. | no |
 | `CustomFirstRunTime` | CustomFirstRunTime is the timestamp for the user configured first run time. Expressed as nanoseconds since the Unix epoch. | no |
-| `OnExecutionFailure` | OnExecutionFailure describes the desired behavior if the schedule fails to execute. | no |
-| `OnPreviousRunning` | OnPreviousRunning describes the desired behavior if the previously scheduled BACKUP is still running. | no |
 | `IgnoreExistingBackup` | IgnoreExistingBackup is true iff the BACKUP schedule should still be created even if a backup is already present in its destination. | no |
 | `ApplicationName` | The application name for the session where recovery event was created. | no |
 | `NumRows` | NumRows is the number of rows successfully imported, backed up or restored. | no |

pkg/backup/backup_job.go

@@ -666,7 +666,7 @@ func (b *backupResumer) Resume(ctx context.Context, execCtx interface{}) error {
 		}
 
 		// Collect telemetry, once per backup after resolving its destination.
-		collectTelemetry(ctx, backupManifest, initialDetails, details, true, b.job.ID())
+		collectTelemetry(ctx, backupManifest, initialDetails, details, b.job.ID())
 	}
 
 	// For all backups, partitioned or not, the main BACKUP manifest is stored at
@@ -984,7 +984,6 @@ func collectTelemetry(
 	ctx context.Context,
 	backupManifest *backuppb.BackupManifest,
 	initialDetails, backupDetails jobspb.BackupDetails,
-	licensed bool,
 	jobID jobspb.JobID,
 ) {
 	// sourceSuffix specifies if this schedule was created by a schedule.
@@ -1000,14 +999,6 @@ func collectTelemetry(
 	}
 
 	countSource("backup.total.started")
-	if backupManifest.IsIncremental() || backupDetails.EncryptionOptions != nil {
-		countSource("backup.using-enterprise-features")
-	}
-	if licensed {
-		countSource("backup.licensed")
-	} else {
-		countSource("backup.free")
-	}
 	if backupDetails.StartTime.IsEmpty() {
 		countSource("backup.span.full")
 	} else {
@@ -1031,33 +1022,6 @@ func collectTelemetry(
 			countSource("backup.encryption.kms")
 		}
 	}
-	if backupDetails.CollectionURI != "" {
-		countSource("backup.nested")
-		timeBaseSubdir := true
-		if _, err := time.Parse(backupbase.DateBasedIntoFolderName,
-			initialDetails.Destination.Subdir); err != nil {
-			timeBaseSubdir = false
-		}
-		if backupDetails.StartTime.IsEmpty() {
-			if !timeBaseSubdir {
-				countSource("backup.deprecated-full-nontime-subdir")
-			} else if initialDetails.Destination.Exists {
-				countSource("backup.deprecated-full-time-subdir")
-			} else {
-				countSource("backup.full-no-subdir")
-			}
-		} else {
-			if initialDetails.Destination.Subdir == backupbase.LatestFileName {
-				countSource("backup.incremental-latest-subdir")
-			} else if !timeBaseSubdir {
-				countSource("backup.deprecated-incremental-nontime-subdir")
-			} else {
-				countSource("backup.incremental-explicit-subdir")
-			}
-		}
-	} else {
-		countSource("backup.deprecated-non-collection")
-	}
 	if backupManifest.DescriptorCoverage == tree.AllDescriptors {
 		countSource("backup.targets.full_cluster")
 	}

pkg/backup/backup_telemetry.go

@@ -9,10 +9,8 @@ import (
 	"context"
 	"net/url"
 	"sort"
-	"strings"
 	"time"
 
-	"github.com/cockroachdb/cockroach/pkg/backup/backupbase"
 	"github.com/cockroachdb/cockroach/pkg/backup/backupdest"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/cockroach/pkg/jobs/jobspb"
@@ -47,10 +45,6 @@ const (
 	scheduledBackupJobEventType eventpb.RecoveryEventType = "scheduled_backup_job"
 	restoreJobEventType         eventpb.RecoveryEventType = "restore_job"
 
-	latestSubdirType   = "latest"
-	standardSubdirType = "standard"
-	customSubdirType   = "custom"
-
 	// Currently the telemetry event payload only contains keys of options. Future
 	// changes to telemetry should refrain from adding values to the payload
 	// unless they are properly redacted.
@@ -100,21 +94,6 @@ func createBackupRecoveryEvent(
 		}
 	}
 
-	timeBaseSubdir := true
-	var subdirType string
-	if _, err := time.Parse(backupbase.DateBasedIntoFolderName,
-		initialDetails.Destination.Subdir); err != nil {
-		timeBaseSubdir = false
-	}
-
-	if strings.EqualFold(initialDetails.Destination.Subdir, backupbase.LatestFileName) {
-		subdirType = latestSubdirType
-	} else if !timeBaseSubdir {
-		subdirType = customSubdirType
-	} else {
-		subdirType = standardSubdirType
-	}
-
 	authTypes := make(map[string]struct{})
 	storageTypes := make(map[string]struct{})
 	defaultURI, urisByLocalityKV, err := backupdest.GetURIsByLocalityKV(initialDetails.Destination.To, "")
@@ -156,14 +135,12 @@ func createBackupRecoveryEvent(
 		TargetScope:             largestScope.String(),
 		IsMultiregionTarget:     multiRegion,
 		TargetCount:             uint32(targetCount),
-		DestinationSubdirType:   subdirType,
 		IsLocalityAware:         isLocalityAware,
 		WithRevisionHistory:     initialDetails.RevisionHistory,
 		HasEncryptionPassphrase: passphrase,
 		KMSType:                 kms,
 		KMSCount:                uint32(kmsCount),
 		JobID:                   uint64(jobID),
-		AsOfInterval:            initialDetails.AsOfInterval,
 		Options:                 options,
 		ApplicationName:         initialDetails.ApplicationName,
 	}
@@ -245,25 +222,6 @@ func parseStorageAndAuth(uri string) (string, string, error) {
 	return storageType, authType, nil
 }
 
-func loggedSubdirType(subdir string) string {
-	timeBaseSubdir := true
-	var subdirType string
-	if _, err := time.Parse(backupbase.DateBasedIntoFolderName,
-		subdir); err != nil {
-		timeBaseSubdir = false
-	}
-
-	if strings.EqualFold(subdir, backupbase.LatestFileName) {
-		subdirType = latestSubdirType
-	} else if !timeBaseSubdir {
-		subdirType = customSubdirType
-	} else {
-		subdirType = standardSubdirType
-	}
-
-	return subdirType
-}
-
 // logCreateScheduleTelemetry publishes an eventpb.RecoveryEvent about a created
 // backup schedule.
 func logCreateScheduleTelemetry(
@@ -296,8 +254,6 @@ func logCreateScheduleTelemetry(
 	backupEvent.RecurringCron = recurringCron
 	backupEvent.FullBackupCron = fullCron
 	backupEvent.CustomFirstRunTime = firstRunNanos
-	backupEvent.OnPreviousRunning = jobspb.ScheduleDetails_WaitBehavior_name[int32(details.Wait)]
-	backupEvent.OnExecutionFailure = jobspb.ScheduleDetails_ErrorHandlingBehavior_name[int32(details.OnError)]
 	backupEvent.IgnoreExistingBackup = ignoreExisting
 
 	log.StructuredEvent(ctx, severity.INFO, &backupEvent)
@@ -312,7 +268,6 @@ func logRestoreTelemetry(
 	intoDB string,
 	newDBName string,
 	subdir string,
-	asOfInterval int64,
 	opts tree.RestoreOptions,
 	descsByTablePattern map[tree.TablePattern]catalog.Descriptor,
 	restoreDBs []catalog.DatabaseDescriptor,
@@ -410,9 +365,7 @@ func logRestoreTelemetry(
 		TargetScope:             largestScope.String(),
 		TargetCount:             uint32(targetCount),
 		IsMultiregionTarget:     multiRegion,
-		DestinationSubdirType:   loggedSubdirType(subdir),
 		IsLocalityAware:         localityAware,
-		AsOfInterval:            asOfInterval,
 		HasEncryptionPassphrase: passphrase,
 		KMSType:                 kmsType,
 		KMSCount:                uint32(kmsCount),

pkg/backup/backup_test.go

@@ -10297,11 +10297,9 @@ func TestBackupRestoreTelemetryEvents(t *testing.T) {
 		RecoveryType:            backupEventType,
 		TargetScope:             databaseScope.String(),
 		TargetCount:             2,
-		DestinationSubdirType:   standardSubdirType,
 		DestinationStorageTypes: []string{"nodelocal", "userfile"},
 		DestinationAuthTypes:    []string{"specified"},
 		IsLocalityAware:         true,
-		AsOfInterval:            -1 * time.Millisecond.Nanoseconds(),
 		WithRevisionHistory:     true,
 		ApplicationName:         "backup_test",
 	}
@@ -10332,11 +10330,9 @@ func TestBackupRestoreTelemetryEvents(t *testing.T) {
 		RecoveryType:            restoreEventType,
 		TargetScope:             tableScope.String(),
 		TargetCount:             1,
-		DestinationSubdirType:   latestSubdirType,
 		DestinationStorageTypes: []string{"nodelocal", "userfile"},
 		DestinationAuthTypes:    []string{"specified"},
 		IsLocalityAware:         true,
-		AsOfInterval:            0,
 		Options:                 []string{telemetryOptionIntoDB, telemetryOptionSkipMissingFK},
 		ApplicationName:         "backup_test",
 	}

pkg/backup/create_scheduled_backup_test.go

@@ -1432,15 +1432,13 @@ func TestCreateScheduledBackupTelemetry(t *testing.T) {
 
 	th, cleanup := newTestHelper(t)
 	defer cleanup()
-	var asOfInterval int64
 
 	// We'll be manipulating schedule time via th.env, but we can't fool actual backup
 	// when it comes to AsOf time.  So, override AsOf backup clause to be the current time.
 	useRealTimeAOST := func() func() {
 		knobs := th.cfg.TestingKnobs.(*jobs.TestingKnobs)
 		knobs.OverrideAsOfClause = func(clause *tree.AsOfClause, stmtTimestamp time.Time) {
 			expr, err := tree.MakeDTimestampTZ(th.cfg.DB.KV().Clock().PhysicalTime(), time.Microsecond)
-			asOfInterval = expr.Time.UnixNano() - stmtTimestamp.UnixNano()
 			require.NoError(t, err)
 			clause.Expr = expr
 		}
@@ -1472,15 +1470,11 @@ WITH SCHEDULE OPTIONS on_execution_failure = 'pause', ignore_existing_backups, f
 		RecoveryType:            createdScheduleEventType,
 		TargetScope:             clusterScope.String(),
 		TargetCount:             1,
-		DestinationSubdirType:   standardSubdirType,
 		DestinationStorageTypes: []string{"userfile"},
 		DestinationAuthTypes:    []string{"specified"},
-		AsOfInterval:            asOfInterval,
 		Options:                 []string{telemetryOptionDetached},
 		RecurringCron:           "@hourly",
 		FullBackupCron:          "@daily",
-		OnExecutionFailure:      "PAUSE_SCHED",
-		OnPreviousRunning:       "WAIT",
 		IgnoreExistingBackup:    true,
 		CustomFirstRunTime:      firstRun.UnixNano(),
 		ApplicationName:         "backup_test",
@@ -1501,10 +1495,8 @@ WITH SCHEDULE OPTIONS on_execution_failure = 'pause', ignore_existing_backups, f
 		RecoveryType:            scheduledBackupEventType,
 		TargetScope:             clusterScope.String(),
 		TargetCount:             1,
-		DestinationSubdirType:   standardSubdirType,
 		DestinationStorageTypes: []string{"userfile"},
 		DestinationAuthTypes:    []string{"specified"},
-		AsOfInterval:            asOfInterval,
 		Options:                 []string{telemetryOptionDetached},
 	}
 	requireRecoveryEvent(t, beforeBackup.UnixNano(), scheduledBackupEventType, expectedScheduledBackup)

pkg/backup/restore_planning.go

@@ -1953,11 +1953,6 @@ func doRestorePlan(
 		}
 	}
 
-	var asOfInterval int64
-	if !endTime.IsEmpty() {
-		asOfInterval = endTime.WallTime - p.ExtendedEvalContext().StmtTimestamp.UnixNano()
-	}
-
 	filteredTablesByID, err := maybeFilterMissingViews(
 		tablesByID,
 		typesByID,
@@ -2133,7 +2128,7 @@ func doRestorePlan(
 		}
 		resultsCh <- tree.Datums{tree.NewDInt(tree.DInt(jobID))}
 		collectRestoreTelemetry(ctx, jobID, restoreDetails, intoDB, newDBName, subdir, restoreStmt,
-			descsByTablePattern, restoreDBs, asOfInterval, p.SessionData().ApplicationName)
+			descsByTablePattern, restoreDBs, p.SessionData().ApplicationName)
 		return nil
 	}
 
@@ -2180,7 +2175,7 @@ func doRestorePlan(
 	// execution.
 	p.InternalSQLTxn().Descriptors().ReleaseAll(ctx)
 	collectRestoreTelemetry(ctx, sj.ID(), restoreDetails, intoDB, newDBName, subdir, restoreStmt,
-		descsByTablePattern, restoreDBs, asOfInterval, p.SessionData().ApplicationName)
+		descsByTablePattern, restoreDBs, p.SessionData().ApplicationName)
 	if err := sj.Start(ctx); err != nil {
 		return err
 	}
@@ -2200,20 +2195,13 @@ func collectRestoreTelemetry(
 	restoreStmt *tree.Restore,
 	descsByTablePattern map[tree.TablePattern]catalog.Descriptor,
 	restoreDBs []catalog.DatabaseDescriptor,
-	asOfInterval int64,
 	applicationName string,
 ) {
 	telemetry.Count("restore.total.started")
 	if restoreStmt.DescriptorCoverage == tree.AllDescriptors {
 		telemetry.Count("restore.full-cluster")
 	}
-	if restoreStmt.Subdir == nil {
-		telemetry.Count("restore.deprecated-subdir-syntax")
-	} else {
-		telemetry.Count("restore.collection")
-	}
-
-	logRestoreTelemetry(ctx, jobID, details, intoDB, newDBName, subdir, asOfInterval, restoreStmt.Options,
+	logRestoreTelemetry(ctx, jobID, details, intoDB, newDBName, subdir, restoreStmt.Options,
 		descsByTablePattern, restoreDBs, applicationName)
 }