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

implementing PREPARE, EXECUTE, and DEALLOCATE #1448

Merged
merged 59 commits into from
Dec 16, 2022
Merged

implementing PREPARE, EXECUTE, and DEALLOCATE #1448

merged 59 commits into from
Dec 16, 2022

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Dec 3, 2022

Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Looking pretty good. I think it's worth going a little further to tidy things up though.

engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
engine.go Outdated Show resolved Hide resolved
sql/parse/parse.go Outdated Show resolved Hide resolved
sql/plan/prepare.go Outdated Show resolved Hide resolved
sql/plan/prepare.go Show resolved Hide resolved
@@ -334,6 +351,73 @@ func (e *Engine) analyzeQuery(ctx *sql.Context, query string, parsed sql.Node, b
return nil, err
}

switch n := parsed.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't thought through this fully yet... so I might be missing something... but I'm wondering if there is another way to execute these statements that is more similar to how we execute other statements. It seems inconsistent that these statements are "executed" directly inside engine.go. I'm wondering if that creates other edge cases.

I see that they need to access the engine to call some functions, but I'm wondering if you had the PreparedStatementCache type I mentioned in a previous comment that could be set in these types, if that would decouple them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do agree it's a little strange to have these in the engine.go file, but I couldn't think of another place to put it.
I'm not quite sure what happens when you prepare a prepare statement either, but I'm assuming not great things.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good edge case with preparing a prepare statement! It looks like MySQL's grammar doesn't allow this, so it's always a syntax error. Would definitely be a good test case to cover.

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that you have PreparedDataCache... it seems like you're closer to being able to pull this out of engine.go... if you could set the PreparedDataCache instance on these plan nodes, then they could run their logic to set/delete in the cache as part of their rowIter func. Seems like we would need another analyzer rule to catch these nodes and run the Analyzer calls to PrepareQuery and AnalyzedPrepared though.

@max-hoffman – what do you think? any better ideas for us here?

Copy link
Contributor

Choose a reason for hiding this comment

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

i think the answer depends on whether PREPARE and DEALLOCATE are exempted from the transactional lifecycle. for example, if you start a transaction with an already prepared query, unbind a prepared statement, and then rollback the unbind, does MySQL restore the prepared query? or did the unbind not respect transaction boundaries? the reverse -- prepare a statement, and then rollback -- does the session still see the cached plan?

enginetest/queries/script_queries.go Show resolved Hide resolved
@@ -55,6 +55,65 @@ type TemporaryUser struct {
Password string
}

// PreparedDataCache manages all the prepared data for every session for every query for an engine
type PreparedDataCache struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is beautiful! It encapsulates this logic cleanly, shrinks the locking granularity, and makes this code a lot easier to read. Nice work! 👏

engine.go Show resolved Hide resolved
engine.go Show resolved Hide resolved
Copy link
Contributor

@fulghum fulghum left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this one! I think your changes really improved this code a ton.

@jycor jycor merged commit 2c59563 into main Dec 16, 2022
@Hydrocharged Hydrocharged deleted the james/prepareds branch December 16, 2022 10:49
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.

3 participants