vttablet: new flag: enable_strict_trans_tables.

Many users are running MySQL with this flag turned off, and it's
non-trivial to turn it on due to legacy reasons. This flag allows
you to keep going. The associated risk of using this flag is in
the description of the command line argument.

Author: sougou

go/vt/vtgate/executor_dml_test.go

@@ -787,11 +787,14 @@ func TestInsertFail(t *testing.T) {
 	}
 }
 
-func TestInsertPartialFail(t *testing.T) {
-	executor, sbc1, _, sbclookup := createExecutorEnv()
+// If a statement gets broken up into two, and the first one fails,
+// then an error should be returned normally.
+func TestInsertPartialFail1(t *testing.T) {
+	executor, _, _, sbclookup := createExecutorEnv()
 
-	// If the first DML fails, there should be no rollback.
+	// Make the first DML fail, there should be no rollback.
 	sbclookup.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
+
 	_, err := executor.Execute(
 		context.Background(),
 		&vtgatepb.Session{InTransaction: true},
@@ -802,16 +805,24 @@ func TestInsertPartialFail(t *testing.T) {
 	if err == nil || !strings.HasPrefix(err.Error(), want) {
 		t.Errorf("insert first DML fail: %v, must start with %s", err, want)
 	}
+}
 
-	// If the second DML fails, we should rollback.
+// If a statement gets broken up into two, and the second one fails
+// after successful execution of the first, then the transaction must
+// be rolled back due to partial execution.
+func TestInsertPartialFail2(t *testing.T) {
+	executor, sbc1, _, _ := createExecutorEnv()
+
+	// Make the second DML fail, it should result in a rollback.
 	sbc1.MustFailCodes[vtrpcpb.Code_INVALID_ARGUMENT] = 1
-	_, err = executor.Execute(
+
+	_, err := executor.Execute(
 		context.Background(),
 		&vtgatepb.Session{InTransaction: true},
 		"insert into user(id, v, name) values (1, 2, 'myname')",
 		nil,
 	)
-	want = "transaction rolled back"
+	want := "transaction rolled back"
 	if err == nil || !strings.HasPrefix(err.Error(), want) {
 		t.Errorf("insert first DML fail: %v, must start with %s", err, want)
 	}

go/vt/vttablet/tabletserver/connpool/dbconn.go

@@ -177,18 +177,20 @@ var (
 // VerifyMode is a helper method to verify mysql is running with
 // sql_mode = STRICT_TRANS_TABLES and autocommit=ON. It also returns
 // the current binlog format.
-func (dbc *DBConn) VerifyMode() (BinlogFormat, error) {
-	qr, err := dbc.conn.ExecuteFetch(getModeSQL, 2, false)
-	if err != nil {
-		return 0, fmt.Errorf("could not verify mode: %v", err)
-	}
-	if len(qr.Rows) != 1 {
-		return 0, fmt.Errorf("incorrect rowcount received for %s: %d", getModeSQL, len(qr.Rows))
-	}
-	if !strings.Contains(qr.Rows[0][0].String(), "STRICT_TRANS_TABLES") {
-		return 0, fmt.Errorf("require sql_mode to be STRICT_TRANS_TABLES: got %s", qr.Rows[0][0].String())
+func (dbc *DBConn) VerifyMode(strictTransTables bool) (BinlogFormat, error) {
+	if strictTransTables {
+		qr, err := dbc.conn.ExecuteFetch(getModeSQL, 2, false)
+		if err != nil {
+			return 0, fmt.Errorf("could not verify mode: %v", err)
+		}
+		if len(qr.Rows) != 1 {
+			return 0, fmt.Errorf("incorrect rowcount received for %s: %d", getModeSQL, len(qr.Rows))
+		}
+		if !strings.Contains(qr.Rows[0][0].String(), "STRICT_TRANS_TABLES") {
+			return 0, fmt.Errorf("require sql_mode to be STRICT_TRANS_TABLES: got '%s'", qr.Rows[0][0].String())
+		}
 	}
-	qr, err = dbc.conn.ExecuteFetch(getAutocommit, 2, false)
+	qr, err := dbc.conn.ExecuteFetch(getAutocommit, 2, false)
 	if err != nil {
 		return 0, fmt.Errorf("could not verify mode: %v", err)
 	}

go/vt/vttablet/tabletserver/query_engine.go

@@ -132,6 +132,8 @@ type QueryEngine struct {
 	// TODO(sougou) There are two acl packages. Need to rename.
 	exemptACL tacl.ACL
 
+	strictTransTables bool
+
 	// Loggers
 	accessCheckerLogger *logutil.ThrottledLogger
 }
@@ -173,6 +175,8 @@ func NewQueryEngine(checker connpool.MySQLChecker, se *schema.Engine, config tab
 	qe.strictTableACL = config.StrictTableACL
 	qe.enableTableACLDryRun = config.EnableTableACLDryRun
 
+	qe.strictTransTables = config.EnforceStrictTransTables
+
 	if config.TableACLExemptACL != "" {
 		if f, err := tableacl.GetCurrentAclFactory(); err == nil {
 			if exemptACL, err := f.New([]string{config.TableACLExemptACL}); err == nil {
@@ -235,7 +239,7 @@ func (qe *QueryEngine) Open(dbconfigs dbconfigs.DBConfigs) error {
 		qe.conns.Close()
 		return err
 	}
-	qe.binlogFormat, err = conn.VerifyMode()
+	qe.binlogFormat, err = conn.VerifyMode(qe.strictTransTables)
 	conn.Recycle()
 
 	if err != nil {

go/vt/vttablet/tabletserver/query_engine_test.go

@@ -15,11 +15,56 @@ import (
 
 	"github.com/youtube/vitess/go/mysqlconn/fakesqldb"
 	"github.com/youtube/vitess/go/sqltypes"
+	querypb "github.com/youtube/vitess/go/vt/proto/query"
 	"github.com/youtube/vitess/go/vt/vttablet/tabletserver/schema"
 	"github.com/youtube/vitess/go/vt/vttablet/tabletserver/schema/schematest"
 	"github.com/youtube/vitess/go/vt/vttablet/tabletserver/tabletenv"
 )
 
+func TestStrictTransTables(t *testing.T) {
+	db := fakesqldb.New(t)
+	defer db.Close()
+	for query, result := range schematest.Queries() {
+		db.AddQuery(query, result)
+	}
+	testUtils := newTestUtils()
+	dbconfigs := testUtils.newDBConfigs(db)
+
+	// Test default behavior.
+	config := tabletenv.DefaultQsConfig
+	// config.EnforceStrictTransTable is true by default.
+	qe := NewQueryEngine(DummyChecker, schema.NewEngine(DummyChecker, config), config)
+	qe.se.Open(db.ConnParams())
+	if err := qe.Open(dbconfigs); err != nil {
+		t.Error(err)
+	}
+	qe.Close()
+
+	// Check that we fail if STRICT_TRANS_TABLES is not set.
+	db.AddQuery(
+		"select @@global.sql_mode",
+		&sqltypes.Result{
+			Fields: []*querypb.Field{{Type: sqltypes.VarChar}},
+			Rows:   [][]sqltypes.Value{{sqltypes.MakeString([]byte(""))}},
+		},
+	)
+	qe = NewQueryEngine(DummyChecker, schema.NewEngine(DummyChecker, config), config)
+	err := qe.Open(dbconfigs)
+	wantErr := "require sql_mode to be STRICT_TRANS_TABLES: got ''"
+	if err == nil || err.Error() != wantErr {
+		t.Errorf("Open: %v, want %s", err, wantErr)
+	}
+	qe.Close()
+
+	// Test that we succeed if the enforcement flag is off.
+	config.EnforceStrictTransTables = false
+	qe = NewQueryEngine(DummyChecker, schema.NewEngine(DummyChecker, config), config)
+	if err := qe.Open(dbconfigs); err != nil {
+		t.Error(err)
+	}
+	qe.Close()
+}
+
 func TestGetPlanPanicDuetoEmptyQuery(t *testing.T) {
 	db := fakesqldb.New(t)
 	defer db.Close()

go/vt/vttablet/tabletserver/tabletenv/config.go

@@ -54,7 +54,7 @@ func init() {
 	flag.BoolVar(&Config.TerseErrors, "queryserver-config-terse-errors", DefaultQsConfig.TerseErrors, "prevent bind vars from escaping in returned errors")
 	flag.StringVar(&Config.PoolNamePrefix, "pool-name-prefix", DefaultQsConfig.PoolNamePrefix, "pool name prefix, vttablet has several pools and each of them has a name. This config specifies the prefix of these pool names")
 	flag.BoolVar(&Config.WatchReplication, "watch_replication_stream", false, "When enabled, vttablet will stream the MySQL replication stream from the local server, and use it to support the include_event_token ExecuteOptions.")
-	flag.BoolVar(&Config.EnableAutoCommit, "enable-autocommit", DefaultQsConfig.EnableAutoCommit, "if the flag is on, a DML outsides a transaction will be auto committed.")
+	flag.BoolVar(&Config.EnableAutoCommit, "enable-autocommit", DefaultQsConfig.EnableAutoCommit, "if the flag is on, a DML outsides a transaction will be auto committed. This flag is deprecated and is unsafe. Instead, use the VTGate provided autocommit feature.")
 	flag.BoolVar(&Config.TwoPCEnable, "twopc_enable", DefaultQsConfig.TwoPCEnable, "if the flag is on, 2pc is enabled. Other 2pc flags must be supplied.")
 	flag.StringVar(&Config.TwoPCCoordinatorAddress, "twopc_coordinator_address", DefaultQsConfig.TwoPCCoordinatorAddress, "address of the (VTGate) process(es) that will be used to notify of abandoned transactions.")
 	flag.Float64Var(&Config.TwoPCAbandonAge, "twopc_abandon_age", DefaultQsConfig.TwoPCAbandonAge, "time in seconds. Any unresolved transaction older than this time will be sent to the coordinator to be resolved.")
@@ -69,6 +69,8 @@ func init() {
 
 	flag.BoolVar(&Config.HeartbeatEnable, "heartbeat_enable", DefaultQsConfig.HeartbeatEnable, "If true, vttablet records (if master) or checks (if replica) the current time of a replication heartbeat in the table _vt.heartbeat. The result is used to inform the serving state of the vttablet via healthchecks.")
 	flag.DurationVar(&Config.HeartbeatInterval, "heartbeat_interval", DefaultQsConfig.HeartbeatInterval, "How frequently to read and write replication heartbeat.")
+
+	flag.BoolVar(&Config.EnforceStrictTransTables, "enforce_strict_trans_tables", DefaultQsConfig.EnforceStrictTransTables, "If true, vttablet requires MySQL to run with STRICT_TRANS_TABLES on. It is recommended to not turn this flag off. Otherwise MySQL may alter your supplied values before saving them to the database.")
 }
 
 // Init must be called after flag.Parse, and before doing any other operations.
@@ -115,6 +117,8 @@ type TabletConfig struct {
 
 	HeartbeatEnable   bool
 	HeartbeatInterval time.Duration
+
+	EnforceStrictTransTables bool
 }
 
 // DefaultQsConfig is the default value for the query service config.
@@ -162,6 +166,8 @@ var DefaultQsConfig = TabletConfig{
 
 	HeartbeatEnable:   false,
 	HeartbeatInterval: 1 * time.Second,
+
+	EnforceStrictTransTables: true,
 }
 
 // defaultTxThrottlerConfig formats the default throttlerdata.Configuration