Merge #155393 #155453 #155462 #155463

155393: schemachangerccl: limit number of stages tested for MR r=fqazi a=fqazi

  Previously, this test was timing out frequently because of the number of
  stages that we were testing causing it to hit the test timeout. This
  patch reduces the number of stages tested on MR for declarative schema
  changes during backup / restore. This patch also adjusts timeout waiting
  for schema changes to allow for stress runs for testing.

  Release note: None
  Fixes: #154993
  Fixes: #155137
  Fixes: #154786
  Fixes: #152076
  Fixes: #155327
  Fixes: #154805
  Fixes: #155147

155453: server: deflake TestCheckRestartSafe_Integration r=stevendanna a=stevendanna

This test seems to fail under DRPC.

Informs #155111

Release note: None

155462: sql: use existing PK index for inverted index valdiation r=fqazi a=fqazi

Previously, when we were validating inverted index, we made all first mutations public. For a PK swap operation this meant that incomplete primary indexes would be made public, which may not be fully ready for scans depending on the plan. This meant after recent changes to have better plans for alter primary key, we could end up scanning incomplete primary indexes. To address this, this patch ensures that only non-PK swap indexes are used for validating inverted indexes.

Fixes: #155145
Fixes: #155141
Fixes: #155053
Fixes: #155038

Release note: None

155463: spanconfigjob: include underlying error in job failure message r=spilchen a=spilchen

Previously, when the span config reconciliation job exhausted its internal retries, it would fail with a generic "reconciliation unsuccessful, failing job" message without including the underlying cause of the failure.

This change tracks the last reconciliation error and wraps it in the final job error message.

Closes #155380

Epic: None
Release note: none

Co-authored-by: Faizan Qazi <faizan@cockroachlabs.com>
Co-authored-by: Steven Danna <danna@cockroachlabs.com>
Co-authored-by: Matt Spilchen <matt.spilchen@cockroachlabs.com>

Author: 4 people

pkg/server/api_v2_test.go

@@ -575,7 +575,12 @@ func TestCheckRestartSafe_Integration(t *testing.T) {
 	ctx := context.Background()
 	var err error
 
-	testCluster := serverutils.StartCluster(t, 3, base.TestClusterArgs{})
+	testCluster := serverutils.StartCluster(t, 3, base.TestClusterArgs{
+		ServerArgs: base.TestServerArgs{
+			// TODO(155111): Test is flakey with DRPC enabled.
+			DefaultDRPCOption: base.TestDRPCDisabled,
+		},
+	})
 	defer testCluster.Stopper().Stop(ctx)
 
 	ts0 := testCluster.Server(0)

pkg/spanconfig/spanconfigjob/job.go

@@ -134,6 +134,7 @@ func (r *resumer) Resume(ctx context.Context, execCtxI interface{}) (jobErr erro
 	}
 
 	var lastCheckpoint = hlc.Timestamp{}
+	var lastErr error
 	const aWhile = 5 * time.Minute // arbitrary but much longer than a retry
 	for retrier := retry.StartWithCtx(ctx, retryOpts); retrier.Next(); {
 		started := timeutil.Now()
@@ -167,6 +168,7 @@ func (r *resumer) Resume(ctx context.Context, execCtxI interface{}) (jobErr erro
 				Checkpoint: rc.Checkpoint(),
 			})
 		}); err != nil {
+			lastErr = err
 			if shouldSkipRetry {
 				break
 			}
@@ -192,6 +194,9 @@ func (r *resumer) Resume(ctx context.Context, execCtxI interface{}) (jobErr erro
 		return nil // we're done here (the stopper was stopped, Reconcile exited cleanly)
 	}
 
+	if lastErr != nil {
+		return errors.Wrap(lastErr, "reconciliation unsuccessful, failing job")
+	}
 	return errors.Newf("reconciliation unsuccessful, failing job")
 }
 

pkg/spanconfig/spanconfigmanager/BUILD.bazel

