When going through some SQL logs, I noticed that the some queries are not coherent with what the code intend to do due to mixing usage of Active Record & mini_sql.
When trying to update some reviewables, we increment the version inside a transaction to prevent some possible race conditions (as the code snippet below shows). Although it appears that the increment is part of the transaction, it is actually not.
Reviewable.transaction here is a Active Record feature, while increment_version! uses mini_sql:
https://github.com/discourse/discourse/blob/d0ef952af296f4fe00c8be56f4e8cede8095d2d6/app/models/reviewable.rb#L628-L650
Active Record is not aware of what mini_sql does. It seems that Active Record uses some lazy feature, in which the BEGIN is issued right before the first query inside the transaction is issued. So version is incremented first and then the transaction begins right before the first query from Active Record is issued. It become something like this:
UPDATE reviewables SET version = version + 1 WHERE version=version AND id = reviewable.id RETURNING version;
BEGIN;
...
With the version increment outside transaction, some guarantee given by optimistic locking won’t be achieved. Because the increment is not atomic with the transaction.