Only run sidecardb change detection on serving primary tablets (#17051)

Signed-off-by: Manan Gupta <[email protected]>

go/test/endtoend/vreplication/sidecardb_test.go

@@ -91,7 +91,7 @@ func TestSidecarDB(t *testing.T) {
 
 		prs(t, keyspace, shard)
 		currentPrimary = tablet101
-		expectedChanges100 += numChanges
+		expectedChanges101 += numChanges
 		validateSidecarDBTables(t, tablet100, sidecarDBTables)
 		validateSidecarDBTables(t, tablet101, sidecarDBTables)
 		require.Equal(t, expectedChanges100, getNumExecutedDDLQueries(t, tablet100Port))
@@ -100,7 +100,7 @@ func TestSidecarDB(t *testing.T) {
 
 	t.Run("modify schema, prs, and self heal on new primary", func(t *testing.T) {
 		numChanges := modifySidecarDBSchema(t, vc, currentPrimary, ddls1)
-		expectedChanges101 += numChanges
+		expectedChanges100 += numChanges
 		prs(t, keyspace, shard)
 		// nolint
 		currentPrimary = tablet100

go/vt/vttablet/tabletserver/schema/engine.go

@@ -180,14 +180,15 @@ func (se *Engine) syncSidecarDB(ctx context.Context, conn *dbconnpool.DBConnecti
 // EnsureConnectionAndDB ensures that we can connect to mysql.
 // If tablet type is primary and there is no db, then the database is created.
 // This function can be called before opening the Engine.
-func (se *Engine) EnsureConnectionAndDB(tabletType topodatapb.TabletType) error {
+func (se *Engine) EnsureConnectionAndDB(tabletType topodatapb.TabletType, serving bool) error {
 	ctx := tabletenv.LocalContext()
 	// We use AllPrivs since syncSidecarDB() might need to upgrade the schema
 	conn, err := dbconnpool.NewDBConnection(ctx, se.env.Config().DB.AllPrivsWithDB())
 	if err == nil {
 		se.dbCreationFailed = false
 		// upgrade sidecar db if required, for a tablet with an existing database
-		if tabletType == topodatapb.TabletType_PRIMARY {
+		// only run DDL updates when a PRIMARY is transitioning to serving state.
+		if tabletType == topodatapb.TabletType_PRIMARY && serving {
 			if err := se.syncSidecarDB(ctx, conn); err != nil {
 				conn.Close()
 				return err
@@ -196,7 +197,7 @@ func (se *Engine) EnsureConnectionAndDB(tabletType topodatapb.TabletType) error
 		conn.Close()
 		return nil
 	}
-	if tabletType != topodatapb.TabletType_PRIMARY {
+	if tabletType != topodatapb.TabletType_PRIMARY || !serving {
 		return err
 	}
 	if merr, isSQLErr := err.(*sqlerror.SQLError); !isSQLErr || merr.Num != sqlerror.ERBadDb {

go/vt/vttablet/tabletserver/schema/engine_test.go

@@ -42,6 +42,7 @@ import (
 	"vitess.io/vitess/go/stats"
 	"vitess.io/vitess/go/test/utils"
 	"vitess.io/vitess/go/vt/dbconfigs"
+	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 	"vitess.io/vitess/go/vt/sqlparser"
 	"vitess.io/vitess/go/vt/vtenv"
 	"vitess.io/vitess/go/vt/vttablet/tabletserver/connpool"
@@ -95,6 +96,19 @@ func TestOpenAndReload(t *testing.T) {
 	assert.Equal(t, int64(0), se.tableFileSizeGauge.Counts()["msg"])
 	assert.Equal(t, int64(0), se.tableAllocatedSizeGauge.Counts()["msg"])
 
+	t.Run("EnsureConnectionAndDB", func(t *testing.T) {
+		// Verify that none of the following configurations run any schema change detection queries -
+		// 1. REPLICA serving
+		// 2. REPLICA non-serving
+		// 3. PRIMARY serving
+		err := se.EnsureConnectionAndDB(topodatapb.TabletType_REPLICA, true)
+		require.NoError(t, err)
+		err = se.EnsureConnectionAndDB(topodatapb.TabletType_PRIMARY, false)
+		require.NoError(t, err)
+		err = se.EnsureConnectionAndDB(topodatapb.TabletType_REPLICA, false)
+		require.NoError(t, err)
+	})
+
 	// Advance time some more.
 	db.AddQuery("select unix_timestamp()", sqltypes.MakeTestResult(sqltypes.MakeTestFields(
 		"t",
@@ -626,8 +640,10 @@ func newEngine(reloadTime time.Duration, idleTimeout time.Duration, schemaMaxAge
 	cfg.OlapReadPool.IdleTimeout = idleTimeout
 	cfg.TxPool.IdleTimeout = idleTimeout
 	cfg.SchemaVersionMaxAgeSeconds = schemaMaxAgeSeconds
+	dbConfigs := newDBConfigs(db)
+	cfg.DB = dbConfigs
 	se := NewEngine(tabletenv.NewEnv(vtenv.NewTestEnv(), cfg, "SchemaTest"))
-	se.InitDBConfig(newDBConfigs(db).DbaWithDB())
+	se.InitDBConfig(dbConfigs.DbaWithDB())
 	return se
 }
 

go/vt/vttablet/tabletserver/state_manager.go

@@ -140,7 +140,7 @@ type stateManager struct {
 
 type (
 	schemaEngine interface {
-		EnsureConnectionAndDB(topodatapb.TabletType) error
+		EnsureConnectionAndDB(topodatapb.TabletType, bool) error
 		Open() error
 		MakeNonPrimary()
 		MakePrimary(bool)
@@ -447,7 +447,7 @@ func (sm *stateManager) verifyTargetLocked(ctx context.Context, target *querypb.
 func (sm *stateManager) servePrimary() error {
 	sm.watcher.Close()
 
-	if err := sm.connect(topodatapb.TabletType_PRIMARY); err != nil {
+	if err := sm.connect(topodatapb.TabletType_PRIMARY, true); err != nil {
 		return err
 	}
 
@@ -476,7 +476,7 @@ func (sm *stateManager) unservePrimary() error {
 
 	sm.watcher.Close()
 
-	if err := sm.connect(topodatapb.TabletType_PRIMARY); err != nil {
+	if err := sm.connect(topodatapb.TabletType_PRIMARY, false); err != nil {
 		return err
 	}
 
@@ -500,7 +500,7 @@ func (sm *stateManager) serveNonPrimary(wantTabletType topodatapb.TabletType) er
 	sm.se.MakeNonPrimary()
 	sm.hs.MakeNonPrimary()
 
-	if err := sm.connect(wantTabletType); err != nil {
+	if err := sm.connect(wantTabletType, true); err != nil {
 		return err
 	}
 
@@ -518,7 +518,7 @@ func (sm *stateManager) unserveNonPrimary(wantTabletType topodatapb.TabletType)
 	sm.se.MakeNonPrimary()
 	sm.hs.MakeNonPrimary()
 
-	if err := sm.connect(wantTabletType); err != nil {
+	if err := sm.connect(wantTabletType, false); err != nil {
 		return err
 	}
 
@@ -528,8 +528,8 @@ func (sm *stateManager) unserveNonPrimary(wantTabletType topodatapb.TabletType)
 	return nil
 }
 
-func (sm *stateManager) connect(tabletType topodatapb.TabletType) error {
-	if err := sm.se.EnsureConnectionAndDB(tabletType); err != nil {
+func (sm *stateManager) connect(tabletType topodatapb.TabletType, serving bool) error {
+	if err := sm.se.EnsureConnectionAndDB(tabletType, serving); err != nil {
 		return err
 	}
 	if err := sm.se.Open(); err != nil {

go/vt/vttablet/tabletserver/state_manager_test.go

@@ -809,7 +809,7 @@ type testSchemaEngine struct {
 	failMySQL bool
 }
 
-func (te *testSchemaEngine) EnsureConnectionAndDB(tabletType topodatapb.TabletType) error {
+func (te *testSchemaEngine) EnsureConnectionAndDB(topodatapb.TabletType, bool) error {
 	if te.failMySQL {
 		te.failMySQL = false
 		return errors.New("intentional error")