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

Race condition in update #6606

Closed
leedm777 opened this issue Sep 23, 2015 · 19 comments

Comments

@leedm777
Copy link
Contributor

commented Sep 23, 2015

Reopening #189, since it doesn't appear to be resolved.

There is a race condition in update (in adapter.js) that I've run into a few times. The current update implementation works by:

  1. Query with the given criteria to get a list of ids
  2. Update with the given criteria
  3. Query with the list of ids from step 1 to get the updated objects

The problem is that if other updates happen between steps 1 and 2, the data returned in step 3 could be inaccurate (either omitting/including records that were/were not affected).

Since this is a MySQL specific driver, there's a pretty clever way of capturing the ids for updated records.

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

@leedm777 thanks for reopening, and thanks for the link. This is definitely something I'd like to see a PR for-- we're not currently using MySQL ourselves, but I suspect there's a similar situation in postgresql that we could tackle ourselves as a template cc @devinivy @particlebanana

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Oct 5, 2015

@leedm777 added to the Sails core ROADMAP.md so we have this centralized, and in case anyone reading that has time to help (9c587b7)

@devinivy

This comment has been minimized.

Copy link

commented Oct 5, 2015

I like that. @tjwebb are knex or your new adapters aware of this?

@particlebanana

This comment has been minimized.

Copy link
Contributor

commented Oct 16, 2015

@mikermcneil the postgres adapter wouldn't have this issue because it can return the values from the update. This would be useful and would cut a lot out of the update function.

It would look something like this: https://github.com/balderdashy/sails-postgresql/blob/master/lib/adapter.js#L981-L989 with SET @uids := null; appended to the beginning of the generated query and AND ( SELECT @uids := CONCAT_WS(',', pk, @uids) ); SELECT @uids; appended to the end.

@sailsbot

This comment has been minimized.

Copy link

commented Nov 16, 2015

Thanks for posting, @leedm777. I'm a repo bot-- nice to meet you!

It has been 30 days since there have been any updates or new comments on this page. If this issue has been resolved, feel free to disregard the rest of this message. On the other hand, if you are still waiting on a patch, please:

  • review our contribution guide to make sure this submission meets our criteria (only verified bugs with documented features, please; no questions, commentary, or bug reports about undocumented features or unofficial plugins)
  • create a new issue with the latest information, including updated version details with error messages, failing tests, and a link back to the original issue. This allows GitHub to automatically create a back-reference for future visitors arriving from search engines.

Thanks so much for your help!

@sailsbot sailsbot closed this Nov 16, 2015

@devinivy

This comment has been minimized.

Copy link

commented Nov 16, 2015

Reopening because this is a pending bug.

@devinivy devinivy reopened this Nov 16, 2015

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

@devinivy thank you

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Dec 22, 2015

@devinivy this should be good now-- the "bug" label is ignored as of Nov 16

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

@leedm777 Thanks again for reopening this issue.

@shamasis is this one of the issues you guys recently addressed/encountered in your fork?

@mikermcneil

This comment has been minimized.

Copy link
Member

commented Jan 14, 2016

By the way @leedm777, I'm working on cleaning up the roadmap, so if you wouldn't mind adding any other specific suggestions you might have as far as an approach for a patch, I'd appreciate it. I took a look at your example query in the link-- if we could implement this as a single query, this wouldn't depend on transactions, and it could be implemented as an improvement to this module. I'm going to leave it as a pending proposal either way for the time being-- if it turns out this cannot be implemented in a single query, would one of you guys open a PR to Waterline adding a backlog item to ROADMAP.md?

@shamasis

This comment has been minimized.

Copy link

commented Jan 20, 2016

This is generic transaction specific issue and IMHO not a race condition. :-)

@particlebanana

This comment has been minimized.

Copy link
Contributor

commented Jan 26, 2016

Yeah because these all happen on the same connection it's probably possible to wrap this in a transaction even without adding support for transactions at a higher level.

