Merge #79976 #80408 #80730

79976: backupccl: enable chaining of pts records for scheduled backups r=adityamaru a=adityamaru

This change does a few things:

1) `schedules.backup.gc_protection.enabled` now defaults to true.

2) Previously, database and wildcard table scheduled backups were
not allowed to perform chaining. With pts records now protecting
schema objects, even if the target of the backup expands between two
backups in a chain, the protection policy on the parent schema object
will automatically apply to it. So now, all forms of scheduled backups
can take advantage of chaining.

3) Added more tests to `schedule_pts_chaining.go`.

4) General spring cleaning of outdated comments, and assertions.

Informs: #67282

Release note: None

80408: added st_makeenvelope builtin r=otan a=fredbi

* fixes #80357

Release note (sql change): added builtin st_makeenvelope

Signed-off-by: Frédéric BIDON <frederic@oneconcern.com>

80730: sql: add more tests for unknown database errors r=mgartner a=mgartner

When connected to an non-existent database in versions 21.1 and prior,
querying catalog tables resulted in an unknown database error code
(`3D000`), but querying non-catalog tables resulted in an unknown table
error code (`42P01`). In 21.2 we made the error codes the same for both
cases (`3D000`). I couldn't find any tests that verified this behavior,
so this commit adds them.

Release note: None

Co-authored-by: Aditya Maru <adityamaru@gmail.com>
Co-authored-by: Frédéric BIDON <frederic@oneconcern.com>
Co-authored-by: Marcus Gartner <marcus@cockroachlabs.com>

docs/generated/settings/settings-for-tenants.txt

@@ -43,7 +43,7 @@ kv.replication_reports.interval	duration	1m0s	the frequency for generating the r
 kv.transaction.max_intents_bytes	integer	4194304	maximum number of bytes used to track locks in transactions
 kv.transaction.max_refresh_spans_bytes	integer	4194304	maximum number of bytes used to track refresh spans in serializable transactions
 kv.transaction.reject_over_max_intents_budget.enabled	boolean	false	if set, transactions that exceed their lock tracking budget (kv.transaction.max_intents_bytes) are rejected instead of having their lock spans imprecisely compressed
-schedules.backup.gc_protection.enabled	boolean	false	enable chaining of GC protection across backups run as part of a schedule; default is false
+schedules.backup.gc_protection.enabled	boolean	true	enable chaining of GC protection across backups run as part of a schedule
 security.ocsp.mode	enumeration	off	use OCSP to check whether TLS certificates are revoked. If the OCSP server is unreachable, in strict mode all certificates will be rejected and in lax mode all certificates will be accepted. [off = 0, lax = 1, strict = 2]
 security.ocsp.timeout	duration	3s	timeout before considering the OCSP server unreachable
 server.auth_log.sql_connections.enabled	boolean	false	if set, log SQL client connect and disconnect events (note: may hinder performance on loaded nodes)

docs/generated/settings/settings.html

@@ -58,7 +58,7 @@
 <tr><td><code>kv.transaction.max_intents_bytes</code></td><td>integer</td><td><code>4194304</code></td><td>maximum number of bytes used to track locks in transactions</td></tr>
 <tr><td><code>kv.transaction.max_refresh_spans_bytes</code></td><td>integer</td><td><code>4194304</code></td><td>maximum number of bytes used to track refresh spans in serializable transactions</td></tr>
 <tr><td><code>kv.transaction.reject_over_max_intents_budget.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if set, transactions that exceed their lock tracking budget (kv.transaction.max_intents_bytes) are rejected instead of having their lock spans imprecisely compressed</td></tr>
-<tr><td><code>schedules.backup.gc_protection.enabled</code></td><td>boolean</td><td><code>false</code></td><td>enable chaining of GC protection across backups run as part of a schedule; default is false</td></tr>
+<tr><td><code>schedules.backup.gc_protection.enabled</code></td><td>boolean</td><td><code>true</code></td><td>enable chaining of GC protection across backups run as part of a schedule</td></tr>
 <tr><td><code>security.ocsp.mode</code></td><td>enumeration</td><td><code>off</code></td><td>use OCSP to check whether TLS certificates are revoked. If the OCSP server is unreachable, in strict mode all certificates will be rejected and in lax mode all certificates will be accepted. [off = 0, lax = 1, strict = 2]</td></tr>
 <tr><td><code>security.ocsp.timeout</code></td><td>duration</td><td><code>3s</code></td><td>timeout before considering the OCSP server unreachable</td></tr>
 <tr><td><code>server.auth_log.sql_connections.enabled</code></td><td>boolean</td><td><code>false</code></td><td>if set, log SQL client connect and disconnect events (note: may hinder performance on loaded nodes)</td></tr>

