Query executor: preparing for `SHOW VITESS_MIGRATIONS` via `ShowBasic` #12688

shlomi-noach · 2023-03-22T08:28:38Z

Description

Today, a SHOW VITESS_MIGRATIONS command is parsed in vtagte as ShowBasic, but converted to a SELECT... before being sent to vttablet:

vitess/go/vt/vtgate/planbuilder/show.go

Lines 276 to 290 in 7af519e

    
           sql := sqlparser.BuildParsedQuery("SELECT * FROM %s.schema_migrations", sidecarDBID).Query 
        
           if show.Filter != nil { 
        
           	if show.Filter.Filter != nil { 
        
           		sql += fmt.Sprintf(" where %s", sqlparser.String(show.Filter.Filter)) 
        
           	} else if show.Filter.Like != "" { 
        
           		lit := sqlparser.String(sqlparser.NewStrLiteral(show.Filter.Like)) 
        
           		sql += fmt.Sprintf(" where migration_uuid LIKE %s OR migration_context LIKE %s OR migration_status LIKE %s", lit, lit, lit) 
        
           	} 
        
           } 
        
           return &engine.Send{ 
        
           	Keyspace:          ks, 
        
           	TargetDestination: dest, 
        
           	Query:             sql, 
        
           }, nil

We want to send the original ShowBasic, instead. However, for compatibility reasons we must first ensure vttablet can intercept SHOW VITESS_MIGRATIONS via ShowBasic.

In this PR query executor intercepts a ShowBasic and identifies if it has a VitessMigrations command. It then generates a PlanShowMigrations. With this plan, the executor calls onlineddl.Executor's ShowMigrations() function, which then generates the SQL query, same way that this is done today in vtgate.

This code path it not being used now. This will be included in v17. And in v18 we will re-implement vtgate's behavior as explained above.

@harshit-gangal please note I chose to create a PlanShowMigrations as opposed to generate the SQL directly in query executor.

Related Issue(s)

Replaces #12676

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Deployment Notes

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…es a ShowMigrationsPlan based on ShowBasic, in preparation for ShowBasic statement coming up in next releases Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2023-03-22T08:30:13Z

go/vt/vttablet/tabletserver/planbuilder/builder.go

-		if showInternal.Command == sqlparser.Table {
+		switch showInternal.Command {
+		case sqlparser.VitessMigrations:
+			return &Plan{PlanID: PlanShowMigrations, FullStmt: show}, nil


i chose to return a PlanShowMigrations rather than returning a PlanSelect and generating SQL here.

The benefit of doing it at plan time is that it is cached. So, at execution, it does not need to create the select query and parse it.
I will leave this to you as it is not a critical query on execution.

Caching is a good point. I feel like it's wrong to construct this query in query_executor, though, and that it makes more sense in onlineddl.Executor. We can always change that in the future.

go/vt/vttablet/onlineddl/executor.go

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

go/vt/vttablet/onlineddl/executor.go

mattlord

We need to handle the variable sidecar database name. Otherwise looks good (two very nitty comments).

mattlord · 2023-03-30T22:05:02Z

go/vt/vttablet/onlineddl/executor.go

+// ShowMigrations shows migrations, optionally filtered by a condition
+func (e *Executor) ShowMigrations(ctx context.Context, show *sqlparser.Show) (result *sqltypes.Result, err error) {
+	if atomic.LoadInt64(&e.isOpen) == 0 {
+		return nil, vterrors.New(vtrpcpb.Code_FAILED_PRECONDITION, "online ddl is disabled")


Nit, but IMO OnlineDDL or online DDL is better. I feel like perhaps the former so as to identify that it's really the Vitess OnlineDDL functionality that's disabled (MySQL itself supports online DDL, and this could be confusing for MySQL users).

mattlord · 2023-03-30T22:06:54Z

go/vt/vttablet/onlineddl/executor.go

+		return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] ShowMigrations expects a ShowBasic statement. Got: %s", sqlparser.String(show))
+	}
+	if showBasic.Command != sqlparser.VitessMigrations {
+		return nil, vterrors.Errorf(vtrpcpb.Code_INTERNAL, "[BUG] ShowMigrations expects a VitessMigrations command, got %v. Statement: %s", showBasic.Command, sqlparser.String(show))


Might be worth %+v for the struct.

go/vt/vttablet/onlineddl/schema.go

mattlord

I only had two very minor comments. LGTM!

Sorry about the temporary confusion around the sidecardb. That was my fault.

go/vt/vttablet/onlineddl/schema.go

go/vt/vttablet/onlineddl/executor.go

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added 5 commits March 21, 2023 11:00

formalize ShowMigrations command

6c09953

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vtgate supports both new and old execution methods for ShowMigrations

ec5e871

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

support ShowMigrations in query executor and OnlineDDL executor

a654711

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Merge branch 'main' into formalize-show-vitess-migrations

bf8c073

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

Simplified: vtgate and ast restored from main; query executor generat…

703c532

…es a ShowMigrationsPlan based on ShowBasic, in preparation for ShowBasic statement coming up in next releases Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach added Type: Internal Cleanup Component: Query Serving Component: Online DDL Online DDL (vitess/native/gh-ost/pt-osc) labels Mar 22, 2023

shlomi-noach requested review from harshit-gangal, systay, rohit-nayak-ps and mattlord as code owners March 22, 2023 08:28

shlomi-noach commented Mar 22, 2023

View reviewed changes

harshit-gangal reviewed Mar 22, 2023

View reviewed changes

go/vt/vttablet/onlineddl/executor.go Outdated Show resolved Hide resolved

fix value

c4be8e2

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal reviewed Mar 22, 2023

View reviewed changes

go/vt/vttablet/onlineddl/executor.go Show resolved Hide resolved

harshit-gangal approved these changes Mar 23, 2023

View reviewed changes

harshit-gangal removed request for systay and rohit-nayak-ps March 23, 2023 05:56

shlomi-noach requested a review from a team March 28, 2023 07:08

mattlord reviewed Mar 30, 2023

View reviewed changes

mattlord approved these changes Mar 31, 2023

View reviewed changes

go/vt/vttablet/onlineddl/schema.go Show resolved Hide resolved

go/vt/vttablet/onlineddl/executor.go Show resolved Hide resolved

reuse schema.ErrOnlineDDLDisabled

d0c46ac

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

harshit-gangal merged commit 746c37f into vitessio:main Apr 3, 2023

harshit-gangal deleted the show-migrations-prepare-tablet branch April 3, 2023 05:07

harshit-gangal mentioned this pull request Aug 3, 2023

passdown show vitess migration query to vttablet #13706

Closed

shlomi-noach mentioned this pull request Aug 6, 2023

Vtgate: pass 'SHOW VITESS_MIGRATIONS' to tablet's query executor #13726

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Query executor: preparing for `SHOW VITESS_MIGRATIONS` via `ShowBasic` #12688

Query executor: preparing for `SHOW VITESS_MIGRATIONS` via `ShowBasic` #12688

shlomi-noach commented Mar 22, 2023 •

edited

Loading

shlomi-noach Mar 22, 2023

harshit-gangal Mar 22, 2023 •

edited

Loading

shlomi-noach Mar 23, 2023

mattlord left a comment

mattlord Mar 30, 2023

shlomi-noach Apr 2, 2023

mattlord Mar 30, 2023

shlomi-noach Apr 2, 2023

mattlord left a comment

	sql := sqlparser.BuildParsedQuery("SELECT * FROM %s.schema_migrations", sidecarDBID).Query

	if show.Filter != nil {
	if show.Filter.Filter != nil {
	sql += fmt.Sprintf(" where %s", sqlparser.String(show.Filter.Filter))
	} else if show.Filter.Like != "" {
	lit := sqlparser.String(sqlparser.NewStrLiteral(show.Filter.Like))
	sql += fmt.Sprintf(" where migration_uuid LIKE %s OR migration_context LIKE %s OR migration_status LIKE %s", lit, lit, lit)
	}
	}
	return &engine.Send{
	Keyspace: ks,
	TargetDestination: dest,
	Query: sql,
	}, nil

Query executor: preparing for SHOW VITESS_MIGRATIONS via ShowBasic #12688

Query executor: preparing for SHOW VITESS_MIGRATIONS via ShowBasic #12688

Conversation

shlomi-noach commented Mar 22, 2023 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

shlomi-noach Mar 22, 2023

Choose a reason for hiding this comment

harshit-gangal Mar 22, 2023 • edited Loading

Choose a reason for hiding this comment

shlomi-noach Mar 23, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

mattlord Mar 30, 2023

Choose a reason for hiding this comment

shlomi-noach Apr 2, 2023

Choose a reason for hiding this comment

mattlord Mar 30, 2023

Choose a reason for hiding this comment

shlomi-noach Apr 2, 2023

Choose a reason for hiding this comment

mattlord left a comment

Choose a reason for hiding this comment

Query executor: preparing for `SHOW VITESS_MIGRATIONS` via `ShowBasic` #12688

Query executor: preparing for `SHOW VITESS_MIGRATIONS` via `ShowBasic` #12688

shlomi-noach commented Mar 22, 2023 •

edited

Loading

harshit-gangal Mar 22, 2023 •

edited

Loading