Page MenuHomePhabricator

Over-reliance on timestamps can lead to incorrect counts
Open, MediumPublic

Description

In several places in the codebase, timestamps are used to determine the order of edits and other actions, which can lead to issues in situations where several actions happen at nearly the same time. One example of this is countRevisionsBetween in Title.php. If multiple revisions of a page are made within 2 seconds, it will return an incorrect count when used on those revisions. Shouldn't this and all similar uses be changed to compare based on the table's primary key instead (revision ID in this case)?

Details

Reference
bz59609

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:42 AM
bzimport set Reference to bz59609.
bzimport added a subscriber: Unknown Object (MLST).

Maybe related: bug 2930 / bug 17591

(In reply to comment #0)

In several places in the codebase

Thanks for reporting this. If you are aware of more places, please list them.

I've just now noticed that edit conflict detection in EditPage.php does the same thing.

624dfd884e683dddc3fb2d86e6764277e8546bef by @aaron changed countRevisionsBetween and countAuthorsBetween to use rev_timestamp instead of rev_id.

It took me some time to figure out why diffs like this don't show the multinotice.

The reason our code often uses timestamps (instead of primary keys) is because revisions can be inserted out of sequence. Most commonly as a result of importing revisions and pages via Special:Import.

It may also happen as result of a history merge (unsure?).

Timestamps tend be to be preferred because they match the way we view revisions on the history page.

As described, I think the task should be declined. But it would be reasonable to extend the resolution of timestamps to the point where they are unique, for example, using a time UUID like UIDGenerator::newTimestampedUID88(). Revision IDs are not monotonic in time and should never be used to select ranges for display.