@leedm777

This comment has been minimized.

Copy link
Contributor Author

commented Jan 26, 2016

I don't think transactions alone would help; at least not with the default isolation level.

From the MySQL docs:

The snapshot of the database state applies to SELECT statements within a transaction, not necessarily to DML statements. If you insert or modify some rows and then commit that transaction, a DELETE or UPDATE statement issued from another concurrent REPEATABLE READ transaction could affect those just-committed rows, even though the session could not query them. If a transaction does update or delete rows committed by a different transaction, those changes do become visible to the current transaction. For example, you might encounter a situation like the following:

SELECT COUNT(c1) FROM t1 WHERE c1 = 'xyz'; -- Returns 0: no rows match.
DELETE FROM t1 WHERE c1 = 'xyz'; -- Deletes several rows recently committed by other transaction.

SELECT COUNT(c2) FROM t1 WHERE c2 = 'abc'; -- Returns 0: no rows match.
UPDATE t1 SET c2 = 'cba' WHERE c2 = 'abc'; -- Affects 10 rows: another txn just committed 10 rows with 'abc' values.
SELECT COUNT(c2) FROM t1 WHERE c2 = 'cba'; -- Returns 10: this txn can now see the rows it just updated.
@sailsbot

This comment has been minimized.

Copy link

commented Mar 2, 2016

Thanks for posting, @leedm777. I'm a repo bot-- nice to meet you!

The issue queue in this repo is for verified bugs with documented features. Unfortunately, we can't leave some other types of issues marked as "open". This includes bug reports about undocumented features or unofficial plugins, as well as feature requests, questions, and commentary about the core framework. Please review our contribution guide for details.

If you're here looking for help and can't find it, visit StackOverflow, our Google Group or our chat room. But please do not let this disrupt future conversation in this issue! I am only marking it as "closed" to keep organized.

Thanks so much for your help!

If I am mistaken, and this issue is about a critical bug with a documented feature, please accept my sincerest apologies-- I am but a humble bot working to make GitHub a better place. If you open a new issue, a member of the core team will take a look ASAP.

@sailsbot sailsbot closed this Mar 2, 2016

@magicdawn

This comment has been minimized.

Copy link

commented Jan 7, 2017

Status ?

@magicdawn

This comment has been minimized.

Copy link

commented Jan 7, 2017

@leedm777

Will a transaction + SELECT ... FOR UPDATE work?
since other select will block

@leedm777

This comment has been minimized.

Copy link
Contributor Author

commented Jan 9, 2017

@magicdawn That would catch some cases, but not all. The select would only lock the records that match the where clause at the time of the select. If other records are inserted/updated, they could be included in the update but have been omitted from the original select.

@magicdawn

This comment has been minimized.

Copy link

commented Jan 13, 2017

@leedm777 Gotcha

@magicdawn

This comment has been minimized.

Copy link

commented Jan 13, 2017

To get the updated ids. I tried the stackoverflow answer:

UPDATE TAB_NAME
SET foo = 1
WHERE (
  ... -- condition
) AND (
  SELECT @update_ids := CONCAT_WS(',', @update_ids, 'id')
)

It's Ok, but the LIMIT 10 will not work, if I just want to update only 10 rows
So the assignment must be placed in the SET ... statement

I tried to assign a variable and do not influence any innocent column

UPDATE TAB_NAME
SET foo = 1, id = ELT(1, id, CONCAT_WS(',', @update_ids, 'id'))
WHERE (
  ... -- condition
)

but seems mysql got some optimization on ELT(1, id, assign-expression), the assign-expression did not execute

And finally, I add a new column for this table, called extra

UPDATE TAB_NAME
SET foo = 1, extra = CONCAT_WS(',', @update_ids, 'id')
WHERE (
  ... -- condition
)

It workes well, but need extra column. Posting this.

@johnabrams7 johnabrams7 transferred this issue from balderdashy/sails-mysql Jun 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.