Version increment should be atomic

[pedroteixeira]

Hi,
The current strategy is not very safe. The following line:

version_number = new_record? ? 1 : version + 1

could be executed on the DB to guarantee consistency.
Has anyone thought about this?

thanks,
Pedro

[key88sf]

Why was this closed? It's a valid concern. Should have a unique index on the version number / object ID at least.

[jmckible]

Example code that fails and/or proposed solution?

[key88sf]

• Run multiple app servers
• Issue two simultaneous submits that change a version_fu'd record
  (e.g. double-click on a submit button, or two admins have the same page open and submit at the same time)

Each app server will get the update request, and both will see the version of the object as being N, so both will do an insert of a new row with version N+1, resulting in two rows with the same version.

[key88sf]

Proposed solution is to have a unique index on (object_id, version) in the database table, plus probably rescue the error in the version_fu code.

[jmckible]

Sounds like a good solution, even though I'm not sure the race condition actually comes up, since both of those instances of the master object would be separate, and therefore should instantiate different version. If I'm following your example correct?

Feel free to send over a pull request.

[key88sf]

We are using version_fu in one of our projects and are already noticing duplicate rows for two of the tables (the two which have modifications triggered by humans, not by code). We're adding the unique index and going to look at the stack traces, but the suspected case is a double-click submit or something like that.

[jmckible]

Interesting. So the problem is in the versions table, not the master object, correct? Also, which database? Assuming you're using Rails, what version?

[key88sf]

Yes in the version table.
Rails 3, MySQL

There are rows that look like this:

object_id version [other cols]

---

2 2 ....
2 2 ....

[jmckible]

Ah, I see, so the issue isn't actually with version 1, it's with further versions. Totally get it, yes this is a bug.

[key88sf]

Yeah. Once there is at least one version in the versions table, there can be a duplicate.

Actually I think the best would be to unique index in the database, plus rescue that exception and increment the version and try the insert again until no more exception is thrown.

[key88sf]

How about using self.class.update_counters(self.id, :version => 1) ?

[jmckible]

It might work, but the main problem is that the version is not persisted to the database automatically. It wouldn't get an id until that point. So perhaps update_counters would work inside a callback?

[key88sf]

Well I think ideally should be something like this:

ActiveRecord::Base.transaction do
self.class.update_counters ... # increment 'version' on the main class
new_version = self.class.find(self.id).version

Build and save new row for the versioning table here

...
end