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

Rewrite stmt cache for transactions #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

devsergiy
Copy link

Implements stmt cache for transaction

Reason of rewrite: Prepared statements should be created on transaction. So cache should be created with using transaction as Preparer

Test will be added soon. Waiting for any suggestions

@lann
Copy link
Member

lann commented Jan 28, 2015

I'm trying to be very careful about backwards compatibility in this library, and this change removes an existing interface. I'm probably happy to add your new interface and implementation, but not at the expense of breaking the existing one.

@lann
Copy link
Member

lann commented Jan 28, 2015

Other than backwards compat (and minor comment on your inline example) this looks good to me.

@devsergiy
Copy link
Author

Hi @lann

I didn't find examples of use existing DBProxyBeginner interface.
What was intent of DBProxyBeginner interface and NewStmtCacheProxy method?
At first sight it looks like creating cache for transactions, but it will not get perfomance improvement from transactions.
Or we should use transaction as preparer for builders by hands? But in this case this interface has no sense

So how would be better to adopt existing interface or create a new one?

@lann
Copy link
Member

lann commented Jan 28, 2015

DBProxyBeginner is exported, so any external project that calls NewStmtCacheProxy could be assigning to a variable with that explicit interface type. Removing the interface from squirrel would break that external code.

@devsergiy
Copy link
Author

@lann redo commit as adding new interfaces.

I have one concern about starting transactions on NewStmtCacheTransactionProxy. On the one hand this method should be used only when you need a transaction, but maybe would be better always start transaction by hands and add note that any call to Exec(), QueryRow etc. should be wrapped in cache.Begin() cache.Commit() to be cached for transaction?

@lann
Copy link
Member

lann commented Jan 28, 2015

That's a good point. What if it went directly to the DB by default, and then swapped in the transaction when the user calls Begin and swap back to the DB on Commit/Rollback?

@devsergiy
Copy link
Author

@lann Hi, I've updated pull request and added fallback to StmtCacher when we don't star the transaction

@lann
Copy link
Member

lann commented Jul 29, 2015

Unfortunately I will not be able to review or merge major changes for the forseeable future; see README.

@technosophos
Copy link
Member

Lann has moved Squirrel to the Masterminds project, and is still the architect, but @mattfarina and I will be helping him maintain. We'll start reviewing pull requests and see if we can get this in.

@devsergiy
Copy link
Author

@technosophos Sure, will update PR in case of any issues

@matteosuppo
Copy link

Is this going to be addressed, by any chance?

@devsergiy
Copy link
Author

@matteosuppo I've proposed this long time ago, I could revisit this to make build green

@technosophos will u be able to do a review after that?

@lann
Copy link
Member

lann commented Oct 16, 2019

@technosophos will u be able to do a review after that?

@spetrunin I am (back to) maintaining this project now. I will review this again if it is updated.

@stampy88
Copy link

stampy88 commented Oct 6, 2020

@spetrunin are you still working on fixing this?

@devsergiy
Copy link
Author

@stampy88 didn't use squirrel for a long time
but could fix a build for this pr:)

@2opremio
Copy link

I would also find this useful!

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.

6 participants