Merge #152297

152297: sql: temporarily switch back to WaitForOneVersion for table version bump r=rafiss a=rafiss

65c953b changed the waiting logic for schema changes that were version-bump-only to wait for the new version to be published to all nodes, rather than waiting for all nodes to converge to one version. That change appears to have made tests flaky, so we temporarily go back to the old logic to unblock CI.

This also skips the related flaky tests.

informs #152051
informs #152043

informs #152293
informs #152049
informs #152083
informs #152279
informs #152278
informs #152274
informs #152230
informs #152226
informs #152148
informs #152204
informs #152191
informs #152155
informs #152098
informs #152144
informs #152072
informs #152073

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>

Author: craig[bot]

pkg/sql/catalog/lease/lease.go

@@ -558,8 +558,8 @@ func (m *Manager) WaitForOneVersion(
 func (m *Manager) WaitForNewVersion(
 	ctx context.Context,
 	descriptorId descpb.ID,
-	retryOpts retry.Options,
 	regions regionliveness.CachedDatabaseRegions,
+	retryOpts retry.Options,
 ) (catalog.Descriptor, error) {
 	if retryOpts.MaxRetries != 0 {
 		return nil, errors.New("The MaxRetries option shouldn't be set in WaitForNewVersion")

pkg/sql/catalog/lease/lease_test.go

@@ -557,6 +557,8 @@ func TestWaitForNewVersion(testingT *testing.T) {
 	defer leaktest.AfterTest(testingT)()
 	defer log.Scope(testingT).Close(testingT)
 
+	skip.WithIssue(testingT, 152051)
+
 	var params base.TestClusterArgs
 	params.ServerArgs.Knobs = base.TestingKnobs{
 		SQLLeaseManager: &lease.ManagerTestingKnobs{
@@ -586,7 +588,7 @@ func TestWaitForNewVersion(testingT *testing.T) {
 		timeoutCtx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 
-		_, err := leaseMgr.WaitForNewVersion(timeoutCtx, descID, retry.Options{}, nil)
+		_, err := leaseMgr.WaitForNewVersion(timeoutCtx, descID, nil, retry.Options{})
 		require.ErrorIs(t, err, context.DeadlineExceeded)
 	}
 
@@ -597,7 +599,7 @@ func TestWaitForNewVersion(testingT *testing.T) {
 		require.NoError(t, t.node(2).AcquireFreshestFromStore(ctx, descID))
 		t.expectLeases(descID, "/1/1 /1/2 /2/1 /2/2 /2/3")
 
-		desc, err := leaseMgr.WaitForNewVersion(context.Background(), descID, retry.Options{}, nil)
+		desc, err := leaseMgr.WaitForNewVersion(context.Background(), descID, nil, retry.Options{})
 		require.NoError(t, err)
 		require.Equal(t, desc.GetVersion(), descpb.DescriptorVersion(2))
 	}

pkg/sql/conn_executor_jobs.go

@@ -84,11 +84,13 @@ func (ex *connExecutor) waitForNewVersionPropagation(
 			continue
 		}
 
-		if _, err := ex.planner.LeaseMgr().WaitForNewVersion(ex.Ctx(), idVersion.ID, retry.Options{
+		// TODO(sql-foundations): Change this back to WaitForNewVersion once we
+		// address the flaky behavior around privilege and role membership caching.
+		if _, err := ex.planner.LeaseMgr().WaitForOneVersion(ex.Ctx(), idVersion.ID, cachedRegions, retry.Options{
 			InitialBackoff: time.Millisecond,
 			MaxBackoff:     time.Second,
 			Multiplier:     1.5,
-		}, cachedRegions); err != nil {
+		}); err != nil {
 			return err
 		}
 	}

pkg/sql/tests/allow_user_create_during_transaction_test.go

@@ -12,6 +12,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/base"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -24,6 +25,8 @@ func TestAllowUserCreateDuringTransaction(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	skip.WithIssue(t, 152043)
+
 	ctx := context.Background()
 	s, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{})
 	defer s.Stopper().Stop(ctx)