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

Support for FOR UPDATE and FOR SHARE queries #1777

Merged
merged 2 commits into from May 16, 2014

Conversation

4 participants
@janmeier
Member

janmeier commented May 16, 2014

This PR is mostly just a travis runner before I merge this in :) .Closes #1442

'LIMIT ON UPDATE':true,
rowLocking: true,
forUpdate: 'FOR UPDATE',
forShare: 'LOCK IN SHARE MODE'

This comment has been minimized.

@janmeier

janmeier May 16, 2014

Member

@mickhansen Do you think this is okay - tacking the actual query string into the supports object so we only have to write the code in abstract QG, or should we have a separate object for this?

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

@janmeier not sure, this seems reasonable, although i assume you mean you only write the code in the dialect (Rather than the abstracg QG)

This comment has been minimized.

@janmeier

janmeier May 16, 2014

Member

What i meant was we only need to add the code that adds FOR SHARE / LOCK IN SHARE MODE in the abstract query generator, while the string for the specific dialect is in index

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

Right :)

@@ -956,6 +956,14 @@ module.exports = (function() {
query = mainQueryItems.join('')
}
if (this._dialect.supports.rowLocking) {
if (options.forUpdate) {

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

options.lock is obviously simpler, but if we do need to support both for update and for share that doesn't quite work out i suppose.

This comment has been minimized.

@janmeier

janmeier May 16, 2014

Member

Perhaps a options.lock = 'SHARE' / 'UPDATE' would be cleaner?. Dunno actually

t2Jan.updateAttributes({
awesome: false
}, { transaction: t2 }).then(function () {

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

Isn't there a a forShare: true option missing here?

This comment has been minimized.

@janmeier

janmeier May 16, 2014

Member

t1 has already locked the data for share, which means t1 can select it but not update it before t1 has comitted

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

Oh it's further up, i missed that :D nvm

username: 'jan'
},
forUpdate: true
}, { transaction: t2}).then(function () {

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

is transaction supported as part of the first options object?
This signature seems kind of weird - transaction and forUpdate should probably be together, whether or not that should be it's own object or as a part of a find i'm unsure.
But i guess that transaction is something you can pass to anything and forUpdate is find specific.

This comment has been minimized.

@janmeier

janmeier May 16, 2014

Member

I agree it might be more natural to have forupdate together with the transaction. I just did it this way since it was easier, since the first object is passed to the query generator, while the second with the transaction is not. But we could just copy the options from the second into the first before invoking the query generator i guess

This comment has been minimized.

@mickhansen

mickhansen May 16, 2014

Contributor

Yeah i'm not even sure it makes sense to have transaction in its own option, however i'm not sure that it fits in the primary option either.

Obviously the cleanest API would be magic for the transaction but yeah hmm ;o

janmeier added a commit that referenced this pull request May 16, 2014

Merge pull request #1777 from sequelize/forUpdate
Support for FOR UPDATE and FOR SHARE queries

@janmeier janmeier merged commit 3eff6ce into master May 16, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@janmeier janmeier deleted the forUpdate branch May 16, 2014

@cusspvz

This comment has been minimized.

cusspvz commented on lib/transaction.js in 45c5c8f Mar 30, 2015

Shouldn't this be one of option kind? Or are both lines present just for PoC?

This comment has been minimized.

cusspvz replied Mar 30, 2015

I'm referring to:

 *   lock: t1.LOCK.UPDATE,
 *   lock: t1.LOCK.SHARE

This comment has been minimized.

Member

janmeier replied Mar 30, 2015

This is just an example, yuo should only use one if thats what you mean :)

@DenisGorbachev

This comment has been minimized.

DenisGorbachev commented Apr 30, 2015

@janmeier I thought that was a bug in documentation :) Maybe it's better to clarify that the second row is just for example?

@janmeier

This comment has been minimized.

Member

janmeier commented Apr 30, 2015

@DenisGorbachev Agreed - PR welcome :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment