Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More explicit message on online DDL parsing error #8118

Merged
merged 4 commits into from
May 13, 2021

Conversation

shlomi-noach
Copy link
Contributor

@shlomi-noach shlomi-noach commented May 13, 2021

Description

When submitting an Online DDL request, and the query has a syntax error/typo, Vitess today returns a vague cannot parse statement message.

With this PR, Vitess returns the precise parsing error&position. e.g., right now Vitess does not support GENERATED (notice: fixed by #8117):

mysql> set @@ddl_strategy='online';
Query OK, 0 rows affected (0.00 sec)

mysql> CREATE TABLE if not exists t (id bigint unsigned NOT NULL AUTO_INCREMENT, ts datetime(6) DEFAULT NULL, ts_generated int GENERATED ALWAYS AS (ts) VIRTUAL, PRIMARY KEY (id)) ENGINE=InnoDB;
ERROR 1149 (42000): syntax error at position 130 near 'GENERATED'

Related Issue(s)

Fixes #8109

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

cc @mavenraven @deepthi @piki @sougou

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

make codegen strangely crashes on this branch:

$ make codegen
2021/05/13 12:46:03 saved '/home/shlomi/dev/github/planetscale/vitess-dopple/go/vt/sqlparser/ast_equals.go'
2021/05/13 12:46:03 saved '/home/shlomi/dev/github/planetscale/vitess-dopple/go/vt/sqlparser/ast_clone.go'
2021/05/13 12:46:03 saved '/home/shlomi/dev/github/planetscale/vitess-dopple/go/vt/sqlparser/ast_visit.go'
2021/05/13 12:46:03 saved '/home/shlomi/dev/github/planetscale/vitess-dopple/go/vt/sqlparser/ast_rewrite.go'
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x614764]

goroutine 1 [running]:
go/types.(*Package).Path(...)
	/usr/local/go/src/go/types/package.go:31
main.(*sizegen).getKnownType(0xc00008dc90, 0xc000100b90, 0xc000100b90)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:115 +0x84
main.(*sizegen).sizeStmtForType(0xc00008dc90, 0xc018c43660, 0x6e6b80, 0xc000100b90, 0x0, 0xc0382811d0, 0x1, 0x3)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:385 +0x4b87
main.(*sizegen).sizeImplForStruct(0xc00008dc90, 0xc01de11270, 0xc01ec1e420, 0xc012c29d01, 0x0, 0x6ec401)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:243 +0x371
main.(*sizegen).generateType(0xc00008dc90, 0xc012c779f0, 0xc038247a70, 0xc01ec054a0)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:134 +0xb7
main.(*sizegen).generateType.func1(0x6e6b80, 0xc01ec054a0)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:144 +0xa5
main.findImplementations(0xc012c779a0, 0xc01e0573b0, 0xc00008d7a0)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:168 +0x102
main.(*sizegen).generateType(0xc00008dc90, 0xc012c779f0, 0xc038247a70, 0xc01e057360)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:142 +0x2c9
main.(*sizegen).generateKnownType(0xc00008dc90, 0xc01e057360)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:160 +0x99
main.(*sizegen).finalize(0xc00008dc90, 0xc010195bd0)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:182 +0x9b
main.GenerateSizeHelpers(0xc000104470, 0x1, 0x1, 0xc00012e340, 0x3, 0x4, 0x2b, 0x40dbd8, 0x30)
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:545 +0x45a
main.main()
	/home/shlomi/dev/github/planetscale/vitess-dopple/go/tools/sizegen/sizegen.go:455 +0x230
exit status 2
make: *** [Makefile:145: sizegen] Error 1

…) to extract parsing error. Remove legacy check for alter options

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

Undid changes to the AST, there is no need to modify AST. codegen is good.

@shlomi-noach
Copy link
Contributor Author

This PR is good to go and high priority to merge.

}

if err := sqlparser.Walk(errorOnFKWalk, ddlStmt); err == ErrForeignKeyFound {
return vterrors.Errorf(vtrpcpb.Code_ABORTED, "foreign key constraint are not supported in online DDL")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
return vterrors.Errorf(vtrpcpb.Code_ABORTED, "foreign key constraint are not supported in online DDL")
return vterrors.Errorf(vtrpcpb.Code_ABORTED, "foreign key constraints are not supported in online DDL")

Also, it will be nice to link to the blog post explaining why.

Signed-off-by: deepthi <deepthi@planetscale.com>
Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have addressed my own review comment.
Now LGTM.

@deepthi deepthi merged commit 125117e into vitessio:master May 13, 2021
@deepthi deepthi deleted the online-ddl-parse-error branch May 13, 2021 21:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vitess should propagate parse error back to the caller.
3 participants