Skip to content

Conversation

@hkrutzer
Copy link
Contributor

@hkrutzer hkrutzer commented Oct 21, 2020

No description provided.

@hkrutzer hkrutzer changed the title prepare unnamed reuse statements Reuse prepared statements for repeat statements in unnamed mode Oct 21, 2020
Comment on lines -192 to +194
assert query == query2
assert query.ref != query2.ref
assert query.statement_id != query2.statement_id
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not 100% sure about this one

Copy link
Member

Choose a reason for hiding this comment

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

We should be executing the prepared query without repreparing it.

@hkrutzer hkrutzer force-pushed the prepare-unnamed-reuse-statements branch from ed93b19 to 0f97bab Compare November 30, 2020 07:31
@hkrutzer
Copy link
Contributor Author

@wojtekmach do you have time to take a look at this?

@wojtekmach
Copy link
Member

Yeah, sorry for radio silence, I'll try to get to it soon.

@hkrutzer
Copy link
Contributor Author

@wojtekmach @josevalim ping :)

@greg-rychlewski
Copy link
Member

Is the idea here that you if you execute the same unnamed query more than once in a row, you don't re-prepare? But if you are using query or prepare or prepare_execute then you always re-prepare so you're not comparing statements each time you issue a new unnamed query?

Just asking because I had some ideas reading through this PR but want to make sure I understand the goal.

@josevalim
Copy link
Member

Yes, that's the idea. note it applies only to unnamed mode (when we prepare the queries and immediately discard them).

@greg-rychlewski
Copy link
Member

I think this does what you want: greg-rychlewski@fd99c30

For unnamed:

  • don't immediately close the query
  • close the last query on new prepare or executing a different query

For named:

  • keep the same behaviour immediately closing queries with "" name (i.e. if they call query without specifying cache_statement). the alternative is requiring every named query to check if the last query has name "" and potentially issuing 2 closing messages (the "" query and a stale cache entry)

Sorry I put this in my own branch because I thought it would be clearer than issuing fragmented comments. But if these ideas are good they can be applied here.

@greg-rychlewski
Copy link
Member

@josevalim I didn't notice the age of this PR the first time I replied. Would it be acceptable to submit my version here: greg-rychlewski/myxql@fd99c30? Or if this is expected to pick back up I can leave it alone, no problem.

@josevalim
Copy link
Member

@greg-rychlewski you can also send your commit on top of this one. So everyone gets recognized. Would that work?

@greg-rychlewski
Copy link
Member

Oh yeah, for sure. Sorry I just never did anything like that before. I just push directly to the fork being used by the PR?

@josevalim
Copy link
Member

Here is what I would do:

  1. Get @hkrutzer's branch on your machine
  2. Apply your commit on top of his
  3. Push this new branch from your fork
  4. Send a PR

@wojtekmach
Copy link
Member

Closing in favour of #146

@wojtekmach wojtekmach closed this Jan 22, 2022
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.

4 participants