From be0bca33f89b9212abede7d9479933fbc5db9cde Mon Sep 17 00:00:00 2001 From: Manan Gupta <35839558+GuptaManan100@users.noreply.github.com> Date: Thu, 24 Oct 2024 07:03:02 +0530 Subject: [PATCH] Only run sidecardb change detection on serving primary tablets (#17051) Signed-off-by: Manan Gupta --- .../endtoend/vreplication/sidecardb_test.go | 4 ++-- go/vt/vttablet/tabletserver/schema/engine.go | 7 ++++--- .../tabletserver/schema/engine_test.go | 18 +++++++++++++++++- go/vt/vttablet/tabletserver/state_manager.go | 14 +++++++------- .../tabletserver/state_manager_test.go | 2 +- 5 files changed, 31 insertions(+), 14 deletions(-) diff --git a/go/test/endtoend/vreplication/sidecardb_test.go b/go/test/endtoend/vreplication/sidecardb_test.go index 391f7d60246..be9ce67a626 100644 --- a/go/test/endtoend/vreplication/sidecardb_test.go +++ b/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 diff --git a/go/vt/vttablet/tabletserver/schema/engine.go b/go/vt/vttablet/tabletserver/schema/engine.go index aadba5739c8..afb28080167 100644 --- a/go/vt/vttablet/tabletserver/schema/engine.go +++ b/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 { diff --git a/go/vt/vttablet/tabletserver/schema/engine_test.go b/go/vt/vttablet/tabletserver/schema/engine_test.go index 1c32b7acf73..caaf505779d 100644 --- a/go/vt/vttablet/tabletserver/schema/engine_test.go +++ b/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 } diff --git a/go/vt/vttablet/tabletserver/state_manager.go b/go/vt/vttablet/tabletserver/state_manager.go index 3fe78457b60..cae6a237dc8 100644 --- a/go/vt/vttablet/tabletserver/state_manager.go +++ b/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 { diff --git a/go/vt/vttablet/tabletserver/state_manager_test.go b/go/vt/vttablet/tabletserver/state_manager_test.go index f70e77de710..df819c6f05c 100644 --- a/go/vt/vttablet/tabletserver/state_manager_test.go +++ b/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")