@@ -43,6 +43,7 @@ go_test(
         "//pkg/sql/catalog",
         "//pkg/sql/isql",
         "//pkg/testutils",
+        "//pkg/testutils/jobutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",
         "//pkg/testutils/testcluster",

pkg/spanconfig/spanconfigmanager/manager_test.go

@@ -21,6 +21,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/jobutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/hlc"
@@ -365,6 +366,66 @@ func TestReconciliationJobErrorAndRecovery(t *testing.T) {
 	mu.Unlock()
 }
 
+// TestReconciliationJobFinalErrorIncludesCause verifies that the reconciliation
+// job surfaces the last reconciliation error when internal retries are exhausted.
+func TestReconciliationJobFinalErrorIncludesCause(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	const injectedErrMsg = "injected reconciliation failure"
+	injectedErr := errors.New(injectedErrMsg)
+
+	var jobID jobspb.JobID
+	srv, db, _ := serverutils.StartServer(t, base.TestServerArgs{
+		Knobs: base.TestingKnobs{
+			SpanConfig: &spanconfig.TestingKnobs{
+				ManagerDisableJobCreation: true, // disable automatic job creation; we'll create one explicitly
+				JobDisableInternalRetry:   true,
+				JobOnCheckpointInterceptor: func(_ hlc.Timestamp) error {
+					return injectedErr
+				},
+			},
+		},
+	})
+	defer srv.Stopper().Stop(ctx)
+	ts := srv.ApplicationLayer()
+
+	tdb := sqlutils.MakeSQLRunner(db)
+
+	manager := spanconfigmanager.New(
+		ts.InternalDB().(isql.DB),
+		ts.JobRegistry().(*jobs.Registry),
+		ts.AppStopper(),
+		ts.ClusterSettings(),
+		ts.SpanConfigReconciler().(spanconfig.Reconciler),
+		&spanconfig.TestingKnobs{
+			ManagerCreatedJobInterceptor: func(jobI interface{}) {
+				jobID = jobI.(*jobs.Job).ID()
+			},
+		},
+	)
+
+	started, err := manager.TestingCreateAndStartJobIfNoneExists(ctx, ts.ClusterSettings())
+	require.NoError(t, err)
+	require.True(t, started)
+
+	testutils.SucceedsSoon(t, func() error {
+		if jobID == 0 {
+			return errors.New("waiting for job to be created")
+		}
+		return nil
+	})
+
+	waitForJobState(t, tdb, jobID, jobs.StateFailed)
+
+	payload := jobutils.GetJobPayload(t, tdb, jobID)
+	require.NotNil(t, payload.FinalResumeError)
+	decodedErr := errors.DecodeError(ctx, *payload.FinalResumeError)
+	require.ErrorContains(t, decodedErr, "reconciliation unsuccessful, failing job")
+	require.ErrorContains(t, decodedErr, injectedErrMsg)
+}
+
 // TestReconciliationUsesRightCheckpoint verifies that the reconciliation
 // internal retry uses the right checkpoint during internal retries.
 func TestReconciliationUsesRightCheckpoint(t *testing.T) {

pkg/sql/backfill.go

@@ -1785,8 +1785,10 @@ func countExpectedRowsForInvertedIndex(
 	if withFirstMutationPublic {
 		// Make the mutations public in an in-memory copy of the descriptor and
 		// add it to the Collection's synthetic descriptors, so that we can use
-		// SQL below to perform the validation.
-		fakeDesc, err := tableDesc.MakeFirstMutationPublic(catalog.IgnoreConstraints, catalog.RetainDroppingColumns)
+		// SQL below to perform the validation. Avoid making PK swaps public, otherwise
+		// in the declarative schema changer we can select incomplete primary indexes
+		// for validation.
+		fakeDesc, err := tableDesc.MakeFirstMutationPublic(catalog.IgnoreConstraints, catalog.RetainDroppingColumns, catalog.IgnorePKSwaps)
 		if err != nil {
 			return 0, err
 		}

pkg/sql/logictest/testdata/logic_test/alter_table

@@ -5265,3 +5265,23 @@ statement ok
 RESET experimental_enable_unique_without_index_constraints
 
 subtest end
+
+# Tests for a regression found by #155145 where the wrong primary index could
+# be scanned for when validating inverted indexes recreated after an ALTER
+# PRIMARY KEY.
+subtest validate_alter_pk_with_inverted
+
+statement ok
+CREATE TABLE t_alter_pk_with_inverted(
+    a STRING NOT NULL,
+    d DECIMAL[],
+    INVERTED INDEX (d)
+  );
+
+statement ok
+INSERT INTO t_alter_pk_with_inverted VALUES ('test', ARRAY[1.0, 2.0]);
+
+statement ok
+ALTER TABLE t_alter_pk_with_inverted ALTER PRIMARY KEY USING COLUMNS (a) USING HASH;
+
+subtest end

pkg/sql/schemachanger/sctest/backup.go

@@ -129,7 +129,7 @@ const skipRate = .6
 
 // Stop once max stages has been hit.
 const maxStagesToTest = 8
-const maxStagesToTestMultiRegion = 6
+const maxStagesToTestMultiRegion = 4
 
 // shouldSkipBackup samples about 60% of total stages or up to
 // max stages.

pkg/sql/schemachanger/sctest/framework.go

@@ -965,6 +965,14 @@ func waitForSchemaChangesToSucceed(t *testing.T, tdb *sqlutils.SQLRunner) {
 }
 
 func waitForSchemaChangesToFinish(t *testing.T, tdb *sqlutils.SQLRunner) {
+	// Wait longer on stress for schema changes.
+	if skip.Stress() {
+		old := tdb.SucceedsSoonDuration
+		tdb.SucceedsSoonDuration = time.Minute * 2
+		defer func() {
+			tdb.SucceedsSoonDuration = old
+		}()
+	}
 	tdb.CheckQueryResultsRetry(
 		t, schemaChangeWaitQuery(`('succeeded', 'failed')`), [][]string{},
 	)