Repository: vitessio/vitess
Pull request #8433 (2021-07-08 10:06:43)

Adds flag to vttablet to disallow online DDL statements
## Description
Adds flag to vttablet to disallow online DDL statements. It is used in vtexplain.

## Checklist
- [x] Tests were added or are not required
- [x] Documentation was added or is not required


Patch:
diff --git a/go/vt/vtexplain/vtexplain_vttablet.go b/go/vt/vtexplain/vtexplain_vttablet.go
index 7702e76523..5061678807 100644
--- a/go/vt/vtexplain/vtexplain_vttablet.go
+++ b/go/vt/vtexplain/vtexplain_vttablet.go
@@ -103,6 +103,7 @@ func newTablet(opts *Options, t *topodatapb.Tablet) *explainTablet {
 		config.TwoPCAbandonAge = 1.0
 		config.TwoPCEnable = true
 	}
+	config.EnableOnlineDDL = false
 
 	// XXX much of this is cloned from the tabletserver tests
 	tsv := tabletserver.NewTabletServer(topoproto.TabletAliasString(t.Alias), config, memorytopo.NewServer(""), t.Alias)
diff --git a/go/vt/vtgate/engine/route.go b/go/vt/vtgate/engine/route.go
index 3f00ee3c92..f3f169bc47 100644
--- a/go/vt/vtgate/engine/route.go
+++ b/go/vt/vtgate/engine/route.go
@@ -210,14 +210,9 @@ func (code RouteOpcode) MarshalJSON() ([]byte, error) {
 	return json.Marshal(routeName[code])
 }
 
-// String returns a string presentation of this opcode
-func (code RouteOpcode) String() string {
-	return routeName[code]
-}
-
 // RouteType returns a description of the query routing type used by the primitive
 func (route *Route) RouteType() string {
-	return route.Opcode.String()
+	return routeName[route.Opcode]
 }
 
 // GetKeyspaceName specifies the Keyspace that this primitive routes to.
diff --git a/go/vt/vtgate/planbuilder/jointree.go b/go/vt/vtgate/planbuilder/jointree.go
index dee3a07b6d..ade239ea61 100644
--- a/go/vt/vtgate/planbuilder/jointree.go
+++ b/go/vt/vtgate/planbuilder/jointree.go
@@ -61,13 +61,6 @@ type (
 		pred  sqlparser.Expr
 	}
 
