From ba227f92866333a894be5a6ee5ecac3a74d68dc1 Mon Sep 17 00:00:00 2001 From: Venkatraju Date: Fri, 22 Aug 2025 14:14:42 -0700 Subject: [PATCH] locking fixes for GlobalReadLock held for logical backups (#690) --- go/vt/mysqlctl/fakemysqldaemon.go | 5 +---- go/vt/mysqlctl/mysql_daemon.go | 2 +- go/vt/mysqlctl/mysqld.go | 10 ++++++---- go/vt/mysqlctl/mysqlshellbackupengine.go | 9 ++------- go/vt/mysqlctl/query.go | 24 +++++++++++++++++++----- 5 files changed, 29 insertions(+), 21 deletions(-) diff --git a/go/vt/mysqlctl/fakemysqldaemon.go b/go/vt/mysqlctl/fakemysqldaemon.go index a2b5e66bd49..00ec122e6f8 100644 --- a/go/vt/mysqlctl/fakemysqldaemon.go +++ b/go/vt/mysqlctl/fakemysqldaemon.go @@ -797,11 +797,8 @@ func (fmd *FakeMysqlDaemon) AcquireGlobalReadLock(ctx context.Context) error { } // ReleaseGlobalReadLock is part of the MysqlDaemon interface. -func (fmd *FakeMysqlDaemon) ReleaseGlobalReadLock(ctx context.Context) error { +func (fmd *FakeMysqlDaemon) ReleaseGlobalReadLock(ctx context.Context) { if fmd.GlobalReadLock { fmd.GlobalReadLock = false - return nil } - - return errors.New("no read locks acquired yet") } diff --git a/go/vt/mysqlctl/mysql_daemon.go b/go/vt/mysqlctl/mysql_daemon.go index f7a564c7bec..7dbecb14ad5 100644 --- a/go/vt/mysqlctl/mysql_daemon.go +++ b/go/vt/mysqlctl/mysql_daemon.go @@ -136,7 +136,7 @@ type MysqlDaemon interface { AcquireGlobalReadLock(ctx context.Context) error // ReleaseGlobalReadLock release a lock acquired with the connection from the above function. - ReleaseGlobalReadLock(ctx context.Context) error + ReleaseGlobalReadLock(ctx context.Context) // Close will close this instance of Mysqld. It will wait for all dba // queries to be finished. diff --git a/go/vt/mysqlctl/mysqld.go b/go/vt/mysqlctl/mysqld.go index cb794cb06dd..f73cee24435 100644 --- a/go/vt/mysqlctl/mysqld.go +++ b/go/vt/mysqlctl/mysqld.go @@ -111,10 +111,12 @@ var ( // Mysqld is the object that represents a mysqld daemon running on this server. type Mysqld struct { - dbcfgs *dbconfigs.DBConfigs - dbaPool *dbconnpool.ConnectionPool - appPool *dbconnpool.ConnectionPool - lockConn *dbconnpool.PooledDBConnection + dbcfgs *dbconfigs.DBConfigs + dbaPool *dbconnpool.ConnectionPool + appPool *dbconnpool.ConnectionPool + + lockConnMutex sync.Mutex + lockConn *dbconnpool.PooledDBConnection capabilities capabilitySet diff --git a/go/vt/mysqlctl/mysqlshellbackupengine.go b/go/vt/mysqlctl/mysqlshellbackupengine.go index ac40d9adc17..bb1deeba5df 100644 --- a/go/vt/mysqlctl/mysqlshellbackupengine.go +++ b/go/vt/mysqlctl/mysqlshellbackupengine.go @@ -158,7 +158,7 @@ func (be *MySQLShellBackupEngine) ExecuteBackup(ctx context.Context, params Back // we need to release the global read lock in case the backup fails to start and // the lock wasn't released by releaseReadLock() yet. context might be expired, // so we pass a new one. - defer func() { _ = params.Mysqld.ReleaseGlobalReadLock(context.Background()) }() + defer func() { params.Mysqld.ReleaseGlobalReadLock(context.Background()) }() posBeforeBackup, err := params.Mysqld.PrimaryPosition(ctx) if err != nil { @@ -521,12 +521,7 @@ func releaseReadLock(ctx context.Context, reader io.Reader, params BackupParams, released = true params.Logger.Infof("mysql shell released its global read lock, doing the same") - - err := params.Mysqld.ReleaseGlobalReadLock(ctx) - if err != nil { - params.Logger.Errorf("unable to release global read lock: %v", err) - } - + params.Mysqld.ReleaseGlobalReadLock(ctx) params.Logger.Infof("global read lock released after %v", time.Since(lockAcquired)) } } diff --git a/go/vt/mysqlctl/query.go b/go/vt/mysqlctl/query.go index 7b1e1f0094b..706f5ae914f 100644 --- a/go/vt/mysqlctl/query.go +++ b/go/vt/mysqlctl/query.go @@ -29,6 +29,11 @@ import ( "vitess.io/vitess/go/vt/log" ) +const ( + acquireGlobalReadLockTimeout = time.Minute + releaseGlobalReadLockTimeout = 10 * time.Second +) + // getPoolReconnect gets a connection from a pool, tests it, and reconnects if // the connection is lost. func getPoolReconnect(ctx context.Context, pool *dbconnpool.ConnectionPool) (*dbconnpool.PooledDBConnection, error) { @@ -229,6 +234,9 @@ func (mysqld *Mysqld) fetchStatuses(ctx context.Context, pattern string) (map[st // ExecuteSuperQuery allows the user to execute a query as a super user. func (mysqld *Mysqld) AcquireGlobalReadLock(ctx context.Context) error { + mysqld.lockConnMutex.Lock() + defer mysqld.lockConnMutex.Unlock() + if mysqld.lockConn != nil { return errors.New("lock already acquired") } @@ -238,6 +246,8 @@ func (mysqld *Mysqld) AcquireGlobalReadLock(ctx context.Context) error { return err } + ctx, cancel := context.WithTimeout(ctx, acquireGlobalReadLockTimeout) + defer cancel() err = mysqld.executeSuperQueryListConn(ctx, conn, []string{"FLUSH TABLES WITH READ LOCK"}) if err != nil { conn.Recycle() @@ -248,19 +258,23 @@ func (mysqld *Mysqld) AcquireGlobalReadLock(ctx context.Context) error { return nil } -func (mysqld *Mysqld) ReleaseGlobalReadLock(ctx context.Context) error { +func (mysqld *Mysqld) ReleaseGlobalReadLock(ctx context.Context) { + mysqld.lockConnMutex.Lock() + defer mysqld.lockConnMutex.Unlock() + if mysqld.lockConn == nil { - return errors.New("no read locks acquired yet") + return } + ctx, cancel := context.WithTimeout(ctx, releaseGlobalReadLockTimeout) + defer cancel() err := mysqld.executeSuperQueryListConn(ctx, mysqld.lockConn, []string{"UNLOCK TABLES"}) if err != nil { - return err + log.Warningf("release global read lock failed: %v. closing connection", err) + mysqld.lockConn.Close() } - mysqld.lockConn.Recycle() mysqld.lockConn = nil - return nil } const (