Repository: vitessio/vitess
Pull request #6526 (2020-08-01 22:53:59)

Minor vindex fixes:
 * Fix binary vindex type to have cost 0 (since it's an identity index)
 * Fixes/additions for vindex tests for some alternate data types and
larger input values.

Signed-off-by: Jacques Grove <aquarapid@gmail.com>


Patch:
diff --git a/go/vt/dbconfigs/dbconfigs.go b/go/vt/dbconfigs/dbconfigs.go
index 2c5d544153..efabbe9d5e 100644
--- a/go/vt/dbconfigs/dbconfigs.go
+++ b/go/vt/dbconfigs/dbconfigs.go
@@ -223,11 +223,6 @@ func (dbcfgs *DBConfigs) AppDebugWithDB() Connector {
 	return dbcfgs.makeParams(&dbcfgs.appdebugParams, true)
 }
 
-// AllPrivsConnector returns connection parameters for appdebug with no dbname set.
-func (dbcfgs *DBConfigs) AllPrivsConnector() Connector {
-	return dbcfgs.makeParams(&dbcfgs.allprivsParams, false)
-}
-
 // AllPrivsWithDB returns connection parameters for appdebug with dbname set.
 func (dbcfgs *DBConfigs) AllPrivsWithDB() Connector {
 	return dbcfgs.makeParams(&dbcfgs.allprivsParams, true)
diff --git a/go/vt/vtgate/vindexes/binary.go b/go/vt/vtgate/vindexes/binary.go
index 2affb723c0..95c8208a69 100644
--- a/go/vt/vtgate/vindexes/binary.go
+++ b/go/vt/vtgate/vindexes/binary.go
@@ -46,7 +46,7 @@ func (vind *Binary) String() string {
 
 // Cost returns the cost as 1.
 func (vind *Binary) Cost() int {
-	return 1
+	return 0
 }
 
 // IsUnique returns true since the Vindex is unique.
diff --git a/go/vt/vttablet/tabletmanager/rpc_replication.go b/go/vt/vttablet/tabletmanager/rpc_replication.go
index f6d9087584..1db7e7dccd 100644
--- a/go/vt/vttablet/tabletmanager/rpc_replication.go
+++ b/go/vt/vttablet/tabletmanager/rpc_replication.go
@@ -241,6 +241,11 @@ func (tm *TabletManager) InitMaster(ctx context.Context) (string, error) {
 		return "", err
 	}
 
+	// If using semi-sync, we need to enable it before going read-write.
+	if err := tm.fixSemiSync(topodatapb.TabletType_MASTER); err != nil {
+		return "", err
+	}
+
 	// Set the server read-write, from now on we can accept real
 	// client writes. Note that if semi-sync replication is enabled,
 	// we'll still need some replicas to be able to commit transactions.
@@ -251,13 +256,6 @@ func (tm *TabletManager) InitMaster(ctx context.Context) (string, error) {
 	if err := tm.changeTypeLocked(ctx, topodatapb.TabletType_MASTER); err != nil {
 		return "", err
 	}
-
-	// Enforce semi-sync after changing the type to master. Otherwise, the
-	// master will hang while trying to create the database.
-	if err := tm.fixSemiSync(topodatapb.TabletType_MASTER); err != nil {
-		return "", err
-	}
-
 	return mysql.EncodePosition(pos), nil
 }
 
diff --git a/go/vt/vttablet/tabletserver/schema/engine.go b/go/vt/vttablet/tabletserver/schema/engine.go
index cdbcb635d9..d033c0fac5 100644
--- a/go/vt/vttablet/tabletserver/schema/engine.go
+++ b/go/vt/vttablet/tabletserver/schema/engine.go
@@ -24,7 +24,6 @@ import (
 	"sync"
 	"time"
 
-	"vitess.io/vitess/go/vt/dbconnpool"
 	"vitess.io/vitess/go/vt/vtgate/evalengine"
 
 	"golang.org/x/net/context"
@@ -41,7 +40,6 @@ import (
 	"vitess.io/vitess/go/vt/vttablet/tabletserver/tabletenv"
 
 	binlogdatapb "vitess.io/vitess/go/vt/proto/binlogdata"
-	topodatapb "vitess.io/vitess/go/vt/proto/topodata"
 	vtrpcpb "vitess.io/vitess/go/vt/proto/vtrpc"
 )
 
@@ -73,9 +71,6 @@ type Engine struct {
 
 	conns *connpool.Pool
 	ticks *timer.Timer
-
-	// dbCreationFailed is for preventing log spam.
-	dbCreationFailed bool
 }
 
 // NewEngine creates a new Engine.
@@ -115,49 +110,6 @@ func (se *Engine) InitDBConfig(cp dbconfigs.Connector) {
 	se.cp = cp
 }
 
-// EnsureConnectionAndDB ensures that we can connect to mysql.
-// If tablet type is master 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 {
-	ctx := tabletenv.LocalContext()
-	conn, err := dbconnpool.NewDBConnection(ctx, se.env.Config().DB.AppWithDB())
-	if err == nil {
-		conn.Close()
-		se.dbCreationFailed = false
-		return nil
-	}
-	if tabletType != topodatapb.TabletType_MASTER {
-		return err
-	}
-	if merr, isSQLErr := err.(*mysql.SQLError); !isSQLErr || merr.Num != mysql.ERBadDb {
-		return err
-	}
-
-	// We are master and db is not found. Let's create it.
-	// We use allprivs instead of DBA because we want db create to fail if we're read-only.
-	conn, err = dbconnpool.NewDBConnection(ctx, se.env.Config().DB.AllPrivsConnector())
-	if err != nil {
-		return vterrors.Wrap(err, "allprivs connection failed")
-	}
-	defer conn.Close()
-
-	dbname := se.env.Config().DB.DBName
-	_, err = conn.ExecuteFetch(fmt.Sprintf("create database if not exists `%s`", dbname), 1, false)
-	if err != nil {
-		if !se.dbCreationFailed {
-			// This is the first failure.
-			log.Errorf("db creation failed for %v: %v, will keep retrying", dbname, err)
-			se.dbCreationFailed = true
-		}
-		return err
-	}
-
-	se.dbCreationFailed = false
-	log.Infof("db %v created", dbname)
-	se.dbCreationFailed = false
-	return nil
-}
-
 // Open initializes the Engine. Calling Open on an already
 // open engine is a no-op.
 func (se *Engine) Open() error {
diff --git a/go/vt/vttablet/tabletserver/state_manager.go b/go/vt/vttablet/tabletserver/state_manager.go
index 9135dc38b8..00537d6e70 100644
--- a/go/vt/vttablet/tabletserver/state_manager.go
+++ b/go/vt/vttablet/tabletserver/state_manager.go
@@ -123,7 +123,6 @@ type stateManager struct {
 
 type (
 	schemaEngine interface {
-		EnsureConnectionAndDB(topodatapb.TabletType) error
 		Open() error
 		MakeNonMaster()
 		Close()
@@ -396,7 +395,7 @@ func (sm *stateManager) VerifyTarget(ctx context.Context, target *querypb.Target
 func (sm *stateManager) serveMaster() error {
 	sm.watcher.Close()
 
-	if err := sm.connect(topodatapb.TabletType_MASTER); err != nil {
+	if err := sm.connect(); err != nil {
 		return err
 	}
 
@@ -415,7 +414,7 @@ func (sm *stateManager) unserveMaster() error {
 
 	sm.watcher.Close()
 
-	if err := sm.connect(topodatapb.TabletType_MASTER); err != nil {
+	if err := sm.connect(); err != nil {
 		return err
 	}
 
@@ -429,7 +428,7 @@ func (sm *stateManager) serveNonMaster(wantTabletType topodatapb.TabletType) err
 	sm.tracker.Close()
 	sm.se.MakeNonMaster()
 
-	if err := sm.connect(wantTabletType); err != nil {
+	if err := sm.connect(); err != nil {
 		return err
 	}
 
@@ -447,7 +446,7 @@ func (sm *stateManager) unserveNonMaster(wantTabletType topodatapb.TabletType) e
 
 	sm.se.MakeNonMaster()
 
-	if err := sm.connect(wantTabletType); err != nil {
+	if err := sm.connect(); err != nil {
 		return err
 	}
 
@@ -457,8 +456,8 @@ func (sm *stateManager) unserveNonMaster(wantTabletType topodatapb.TabletType) e
 	return nil
 }
 
-func (sm *stateManager) connect(tabletType topodatapb.TabletType) error {
-	if err := sm.se.EnsureConnectionAndDB(tabletType); err != nil {
+func (sm *stateManager) connect() error {
+	if err := sm.qe.IsMySQLReachable(); err != nil {
 		return err
 	}
 	if err := sm.se.Open(); err != nil {


Test patch:
diff --git a/go/test/endtoend/tabletmanager/tablet_test.go b/go/test/endtoend/tabletmanager/tablet_test.go
index a90b6e313b..8277ee13ae 100644
--- a/go/test/endtoend/tabletmanager/tablet_test.go
+++ b/go/test/endtoend/tabletmanager/tablet_test.go
@@ -20,44 +20,12 @@ import (
 	"fmt"
 	"testing"
 
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	"vitess.io/vitess/go/test/endtoend/cluster"
 	"vitess.io/vitess/go/vt/log"
 )
 
-// TestEnsureDB tests that vttablet creates the db as needed
-func TestEnsureDB(t *testing.T) {
-	defer cluster.PanicHandler(t)
-
-	// Create new tablet
-	tablet := clusterInstance.NewVttabletInstance("replica", 0, "")
-	tablet.MysqlctlProcess = *cluster.MysqlCtlProcessInstance(tablet.TabletUID, tablet.MySQLPort, clusterInstance.TmpDirectory)
-	err := tablet.MysqlctlProcess.Start()
-	require.NoError(t, err)
-
-	log.Info(fmt.Sprintf("Started vttablet %v", tablet))
-	// Start vttablet process as replica. It won't be able to serve because there's no db.
-	err = clusterInstance.StartVttablet(tablet, "NOT_SERVING", false, cell, "dbtest", hostname, "0")
-	require.NoError(t, err)
-
-	// Make it the master.
-	err = clusterInstance.VtctlclientProcess.ExecuteCommand("TabletExternallyReparented", tablet.Alias)
-	require.NoError(t, err)
-
-	// It will still fail because the db is read-only.
-	assert.Equal(t, "NOT_SERVING", tablet.VttabletProcess.GetTabletStatus())
-	status := tablet.VttabletProcess.GetStatusDetails()
-	assert.Contains(t, status, "read-only")
-
-	// Switch to read-write and verify that that we go serving.
-	_ = clusterInstance.VtctlclientProcess.ExecuteCommand("SetReadWrite", tablet.Alias)
-	err = tablet.VttabletProcess.WaitForTabletType("SERVING")
-	require.NoError(t, err)
-	killTablets(t, tablet)
-}
-
 // TestLocalMetadata tests the contents of local_metadata table after vttablet startup
 func TestLocalMetadata(t *testing.T) {
 	defer cluster.PanicHandler(t)
diff --git a/go/vt/dbconfigs/dbconfigs_test.go b/go/vt/dbconfigs/dbconfigs_test.go
index 04acb3f9c1..57f5b695b4 100644
--- a/go/vt/dbconfigs/dbconfigs_test.go
+++ b/go/vt/dbconfigs/dbconfigs_test.go
@@ -227,9 +227,6 @@ func TestAccessors(t *testing.T) {
 	if got, want := dbc.AppWithDB().connParams.DbName, "db"; got != want {
 		t.Errorf("dbc.AppWithDB().DbName: %v, want %v", got, want)
 	}
-	if got, want := dbc.AllPrivsConnector().connParams.DbName, ""; got != want {
-		t.Errorf("dbc.AllPrivsWithDB().DbName: %v, want %v", got, want)
-	}
 	if got, want := dbc.AllPrivsWithDB().connParams.DbName, "db"; got != want {
 		t.Errorf("dbc.AllPrivsWithDB().DbName: %v, want %v", got, want)
 	}
diff --git a/go/vt/vtgate/vindexes/binary_test.go b/go/vt/vtgate/vindexes/binary_test.go
index 9ca5c57a4a..c5935d53d0 100644
--- a/go/vt/vtgate/vindexes/binary_test.go
+++ b/go/vt/vtgate/vindexes/binary_test.go
@@ -35,7 +35,7 @@ func init() {
 }
 
 func TestBinaryInfo(t *testing.T) {
-	assert.Equal(t, 1, binOnlyVindex.Cost())
+	assert.Equal(t, 0, binOnlyVindex.Cost())
 	assert.Equal(t, "binary_varchar", binOnlyVindex.String())
 	assert.True(t, binOnlyVindex.IsUnique())
 	assert.False(t, binOnlyVindex.NeedsVCursor())
diff --git a/go/vt/vtgate/vindexes/null_test.go b/go/vt/vtgate/vindexes/null_test.go
index a357aba1dd..5d12f42828 100644
--- a/go/vt/vtgate/vindexes/null_test.go
+++ b/go/vt/vtgate/vindexes/null_test.go
@@ -51,6 +51,7 @@ func TestNullMap(t *testing.T) {
 		sqltypes.NewInt64(4),
 		sqltypes.NewInt64(5),
 		sqltypes.NewInt64(6),
+		sqltypes.NewVarChar("1234567890123"),
 	})
 	require.NoError(t, err)
 	want := []key.Destination{
@@ -60,6 +61,7 @@ func TestNullMap(t *testing.T) {
 		key.DestinationKeyspaceID([]byte{0}),
 		key.DestinationKeyspaceID([]byte{0}),
 		key.DestinationKeyspaceID([]byte{0}),
+		key.DestinationKeyspaceID([]byte{0}),
 	}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("Map(): %#v, want %+v", got, want)
@@ -75,6 +77,6 @@ func TestNullVerify(t *testing.T) {
 	}
 	want := []bool{true, false}
 	if !reflect.DeepEqual(got, want) {
-		t.Errorf("binaryMD5.Verify: %v, want %v", got, want)
+		t.Errorf("null.Verify: %v, want %v", got, want)
 	}
 }
diff --git a/go/vt/vtgate/vindexes/numeric_test.go b/go/vt/vtgate/vindexes/numeric_test.go
index 57dd11b68b..a593f40f33 100644
--- a/go/vt/vtgate/vindexes/numeric_test.go
+++ b/go/vt/vtgate/vindexes/numeric_test.go
@@ -51,6 +51,7 @@ func TestNumericMap(t *testing.T) {
 		sqltypes.NewInt64(6),
 		sqltypes.NewInt64(7),
 		sqltypes.NewInt64(8),
+		sqltypes.NewInt32(8),
 	})
 	require.NoError(t, err)
 	want := []key.Destination{
@@ -63,6 +64,7 @@ func TestNumericMap(t *testing.T) {
 		key.DestinationKeyspaceID([]byte("\x00\x00\x00\x00\x00\x00\x00\x06")),
 		key.DestinationKeyspaceID([]byte("\x00\x00\x00\x00\x00\x00\x00\x07")),
 		key.DestinationKeyspaceID([]byte("\x00\x00\x00\x00\x00\x00\x00\x08")),
+		key.DestinationKeyspaceID([]byte("\x00\x00\x00\x00\x00\x00\x00\x08")),
 	}
 	if !reflect.DeepEqual(got, want) {
 		t.Errorf("Map(): %+v, want %+v", got, want)
diff --git a/go/vt/vtgate/vindexes/xxhash_test.go b/go/vt/vtgate/vindexes/xxhash_test.go
index dc66aa3d7b..049ce56d86 100644
--- a/go/vt/vtgate/vindexes/xxhash_test.go
+++ b/go/vt/vtgate/vindexes/xxhash_test.go
@@ -56,6 +56,9 @@ func TestXXHashMap(t *testing.T) {
 	}, {
 		in:  sqltypes.NewVarChar("test2"),
 		out: []byte{0x87, 0xeb, 0x11, 0x71, 0x4c, 0xa, 0xe, 0x89},
+	}, {
+		in:  sqltypes.NewVarChar("testaverylongvaluetomakesurethisworks"),
+		out: []byte{0x81, 0xd8, 0xc3, 0x8e, 0xd, 0x85, 0xe, 0x6a},
 	}, {
 		in:  sqltypes.NewInt64(1),
 		out: []byte{0xd4, 0x64, 0x5, 0x36, 0x76, 0x12, 0xb4, 0xb7},
diff --git a/go/vt/vttablet/tabletserver/state_manager_test.go b/go/vt/vttablet/tabletserver/state_manager_test.go
index 319ea21e77..438fd34775 100644
--- a/go/vt/vttablet/tabletserver/state_manager_test.go
+++ b/go/vt/vttablet/tabletserver/state_manager_test.go
@@ -79,7 +79,7 @@ func TestStateManagerServeMaster(t *testing.T) {
 	verifySubcomponent(t, 9, sm.messager, testStateOpen)
 
 	assert.False(t, sm.se.(*testSchemaEngine).nonMaster)
-	assert.True(t, sm.se.(*testSchemaEngine).ensureCalled)
+	assert.True(t, sm.qe.(*testQueryEngine).isReachable)
 	assert.False(t, sm.qe.(*testQueryEngine).stopServing)
 
 	assert.Equal(t, topodatapb.TabletType_MASTER, sm.target.TabletType)
@@ -286,7 +286,7 @@ func TestStateManagerTransitionFailRetry(t *testing.T) {
 	transitionRetryInterval = 10 * time.Millisecond
 
 	sm := newTestStateManager(t)
-	sm.se.(*testSchemaEngine).failMySQL = true
+	sm.qe.(*testQueryEngine).failMySQL = true
 
 	err := sm.SetServingType(topodatapb.TabletType_MASTER, testNow, StateServing, "")
 	require.Error(t, err)
@@ -613,19 +613,7 @@ func (tos testOrderState) State() testState {
 
 type testSchemaEngine struct {
 	testOrderState
-	ensureCalled bool
-	nonMaster    bool
-
-	failMySQL bool
-}
-
-func (te *testSchemaEngine) EnsureConnectionAndDB(tabletType topodatapb.TabletType) error {
-	if te.failMySQL {
-		te.failMySQL = false
-		return errors.New("intentional error")
-	}
-	te.ensureCalled = true
-	return nil
+	nonMaster bool
 }
 
 func (te *testSchemaEngine) Open() error {
@@ -670,6 +658,7 @@ func (te *testReplTracker) Status() (time.Duration, error) {
 
 type testQueryEngine struct {
 	testOrderState
+	isReachable bool
 	stopServing bool
 
 	failMySQL bool
@@ -686,6 +675,7 @@ func (te *testQueryEngine) IsMySQLReachable() error {
 		te.failMySQL = false
 		return errors.New("intentional error")
 	}
+	te.isReachable = true
 	return nil
 }