-	// cost is used to make it easy to compare the cost of two plans with each other
-	cost struct {
-		vindexCost int
-		isUnique   bool
-		opCode     engine.RouteOpcode
-	}
-
 	routePlan struct {
 		routeOpCode engine.RouteOpcode
 		solved      semantics.TableSet
@@ -83,8 +76,7 @@ type (
 		// leftJoins are the join conditions evaluated by this plan
 		leftJoins []*outerTable
 
-		// these fields are set if a vindex will be used for this route
-		currentCost      cost // currentCost tracks the cost of the chosen access method
+		// vindex and vindexValues is set if a vindex will be used for this route.
 		vindex           vindexes.Vindex
 		vindexValues     []sqltypes.PlanValue
 		vindexPredicates []sqlparser.Expr
@@ -111,17 +103,6 @@ type (
 	}
 
 	parenTables []relation
-
-	// vindexPlusPredicates is a struct used to store all the predicates that the vindex can be used to query
-	vindexPlusPredicates struct {
-		colVindex *vindexes.ColumnVindex
-		values    []sqltypes.PlanValue
-
-		// when we have the predicates found, we also know how to interact with this vindex
-		foundVindex vindexes.Vindex
-		opcode      engine.RouteOpcode
-		predicates  []sqlparser.Expr
-	}
 )
 
 // type assertions
@@ -236,6 +217,17 @@ func (rp *routePlan) cost() int {
 	return 1
 }
 
+// vindexPlusPredicates is a struct used to store all the predicates that the vindex can be used to query
+type vindexPlusPredicates struct {
+	colVindex *vindexes.ColumnVindex
+	values    []sqltypes.PlanValue
+
+	// when we have the predicates found, we also know how to interact with this vindex
+	foundVindex vindexes.Vindex
+	opcode      engine.RouteOpcode
+	predicates  []sqlparser.Expr
+}
+
 // addPredicate adds these predicates added to it. if the predicates can help,
 // they will improve the routeOpCode
 func (rp *routePlan) addPredicate(predicates ...sqlparser.Expr) error {
@@ -560,9 +552,7 @@ func (rp *routePlan) pickBestAvailableVindex() {
 			continue
 		}
 		// Choose the minimum cost vindex from the ones which are covered
-		thisCost := costFor(v.foundVindex, v.opcode)
-		if rp.vindex == nil || less(thisCost, rp.currentCost) {
-			rp.currentCost = thisCost
+		if rp.vindex == nil || v.colVindex.Vindex.Cost() < rp.vindex.Cost() {
 			rp.routeOpCode = v.opcode
 			rp.vindex = v.foundVindex
 			rp.vindexValues = v.values
@@ -643,37 +633,3 @@ func (jp *joinPlan) pushOutputColumns(columns []*sqlparser.ColName, semTable *se
 	}
 	return outputColumns
 }
-
-// costFor returns a cost struct to make route choices easier to compare
-func costFor(foundVindex vindexes.Vindex, opcode engine.RouteOpcode) cost {
-	switch opcode {
-	// For these opcodes, we should not have a vindex, so we just return the opcode as the cost
-	case engine.SelectUnsharded, engine.SelectNext, engine.SelectDBA, engine.SelectReference, engine.SelectNone, engine.SelectScatter:
-		return cost{
-			opCode: opcode,
-		}
-	}
-
-	// if we have a multiplier that is non-zero, we should have a vindex
-	if foundVindex == nil {
-		panic("expected a vindex")
-	}
-
-	return cost{
-		vindexCost: foundVindex.Cost(),
-		isUnique:   foundVindex.IsUnique(),
-		opCode:     opcode,
-	}
-}
-
-// less compares two costs and returns true if the first cost is cheaper than the second
-func less(c1, c2 cost) bool {
-	switch {
-	case c1.opCode != c2.opCode:
-		return c1.opCode < c2.opCode
-	case c1.isUnique == c2.isUnique:
-		return c1.vindexCost <= c2.vindexCost
-	default:
-		return c1.isUnique
-	}
-}
diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go
index 5d58f26265..437242c76c 100644
--- a/go/vt/vttablet/onlineddl/executor.go
+++ b/go/vt/vttablet/onlineddl/executor.go
@@ -257,7 +257,7 @@ func (e *Executor) InitDBConfig(keyspace, shard, dbName string) {
 func (e *Executor) Open() error {
 	e.initMutex.Lock()
 	defer e.initMutex.Unlock()
-	if e.isOpen {
+	if e.isOpen || !e.env.Config().EnableOnlineDDL {
 		return nil
 	}
 	e.pool.Open(e.env.Config().DB.AppWithDB(), e.env.Config().DB.DbaWithDB(), e.env.Config().DB.AppDebugWithDB())
@@ -1363,6 +1363,10 @@ func (e *Executor) terminateMigration(ctx context.Context, onlineDDL *schema.Onl
 
 // CancelMigration attempts to abort a scheduled or a running migration
 func (e *Executor) CancelMigration(ctx context.Context, uuid string, terminateRunningMigration bool, message string) (result *sqltypes.Result, err error) {
+	if !e.isOpen {
+		return nil, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "online ddl is disabled")
+	}
+
 	e.migrationMutex.Lock()
 	defer e.migrationMutex.Unlock()
 
@@ -1416,6 +1420,10 @@ func (e *Executor) cancelMigrations(ctx context.Context, uuids []string, message
 // CancelPendingMigrations cancels all pending migrations (that are expected to run or are running)
 // for this keyspace
 func (e *Executor) CancelPendingMigrations(ctx context.Context, message string) (result *sqltypes.Result, err error) {
+	if !e.isOpen {
+		return nil, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "online ddl is disabled")
+	}
+
 	uuids, err := e.readPendingMigrationsUUIDs(ctx)
 	if err != nil {
 		return result, err
@@ -2688,6 +2696,9 @@ func (e *Executor) retryMigrationWhere(ctx context.Context, whereExpr string) (r
 
 // RetryMigration marks given migration for retry
 func (e *Executor) RetryMigration(ctx context.Context, uuid string) (result *sqltypes.Result, err error) {
+	if !e.isOpen {
+		return nil, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "online ddl is disabled")
+	}
 	if !schema.IsOnlineDDLUUID(uuid) {
 		return nil, vterrors.Errorf(vtrpcpb.Code_UNKNOWN, "Not a valid migration ID in RETRY: %s", uuid)
 	}
@@ -2709,7 +2720,9 @@ func (e *Executor) SubmitMigration(
 	ctx context.Context,
 	stmt sqlparser.Statement,
 ) (result *sqltypes.Result, err error) {
-
+	if !e.isOpen {
+		return nil, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "online ddl is disabled")
+	}
 	onlineDDL, err := schema.OnlineDDLFromCommentedStatement(stmt)
 	if err != nil {
 		return nil, vterrors.Errorf(vtrpcpb.Code_INVALID_ARGUMENT, "Error submitting migration %s: %v", sqlparser.String(stmt), err)
diff --git a/go/vt/vttablet/tabletserver/tabletenv/config.go b/go/vt/vttablet/tabletserver/tabletenv/config.go
index 652a21eb0e..1bf959c4d9 100644
--- a/go/vt/vttablet/tabletserver/tabletenv/config.go
+++ b/go/vt/vttablet/tabletserver/tabletenv/config.go
@@ -161,6 +161,7 @@ func init() {
 	flag.DurationVar(&transitionGracePeriod, "serving_state_grace_period", 0, "how long to pause after broadcasting health to vtgate, before enforcing a new serving state")
 
 	flag.BoolVar(&enableReplicationReporter, "enable_replication_reporter", false, "Use polling to track replication lag.")
+	flag.BoolVar(&currentConfig.EnableOnlineDDL, "queryserver_enable_online_ddl", true, "Enable online DDL.")
 }
 
 // Init must be called after flag.Parse, and before doing any other operations.
@@ -279,6 +280,7 @@ type TabletConfig struct {
 	TransactionLimitConfig `json:"-"`
 
 	EnforceStrictTransTables bool `json:"-"`
+	EnableOnlineDDL          bool `json:"-"`
 }
 
 // ConnPoolConfig contains the config for a conn pool.
@@ -478,6 +480,7 @@ var defaultConfig = TabletConfig{
 	TransactionLimitConfig: defaultTransactionLimitConfig(),
 
 	EnforceStrictTransTables: true,
+	EnableOnlineDDL:          true,
 }
 
 // defaultTxThrottlerConfig formats the default throttlerdata.Configuration


Test patch:
diff --git a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt
index 4ae8b355b6..c7642bf536 100644
--- a/go/vt/vtgate/planbuilder/testdata/filter_cases.txt
+++ b/go/vt/vtgate/planbuilder/testdata/filter_cases.txt
@@ -985,7 +985,6 @@ Gen4 plan same as above
     "Vindex": "name_user_map"
   }
 }
-Gen4 plan same as above
 
 # Route with multiple route constraints, SelectEqual is the best constraint.
 "select id from user where user.col = false and user.id in (1, 2) and user.name = 'aa'"
@@ -1008,7 +1007,6 @@ Gen4 plan same as above
     "Vindex": "name_user_map"
   }
 }
-Gen4 plan same as above
 
 # Route with multiple route constraints, SelectEqualUnique is the best constraint.
 "select id from user where user.col = 5 and user.id in (1, 2) and user.name = 'aa' and user.id = 1"
diff --git a/go/vt/vttablet/tabletserver/query_executor_test.go b/go/vt/vttablet/tabletserver/query_executor_test.go
index 8683d63b27..bf064b9b25 100644
--- a/go/vt/vttablet/tabletserver/query_executor_test.go
+++ b/go/vt/vttablet/tabletserver/query_executor_test.go
@@ -389,6 +389,30 @@ func TestQueryExecutorSelectImpossible(t *testing.T) {
 	}
 }
 
+// TestDisableOnlineDDL checks whether disabling online DDLs throws the correct error or not
+func TestDisableOnlineDDL(t *testing.T) {
+	db := setUpQueryExecutorTest(t)
+	defer db.Close()
+	query := "ALTER VITESS_MIGRATION CANCEL ALL"
+
+	db.AddQueryPattern(".*", &sqltypes.Result{})
+
+	ctx := context.Background()
+	tsv := newTestTabletServer(ctx, noFlags, db)
+
+	qre := newTestQueryExecutor(ctx, tsv, query, 0)
+	_, err := qre.Execute()
+	require.NoError(t, err)
+	tsv.StopService()
+
+	tsv = newTestTabletServer(ctx, disableOnlineDDL, db)
+	defer tsv.StopService()
+
+	qre = newTestQueryExecutor(ctx, tsv, query, 0)
+	_, err = qre.Execute()
+	require.EqualError(t, err, "online ddl is disabled")
+}
+
 func TestQueryExecutorLimitFailure(t *testing.T) {
 	type dbResponse struct {
 		query  string
@@ -1117,6 +1141,7 @@ const (
 	noTwopc
 	shortTwopcAge
 	smallResultSize
+	disableOnlineDDL
 )
 
 // newTestQueryExecutor uses a package level variable testTabletServer defined in tabletserver_test.go
@@ -1138,6 +1163,11 @@ func newTestTabletServer(ctx context.Context, flags executorFlags, db *fakesqldb
 	} else {
 		config.TwoPCEnable = true
 	}
+	if flags&disableOnlineDDL > 0 {
+		config.EnableOnlineDDL = false
+	} else {
+		config.EnableOnlineDDL = true
+	}
 	config.TwoPCCoordinatorAddress = "fake"
 	if flags&shortTwopcAge > 0 {
 		config.TwoPCAbandonAge = 0.5
diff --git a/go/vt/vttablet/tabletserver/tabletenv/config_test.go b/go/vt/vttablet/tabletserver/tabletenv/config_test.go
index 71e4ad7f07..0a6d1da293 100644
--- a/go/vt/vttablet/tabletserver/tabletenv/config_test.go
+++ b/go/vt/vttablet/tabletserver/tabletenv/config_test.go
@@ -214,6 +214,7 @@ func TestFlags(t *testing.T) {
 			TransactionLimitByPrincipal: true,
 		},
 		EnforceStrictTransTables: true,
+		EnableOnlineDDL:          true,
 		DB:                       &dbconfigs.DBConfigs{},
 	}
 	assert.Equal(t, want.DB, currentConfig.DB)