docs/generated/sql/functions.md

@@ -2203,6 +2203,10 @@ calculated, the result is transformed back into a Geography with SRID 4326.</p>
 </span></td></tr>
 <tr><td><a name="st_makebox2d"></a><code>st_makebox2d(geometry_a: geometry, geometry_b: geometry) &rarr; box2d</code></td><td><span class="funcdesc"><p>Creates a box2d from two points. Errors if arguments are not two non-empty points.</p>
 </span></td></tr>
+<tr><td><a name="st_makeenvelope"></a><code>st_makeenvelope(xmin: <a href="float.html">float</a>, ymin: <a href="float.html">float</a>, xmax: <a href="float.html">float</a>, ymax: <a href="float.html">float</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Creates a rectangular Polygon from the minimum and maximum values for X and Y with SRID 0.</p>
+</span></td></tr>
+<tr><td><a name="st_makeenvelope"></a><code>st_makeenvelope(xmin: <a href="float.html">float</a>, ymin: <a href="float.html">float</a>, xmax: <a href="float.html">float</a>, ymax: <a href="float.html">float</a>, srid: <a href="int.html">int</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Creates a rectangular Polygon from the minimum and maximum values for X and Y with the given SRID.</p>
+</span></td></tr>
 <tr><td><a name="st_makepoint"></a><code>st_makepoint(x: <a href="float.html">float</a>, y: <a href="float.html">float</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a new Point with the given X and Y coordinates.</p>
 </span></td></tr>
 <tr><td><a name="st_makepoint"></a><code>st_makepoint(x: <a href="float.html">float</a>, y: <a href="float.html">float</a>, z: <a href="float.html">float</a>) &rarr; geometry</code></td><td><span class="funcdesc"><p>Returns a new Point with the given X, Y, and Z coordinates.</p>

pkg/ccl/backupccl/BUILD.bazel

@@ -129,6 +129,7 @@ go_library(
         "//pkg/util/metric",
         "//pkg/util/mon",
         "//pkg/util/protoutil",
+        "//pkg/util/randutil",
         "//pkg/util/retry",
         "//pkg/util/span",
         "//pkg/util/stop",

pkg/ccl/backupccl/backup.proto

@@ -159,38 +159,10 @@ message ScheduledBackupExecutionArgs {
   // i.e. the full schedule will have the inc schedules ID and vice versa.
   // A value of 0 indicates that there is no dependent schedule.
   int64 dependent_schedule_id = 6 [(gogoproto.customname) = "DependentScheduleID"];
-  // Previously, when running incremental backups with revision history, the GC
-  // TTL of the backup targets had to be at least as long as the gap between two
-  // backups. This was to ensure that the later backup in the chain could
-  // successfully backup all revisions since the `EndTime` of the previous
-  // backup until the `EndTime` of the current backup. Chaining of protected
-  // timestamp records across backups in the chain allows us to decouple the
-  // cadence of backups from the GC TTL by protecting what we need as we go.
-  //
-  // Chaining scheme:
-  // - The first scheduled full backup on successful completion, will write a
-  //   PTS record to protect all backed up data after the backups' EndTime. It
-  //   will then store this record on the dependent incremental schedule.
-  //
-  // - Thereafter, every execution of an incremental backup will update the PTS
-  //   record on the incremental schedule to protect the same spans as the
-  //   previous record stored on the schedule, but after the `EndTime` of the
-  //   current backup.
-  //
-  // - Subsequent full backups will release the pts record on the incremental
-  //   schedule, and write a new pts record protecting all backed up data after
-  //   the full backups' EndTime.
-  //   This happens on successful completion of the full backup to ensure that
-  //   it does not interfere with incrementals that might be started while the
-  //   the full backup is executing.
-  //
-  // NB: The PTS records written and managed by the backup schedule is
-  // independent of the PTS record written during the backup planning. Each
-  // backup job will continue to write a pts record for the duration of its
-  // execution. The schedule managed chaining of pts records across backups
-  // ensures that we are never trying to protect a span after a timestamp that
-  // might have fallen out of the GC window, without a previously written pts
-  // record protecting it from GC.
+
+  // ChainProtectedTimestampRecords indicates that chaining of protected
+  // timestamp records is enabled for this schedule. The chaining scheme works
+  // as described in `schedule_pts_chaining.go`.
   bool chain_protected_timestamp_records = 7;
 
   bytes protected_timestamp_record = 8 [

pkg/ccl/backupccl/backup_planning.go

@@ -1171,13 +1171,16 @@ func planSchedulePTSChaining(
 	if err != nil {
 		return err
 	}
+
+	// If chaining of protected timestamp records is disabled, noop.
 	if !args.ChainProtectedTimestampRecords {
 		return nil
 	}
 
 	if args.BackupType == ScheduledBackupExecutionArgs_FULL {
 		// Check if there is a dependent incremental schedule associated with the
 		// full schedule running the current backup.
+		//
 		// If present, the full backup on successful completion, will release the
 		// pts record found on the incremental schedule, and replace it with a new
 		// pts record protecting after the EndTime of the full backup.
@@ -1188,18 +1191,11 @@ func planSchedulePTSChaining(
 		_, incArgs, err := getScheduledBackupExecutionArgsFromSchedule(
 			ctx, env, txn, execCfg.InternalExecutor, args.DependentScheduleID)
 		if err != nil {
-			// If we are unable to resolve the dependent incremental schedule (it
-			// could have been dropped) we do not need to perform any chaining.
-			//
-			// TODO(adityamaru): Update this comment when DROP SCHEDULE is taught
-			// to clear the dependent ID. Once that is done, we should not encounter
-			// this error.
-			if jobs.HasScheduledJobNotFoundError(err) {
-				log.Warningf(ctx, "could not find dependent schedule with id %d",
-					args.DependentScheduleID)
-				return nil
-			}
-			return err
+			// We should always be able to resolve the dependent schedule ID. If the
+			// incremental schedule was dropped then it would have unlinked itself
+			// from the full schedule. Thus, we treat all errors as a problem.
+			return errors.NewAssertionErrorWithWrappedErrf(err,
+				"dependent schedule %d could not be resolved", args.DependentScheduleID)
 		}
 		backupDetails.SchedulePTSChainingRecord = &jobspb.SchedulePTSChainingRecord{
 			ProtectedTimestampRecord: incArgs.ProtectedTimestampRecord,
@@ -1210,7 +1206,7 @@ func planSchedulePTSChaining(
 		// that the job should update on successful completion, to protect data
 		// after the current backups' EndTime.
 		// We save this information on the job instead of reading it from the
-		// schedule on completion, so as to prevent an "overhang" incremental from
+		// schedule on completion, to prevent an "overhang" incremental from
 		// incorrectly pulling forward a pts record that was written by a new full
 		// backup that completed while the incremental was still executing.
 		//

pkg/ccl/backupccl/backup_test.go

@@ -5981,7 +5981,7 @@ func TestProtectedTimestampsDuringBackup(t *testing.T) {
 			runGCAndCheckTraceForSecondaryTenant(ctx, t, tc, systemTenantRunner,
 				false /* skipShouldQueue */, startPretty, checkProtectionPolicyTrace)
 		} else {
-			runGCAndCheckTrace(ctx, t, tc, systemTenantRunner,
+			runGCAndCheckTraceOnCluster(ctx, t, tc, systemTenantRunner,
 				false /* skipShouldQueue */, "defaultdb", "foo", checkProtectionPolicyTrace)
 		}
 
@@ -6016,7 +6016,7 @@ func TestProtectedTimestampsDuringBackup(t *testing.T) {
 			runGCAndCheckTraceForSecondaryTenant(ctx, t, tc, systemTenantRunner, false, /* skipShouldQueue */
 				startPretty, checkGCProgressesTrace)
 		} else {
-			runGCAndCheckTrace(ctx, t, tc, systemTenantRunner, false, /* skipShouldQueue */
+			runGCAndCheckTraceOnCluster(ctx, t, tc, systemTenantRunner, false, /* skipShouldQueue */
 				"defaultdb", "foo", checkGCProgressesTrace)
 		}
 	})
@@ -9232,6 +9232,7 @@ func TestExportRequestBelowGCThresholdOnDataExcludedFromBackup(t *testing.T) {
 		}
 	}
 	conn := tc.ServerConn(0)
+	sqlDB := sqlutils.MakeSQLRunner(conn)
 	_, err := conn.Exec("CREATE TABLE foo (k INT PRIMARY KEY, v BYTES)")
 	require.NoError(t, err)
 
@@ -9247,17 +9248,6 @@ func TestExportRequestBelowGCThresholdOnDataExcludedFromBackup(t *testing.T) {
 	require.NoError(t, err)
 
 	rRand, _ := randutil.NewTestRand()
-	upsertUntilBackpressure := func() {
-		for {
-			_, err := conn.Exec("UPSERT INTO foo VALUES (1, $1)",
-				randutil.RandBytes(rRand, 1<<15))
-			if testutils.IsError(err, "backpressure") {
-				break
-			}
-			require.NoError(t, err)
-		}
-	}
-
 	waitForTableSplit(t, conn, "foo", "defaultdb")
 	waitForReplicaFieldToBeSet(t, tc, conn, "foo", "defaultdb", func(r *kvserver.Replica) (bool, error) {
 		if r.GetMaxBytes() != tableRangeMaxBytes {
@@ -9268,16 +9258,16 @@ func TestExportRequestBelowGCThresholdOnDataExcludedFromBackup(t *testing.T) {
 
 	var tsBefore string
 	require.NoError(t, conn.QueryRow("SELECT cluster_logical_timestamp()").Scan(&tsBefore))
-	upsertUntilBackpressure()
-	runner := sqlutils.MakeSQLRunner(conn)
-	runGCAndCheckTrace(ctx, t, tc, runner, false /* skipShouldQueue */, "defaultdb", "foo", func(traceStr string) error {
-		const processedPattern = `(?s)shouldQueue=true.*processing replica.*GC score after GC`
-		processedRegexp := regexp.MustCompile(processedPattern)
-		if !processedRegexp.MatchString(traceStr) {
-			return errors.Errorf("%q does not match %q", traceStr, processedRegexp)
-		}
-		return nil
-	})
+	upsertUntilBackpressure(t, rRand, conn, "defaultdb", "foo")
+	runGCAndCheckTraceOnCluster(ctx, t, tc, sqlDB, false /* skipShouldQueue */, "defaultdb", "foo",
+		func(traceStr string) error {
+			const processedPattern = `(?s)shouldQueue=true.*processing replica.*GC score after GC`
+			processedRegexp := regexp.MustCompile(processedPattern)
+			if !processedRegexp.MatchString(traceStr) {
+				return errors.Errorf("%q does not match %q", traceStr, processedRegexp)
+			}
+			return nil
+		})
 
 	_, err = conn.Exec(fmt.Sprintf("BACKUP TABLE foo TO $1 AS OF SYSTEM TIME '%s'", tsBefore), localFoo)
 	testutils.IsError(err, "must be after replica GC threshold")
@@ -9337,18 +9327,6 @@ func TestExcludeDataFromBackupDoesNotHoldupGC(t *testing.T) {
 	runner.Exec(t, "ALTER TABLE test.foo CONFIGURE ZONE USING "+
 		"gc.ttlseconds = 1, range_max_bytes = $1, range_min_bytes = 1<<10;", tableRangeMaxBytes)
 
-	rRand, _ := randutil.NewTestRand()
-	upsertUntilBackpressure := func() {
-		for {
-			_, err := conn.Exec("UPSERT INTO test.foo VALUES (1, $1)",
-				randutil.RandBytes(rRand, 1<<15))
-			if testutils.IsError(err, "backpressure") {
-				break
-			}
-			require.NoError(t, err)
-		}
-	}
-
 	// Wait for the span config fields to apply.
 	waitForTableSplit(t, conn, "foo", "test")
 	waitForReplicaFieldToBeSet(t, tc, conn, "foo", "test", func(r *kvserver.Replica) (bool, error) {
@@ -9389,17 +9367,19 @@ func TestExcludeDataFromBackupDoesNotHoldupGC(t *testing.T) {
 	// Now that the backup has written a PTS record protecting the database, we
 	// check that the replica corresponding to `test.foo` continue to GC data
 	// since it has been marked as `exclude_data_from_backup`.
-	upsertUntilBackpressure()
-	runGCAndCheckTrace(ctx, t, tc, runner, false /* skipShouldQueue */, "test", "foo", func(traceStr string) error {
-		const processedPattern = `(?s)shouldQueue=true.*processing replica.*GC score after GC`
-		processedRegexp := regexp.MustCompile(processedPattern)
-		if !processedRegexp.MatchString(traceStr) {
-			return errors.Errorf("%q does not match %q", traceStr, processedRegexp)
-		}
-		thresh := thresholdFromTrace(t, traceStr)
-		require.Truef(t, afterBackup.Less(thresh), "%v >= %v", afterBackup, thresh)
-		return nil
-	})
+	rRand, _ := randutil.NewTestRand()
+	upsertUntilBackpressure(t, rRand, conn, "test", "foo")
+	runGCAndCheckTraceOnCluster(ctx, t, tc, runner, false, /* skipShouldQueue */
+		"test", "foo", func(traceStr string) error {
+			const processedPattern = `(?s)shouldQueue=true.*processing replica.*GC score after GC`
+			processedRegexp := regexp.MustCompile(processedPattern)
+			if !processedRegexp.MatchString(traceStr) {
+				return errors.Errorf("%q does not match %q", traceStr, processedRegexp)
+			}
+			thresh := thresholdFromTrace(t, traceStr)
+			require.Truef(t, afterBackup.Less(thresh), "%v >= %v", afterBackup, thresh)
+			return nil
+		})
 }
 
 // TestBackupRestoreSystemUsers tests RESTORE SYSTEM USERS feature which allows user to

pkg/ccl/backupccl/create_scheduled_backup.go

@@ -63,8 +63,8 @@ var scheduledBackupOptionExpectValues = map[string]sql.KVStringOptValidate{
 var scheduledBackupGCProtectionEnabled = settings.RegisterBoolSetting(
 	settings.TenantWritable,
 	"schedules.backup.gc_protection.enabled",
-	"enable chaining of GC protection across backups run as part of a schedule; default is false",
-	false, /* defaultValue */
+	"enable chaining of GC protection across backups run as part of a schedule",
+	true, /* defaultValue */
 ).WithPublic()
 
 // scheduledBackupEval is a representation of tree.ScheduledBackup, prepared
@@ -237,42 +237,6 @@ func pickFullRecurrenceFromIncremental(inc *scheduleRecurrence) *scheduleRecurre
 
 const scheduleBackupOp = "CREATE SCHEDULE FOR BACKUP"
 
-// canChainProtectedTimestampRecords returns true if the schedule is eligible to
-// participate in the chaining of protected timestamp records between backup
-// jobs running as part of the schedule.
-// Currently, full cluster backups, tenant backups and table backups with
-// revision history can enable chaining, as the spans to be protected remain
-// constant across all backups in the chain. Unlike in database backups where a
-// table could be created in between backups thereby widening the scope of what
-// is to be protected.
-func canChainProtectedTimestampRecords(p sql.PlanHookState, eval *scheduledBackupEval) bool {
-	if !scheduledBackupGCProtectionEnabled.Get(&p.ExecCfg().Settings.SV) ||
-		!eval.BackupOptions.CaptureRevisionHistory {
-		return false
-	}
-
-	// Check if this is a full cluster backup.
-	if eval.Coverage() == tree.AllDescriptors {
-		return true
-	}
-
-	// Check if there are any wildcard table selectors in the specified table
-	// targets. If we find a wildcard selector then we cannot chain PTS records
-	// because of the reason outlined in the comment above the method.
-	for _, t := range eval.Targets.Tables {
-		pattern, err := t.NormalizeTablePattern()
-		if err != nil {
-			return false
-		}
-		if _, ok := pattern.(*tree.AllTablesSelector); ok {
-			return false
-		}
-	}
-
-	// Return true if the backup has table targets or is backing up a tenant.
-	return eval.Targets.Tables != nil || eval.Targets.TenantID.IsSet()
-}
-
 // doCreateBackupSchedule creates requested schedule (or schedules).
 // It is a plan hook implementation responsible for the creating of scheduled backup.
 func doCreateBackupSchedules(
@@ -436,7 +400,7 @@ func doCreateBackupSchedules(
 	var inc *jobs.ScheduledJob
 	var incScheduledBackupArgs *ScheduledBackupExecutionArgs
 	if incRecurrence != nil {
-		chainProtectedTimestampRecords = canChainProtectedTimestampRecords(p, eval)
+		chainProtectedTimestampRecords = scheduledBackupGCProtectionEnabled.Get(&p.ExecCfg().Settings.SV)
 		backupNode.AppendToLatest = true
 
 		var incDests []string