Improve efficiency and accuracy of mysqld.GetVersionString

go/vt/env/env.go

@@ -24,6 +24,7 @@ import (
 	"path"
 	"path/filepath"
 	"strings"
+	"sync"
 )
 
 const (
@@ -33,6 +34,9 @@ const (
 	DefaultVtRoot = "/usr/local/vitess"
 )
 
+// Used to ensure that we only add /usr/sbin to the PATH once.
+var once sync.Once
+
 // VtRoot returns $VTROOT or tries to guess its value if it's not set.
 // This is the root for the 'vt' distribution, which contains bin/vttablet
 // for instance.
@@ -72,11 +76,15 @@ func VtMysqlRoot() (string, error) {
 		return root, nil
 	}
 
-	// otherwise let's look for mysqld in the PATH.
-	// ensure that /usr/sbin is included, as it might not be by default
-	// This is the default location for mysqld from packages.
-	newPath := fmt.Sprintf("/usr/sbin:%s", os.Getenv("PATH"))
-	os.Setenv("PATH", newPath)
+	// Otherwise let's look for mysqld in the PATH env var.
+	// Ensure that /usr/sbin is included, as it might not be by default
+	// and this is often the default location used by mysqld system
+	// packages (apt, dnf, etc).
+	once.Do(func() {
+		newPath := fmt.Sprintf("/usr/sbin:%s", os.Getenv("PATH"))
+		os.Setenv("PATH", newPath)
+	})
+
 	path, err := exec.LookPath("mysqld")
 	if err != nil {
 		return "", errors.New("VT_MYSQL_ROOT is not set and no mysqld could be found in your PATH")

go/vt/env/env_test.go

@@ -19,6 +19,8 @@ package env
 import (
 	"os"
 	"testing"
+
+	"github.com/stretchr/testify/require"
 )
 
 func TestVtDataRoot(t *testing.T) {
@@ -43,3 +45,44 @@ func TestVtDataRoot(t *testing.T) {
 		t.Errorf("The value of VtDataRoot should be %v, not %v.", passed, root)
 	}
 }
+
+func TestVtMysqlRoot(t *testing.T) {
+	envVar := "VT_MYSQL_ROOT"
+	originalMySQLRoot := os.Getenv(envVar)
+	defer os.Setenv(envVar, originalMySQLRoot)
+	originalPATH := os.Getenv("PATH")
+	defer os.Setenv("PATH", originalPATH)
+
+	testcases := []struct {
+		name   string
+		envVal string
+	}{
+		{
+			name:   "env var set",
+			envVal: "/home/mysql/binaries",
+		},
+		{
+			name: "env var unset",
+		},
+		{ // Second call allows us to verify that we're not adding to the PATH multiple times.
+			name: "env var unset again",
+		},
+	}
+
+	for _, tc := range testcases {
+		t.Run(tc.name, func(t *testing.T) {
+			os.Setenv(envVar, tc.envVal)
+			defer os.Setenv(envVar, "")
+
+			path, err := VtMysqlRoot()
+			if tc.envVal != "" {
+				require.Equal(t, tc.envVal, path)
+				require.NoError(t, err)
+			}
+			// We don't require a non-error as the test env may not have mysql installed.
+		})
+	}
+
+	// After all of the test runs, the PATH should only have had /usr/sbin added once.
+	require.Equal(t, "/usr/sbin:"+originalPATH, os.Getenv("PATH"))
+}

go/vt/mysqlctl/mysqld.go

@@ -62,8 +62,14 @@
 	"vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
-var (
+// The string we expect before the MySQL version number
+// in strings containing MySQL version information.
+const versionStringPrefix = "Ver "
 
+// How many bytes from MySQL error log to sample for error messages
+const maxLogFileSampleSize = 4096
+
+var (
 	// DisableActiveReparents is a flag to disable active
 	// reparents for safety reasons. It is used in three places:
 	// 1. in this file to skip registering the commands.
@@ -87,15 +93,15 @@
 
 	replicationConnectRetry = 10 * time.Second
 
-	versionRegex = regexp.MustCompile(`Ver ([0-9]+)\.([0-9]+)\.([0-9]+)`)
+	versionRegex = regexp.MustCompile(fmt.Sprintf(`%s([0-9]+)\.([0-9]+)\.([0-9]+)`, versionStringPrefix))
+	// The SQL query that will return a version string directly from
+	// a MySQL server that will comply with the versionRegex.
+	versionSQLQuery = fmt.Sprintf("select concat('%s', @@global.version, ' ', @@global.version_comment) as version", versionStringPrefix)
 
 	binlogEntryCommittedTimestampRegex = regexp.MustCompile("original_committed_timestamp=([0-9]+)")
 	binlogEntryTimestampGTIDRegexp     = regexp.MustCompile(`^#(.+) server id.*\bGTID\b`)
 )
 
-// How many bytes from MySQL error log to sample for error messages
-const maxLogFileSampleSize = 4096
-
 // Mysqld is the object that represents a mysqld daemon running on this server.
 type Mysqld struct {
 	dbcfgs  *dbconfigs.DBConfigs
@@ -1186,7 +1192,16 @@
 
 // GetVersionString is part of the MysqlExecutor interface.
 func (mysqld *Mysqld) GetVersionString(ctx context.Context) (string, error) {
-	// Execute as remote action on mysqlctld to ensure we get the actual running MySQL version.
+	// Try to query the mysqld instance directly.
+	// This query produces output that is compatible with what we expect from
+	// mysqld --version. For example: Ver 8.0.35 MySQL Community Server - GPL
+	qr, err := mysqld.FetchSuperQuery(ctx, versionSQLQuery)
+	if err == nil && len(qr.Rows) == 1 {
+		return qr.Rows[0][0].ToString(), nil
+
+	}
+	// Execute as remote action on mysqlctld to use the actual running MySQL
+	// version.
 	if socketFile != "" {
 		client, err := mysqlctlclient.New("unix", socketFile)
 		if err != nil {
@@ -1195,6 +1210,7 @@
 		defer client.Close()
 		return client.VersionString(ctx)
 	}
+	// Fall back to the sys exec method using mysqld --version.
 	return GetVersionString()
 }