From d9836a82fa3f9f098ece032e0cc1e0493ce7516b Mon Sep 17 00:00:00 2001 From: Andres Taylor Date: Mon, 11 Mar 2024 17:01:57 +0100 Subject: [PATCH] feat: make sure to not expand expressions for create/alter view Signed-off-by: Andres Taylor --- .../endtoend/vtgate/queries/misc/misc_test.go | 2 +- go/vt/vtgate/planbuilder/ddl.go | 92 +++++++++---------- .../planbuilder/testdata/ddl_cases.json | 2 +- .../ddl_cases_no_default_keyspace.json | 8 +- .../planbuilder/testdata/view_cases.json | 4 +- go/vt/vttablet/onlineddl/executor.go | 1 + 6 files changed, 50 insertions(+), 59 deletions(-) diff --git a/go/test/endtoend/vtgate/queries/misc/misc_test.go b/go/test/endtoend/vtgate/queries/misc/misc_test.go index 0d95bcfa1ac..d1562b3341d 100644 --- a/go/test/endtoend/vtgate/queries/misc/misc_test.go +++ b/go/test/endtoend/vtgate/queries/misc/misc_test.go @@ -395,7 +395,7 @@ func TestAlterTableWithView(t *testing.T) { mcmp.Exec(`insert into t1(id1, id2) values (1, 1)`) mcmp.AssertMatches("select * from v1", `[[INT64(1) INT64(1)]]`) - // alter table + // alter table add column mcmp.Exec(`alter table t1 add column test bigint`) mcmp.Exec(`alter view v1 as select * from t1`) diff --git a/go/vt/vtgate/planbuilder/ddl.go b/go/vt/vtgate/planbuilder/ddl.go index fe5ebeb0889..8d2791b0ad0 100644 --- a/go/vt/vtgate/planbuilder/ddl.go +++ b/go/vt/vtgate/planbuilder/ddl.go @@ -192,57 +192,48 @@ func findTableDestinationAndKeyspace(vschema plancontext.VSchema, ddlStatement s return destination, keyspace, nil } -func buildAlterView(ctx context.Context, vschema plancontext.VSchema, ddl *sqlparser.AlterView, reservedVars *sqlparser.ReservedVars, enableOnlineDDL, enableDirectDDL bool) (key.Destination, *vindexes.Keyspace, error) { - // For Alter View, we require that the view exist and the select query can be satisfied within the keyspace itself - // We should remove the keyspace name from the table name, as the database name in MySQL might be different than the keyspace name - destination, keyspace, err := findTableDestinationAndKeyspace(vschema, ddl) - if err != nil { - return nil, nil, err - } +func buildAlterView( + ctx context.Context, + vschema plancontext.VSchema, + ddl *sqlparser.AlterView, + reservedVars *sqlparser.ReservedVars, + enableOnlineDDL, enableDirectDDL bool, +) (key.Destination, *vindexes.Keyspace, error) { + return buildCreateViewCommon(ctx, vschema, reservedVars, enableOnlineDDL, enableDirectDDL, ddl.Select, ddl) +} - selectPlan, err := createInstructionFor(ctx, sqlparser.String(ddl.Select), ddl.Select, reservedVars, vschema, enableOnlineDDL, enableDirectDDL) - if err != nil { - return nil, nil, err - } - selPlanKs := selectPlan.primitive.GetKeyspaceName() - if keyspace.Name != selPlanKs { - return nil, nil, vterrors.VT12001(ViewDifferentKeyspace) - } - if vschema.IsViewsEnabled() { - if keyspace == nil { - return nil, nil, vterrors.VT09005() - } - return destination, keyspace, nil - } - isRoutePlan, opCode := tryToGetRoutePlan(selectPlan.primitive) - if !isRoutePlan { - return nil, nil, vterrors.VT12001(ViewComplex) - } - if opCode != engine.Unsharded && opCode != engine.EqualUnique && opCode != engine.Scatter { - return nil, nil, vterrors.VT12001(ViewComplex) - } - _ = sqlparser.SafeRewrite(ddl.Select, nil, func(cursor *sqlparser.Cursor) bool { - switch tableName := cursor.Node().(type) { - case sqlparser.TableName: - cursor.Replace(sqlparser.TableName{ - Name: tableName.Name, - }) - } - return true - }) - return destination, keyspace, nil +func buildCreateView( + ctx context.Context, + vschema plancontext.VSchema, + ddl *sqlparser.CreateView, + reservedVars *sqlparser.ReservedVars, + enableOnlineDDL, enableDirectDDL bool, +) (key.Destination, *vindexes.Keyspace, error) { + return buildCreateViewCommon(ctx, vschema, reservedVars, enableOnlineDDL, enableDirectDDL, ddl.Select, ddl) } -func buildCreateView(ctx context.Context, vschema plancontext.VSchema, ddl *sqlparser.CreateView, reservedVars *sqlparser.ReservedVars, enableOnlineDDL, enableDirectDDL bool) (key.Destination, *vindexes.Keyspace, error) { +func buildCreateViewCommon( + ctx context.Context, + vschema plancontext.VSchema, + reservedVars *sqlparser.ReservedVars, + enableOnlineDDL, enableDirectDDL bool, + ddlSelect sqlparser.SelectStatement, + ddl sqlparser.DDLStatement, +) (key.Destination, *vindexes.Keyspace, error) { // For Create View, we require that the keyspace exist and the select query can be satisfied within the keyspace itself // We should remove the keyspace name from the table name, as the database name in MySQL might be different than the keyspace name - destination, keyspace, _, err := vschema.TargetDestination(ddl.ViewName.Qualifier.String()) + destination, keyspace, err := findTableDestinationAndKeyspace(vschema, ddl) if err != nil { return nil, nil, err } - ddl.ViewName.Qualifier = sqlparser.NewIdentifierCS("") - selectPlan, err := createInstructionFor(ctx, sqlparser.String(ddl.Select), ddl.Select, reservedVars, vschema, enableOnlineDDL, enableDirectDDL) + // because we don't trust the schema tracker to have up-to-date info, we don't want to expand any SELECT * here + var expressions []sqlparser.SelectExprs + _ = sqlparser.VisitAllSelects(ddlSelect, func(p *sqlparser.Select, idx int) error { + expressions = append(expressions, sqlparser.CloneSelectExprs(p.SelectExprs)) + return nil + }) + selectPlan, err := createInstructionFor(ctx, sqlparser.String(ddlSelect), ddlSelect, reservedVars, vschema, enableOnlineDDL, enableDirectDDL) if err != nil { return nil, nil, err } @@ -250,6 +241,14 @@ func buildCreateView(ctx context.Context, vschema plancontext.VSchema, ddl *sqlp if keyspace.Name != selPlanKs { return nil, nil, vterrors.VT12001(ViewDifferentKeyspace) } + + _ = sqlparser.VisitAllSelects(ddlSelect, func(p *sqlparser.Select, idx int) error { + p.SelectExprs = expressions[idx] + return nil + }) + + sqlparser.RemoveKeyspace(ddl) + if vschema.IsViewsEnabled() { if keyspace == nil { return nil, nil, vterrors.VT09005() @@ -263,15 +262,6 @@ func buildCreateView(ctx context.Context, vschema plancontext.VSchema, ddl *sqlp if opCode != engine.Unsharded && opCode != engine.EqualUnique && opCode != engine.Scatter { return nil, nil, vterrors.VT12001(ViewComplex) } - _ = sqlparser.SafeRewrite(ddl.Select, nil, func(cursor *sqlparser.Cursor) bool { - switch tableName := cursor.Node().(type) { - case sqlparser.TableName: - cursor.Replace(sqlparser.TableName{ - Name: tableName.Name, - }) - } - return true - }) return destination, keyspace, nil } diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases.json b/go/vt/vtgate/planbuilder/testdata/ddl_cases.json index 41aded18c5d..307a1ac64be 100644 --- a/go/vt/vtgate/planbuilder/testdata/ddl_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases.json @@ -374,7 +374,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view tmp_view as select user_id, col1, col2 from authoritative" + "Query": "create view tmp_view as select * from authoritative" }, "TablesUsed": [ "user.tmp_view" diff --git a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.json b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.json index 343b8d78b14..b62988b2e38 100644 --- a/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.json +++ b/go/vt/vtgate/planbuilder/testdata/ddl_cases_no_default_keyspace.json @@ -125,7 +125,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view view_a as select user_id, col1, col2 from authoritative" + "Query": "create view view_a as select * from authoritative" }, "TablesUsed": [ "user.view_a" @@ -144,7 +144,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view view_a as select a.user_id, a.col1, a.col2, b.user_id, b.col1, b.col2 from authoritative as a join authoritative as b on a.user_id = b.user_id" + "Query": "create view view_a as select * from authoritative as a join authoritative as b on a.user_id = b.user_id" }, "TablesUsed": [ "user.view_a" @@ -163,7 +163,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view view_a as select user_id, col1, col2 from authoritative as a" + "Query": "create view view_a as select a.* from authoritative as a" }, "TablesUsed": [ "user.view_a" @@ -201,7 +201,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view view_a as select `user`.id, a.user_id, a.col1, a.col2, `user`.col1 from authoritative as a join `user` on a.user_id = `user`.id" + "Query": "create view view_a as select `user`.id, a.*, `user`.col1 from authoritative as a join `user` on a.user_id = `user`.id" }, "TablesUsed": [ "user.view_a" diff --git a/go/vt/vtgate/planbuilder/testdata/view_cases.json b/go/vt/vtgate/planbuilder/testdata/view_cases.json index decc6a117cf..5d5dcd81a33 100644 --- a/go/vt/vtgate/planbuilder/testdata/view_cases.json +++ b/go/vt/vtgate/planbuilder/testdata/view_cases.json @@ -11,7 +11,7 @@ "Name": "user", "Sharded": true }, - "Query": "alter view user_extra as select * from `user`.`user`" + "Query": "alter view user_extra as select * from `user`" }, "TablesUsed": [ "user.user_extra" @@ -35,7 +35,7 @@ "Name": "user", "Sharded": true }, - "Query": "create view view_ac as select user_id, col1, col2 from authoritative" + "Query": "create view view_ac as select * from authoritative" }, "TablesUsed": [ "user.view_ac" diff --git a/go/vt/vttablet/onlineddl/executor.go b/go/vt/vttablet/onlineddl/executor.go index 19f5c82ca5a..d4dfd5d686e 100644 --- a/go/vt/vttablet/onlineddl/executor.go +++ b/go/vt/vttablet/onlineddl/executor.go @@ -3069,6 +3069,7 @@ func (e *Executor) executeAlterViewOnline(ctx context.Context, onlineDDL *schema // it actually easier for us to issue a CREATE OR REPLACE, because it // actually creates a view... stmt = &sqlparser.CreateView{ + Algorithm: viewStmt.Algorithm, Definer: viewStmt.Definer, Security: viewStmt.Security,