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

sql: prepare support for INSERT, UPDATE, DELETE #4037

Merged
merged 1 commit into from
Jan 30, 2016
Merged

sql: prepare support for INSERT, UPDATE, DELETE #4037

merged 1 commit into from
Jan 30, 2016

Conversation

maddyblue
Copy link
Contributor

Make sure prepare statements are run in a transaction.

Use a new boolean flag on the planner to indicate prepare. Statement
implementations will gain knowledge of work to omit during prepare. An
alternative implementation would split up the prepare and exec portions of
the work. However, this would require passing around significant amounts
of state and I think the best implementation is the one here.

Keep a whitelist of statement types that support preparation instead of
attempting to exec them during prepare.

This also functions as a refactor for prepare statements in
general. Previously, we called parser.InferArgs, which walked the statement
and ran TypeCheck on all expressions. It turns out this is not needed,
since Select (and others) do this themselves. Thus, we can invoke those
worker functions directly but teach them about prepareOnly so they can
omit work once the val args have been populated and the type checks done.

Fixes #3819

Review on Reviewable

{
"CREATE TABLE d.t (i INT, s STRING, d INT)",
nil,
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sort of strange that these are test cases. They are really part of test setup. I think it would be cleaner to pull them out and actually perform them as separate setup steps. You can do something like:

  setup := `CREATE DATABASE d; CREATE TABLE d.t (i INT, s STRING, d INT);`
  if err := db.Exec(setup); err != nil {
    t.Fatal(err)
  }

Make sure prepare statements are run in a transaction.

Use a new boolean flag on the planner to indicate prepare. Statement
implementations will gain knowledge of work to omit during prepare. An
alternative implementation would split up the prepare and exec portions of
the work. However, this would require passing around significant amounts
of state and I think the best implementation is the one here.

Keep a whitelist of statement types that support preparation instead of
attempting to exec them during prepare.

This also functions as a refactor for prepare statements in
general. Previously, we called parser.InferArgs, which walked the statement
and ran TypeCheck on all expressions. It turns out this is not needed,
since Select (and others) do this themselves. Thus, we can invoke those
worker functions directly but teach them about prepareOnly so they can
omit work once the val args have been populated and the type checks done.

Fixes #3819
@maddyblue
Copy link
Contributor Author

Review status: 0 of 12 files reviewed at latest revision, 4 unresolved discussions.


sql/pgwire_test.go, line 330 [r1] (raw file):
Done.


sql/pgwire_test.go, line 396 [r1] (raw file):
Adding a TODO for this, discussed below.


sql/plan.go, line 171 [r1] (raw file):
Adding a TODO for that.


Comments from the review on Reviewable.io

@petermattis
Copy link
Collaborator

LGTM


Review status: 0 of 12 files reviewed at latest revision, 1 unresolved discussion.


Comments from the review on Reviewable.io

maddyblue added a commit that referenced this pull request Jan 30, 2016
sql: prepare support for INSERT, UPDATE, DELETE
@maddyblue maddyblue merged commit 191a473 into cockroachdb:master Jan 30, 2016
@maddyblue maddyblue deleted the sql-prepare-others branch January 30, 2016 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants