Rewrite stmt cache for transactions

[devsergiy]

trafficstars

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]

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]

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

[devsergiy]

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]

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]

@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]

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]

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

[lann]

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

[technosophos]

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]

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

[matteosuppo]

Is this going to be addressed, by any chance?

[devsergiy]

@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]

@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]

@spetrunin are you still working on fixing this?

[devsergiy]

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

[2opremio]

I would also find this useful!