Page MenuHomePhabricator
Create Task
Maniphest T26782

API uses non-unique value for paging for some modules
Closed, ResolvedPublic
Actions

Description

When listing recentchanges, the API uses rctimestamp for paging, which is not a unique index:

<query-continue>

<recentchanges rcstart="2010-07-14T14:21:31Z" />

</query-continue>

This is quite bad: say, there where 23 changes in 14:21:39, but the first call only retrieved 10. Using 14:21:39 for rcstart for the next page will return the *same* 10 again, not any of the 134 remaining records. This might even put the client into an infinite loop.

Using 14:21:40 would not be better: it go to the next timestamp, thus effectively skipping the 13 items we didn not yet read for 14:21:39.

<query-continue> absultely MUST use a unique index. The only unique index for recentchanges is rcid - but is it guaranteed to increase monotonously? Maybe <query-continue> could use a compund index too, like <recentchanges rcstart="2010-07-14T14:21:31Z" rcid="667455"/> ?

The problem is also how to change this in a B/C fashion. In theory, the client should simply use whatever attriubute it finds in <query-continue><recentchanges>. But can we be sure non has the timestamp hard-coded for that? Would it be ok to break clients that rely on rcstart being a timestamp?

Version: 1.17.x
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=46508
https://bugzilla.wikimedia.org/show_bug.cgi?id=65251

Details

Reference
bz24782

Related Objects
Search...

StatusSubtypeAssignedTask
 ResolvedReedyT157227 MediaWiki DB tables with columns which references other columns but have different type (tracking)
ResolvedNoneT26782 API uses non-unique value for paging for some modules
 ResolvedLSobanskiT17441 Some tables lack unique or primary keys, may allow confusing duplicate data
 ResolvedNoneT35638 Math table doesn't have a PK
 DeclinedNoneT104314 Every Gather table should have a primary key
 ResolveddanielT102992 [Task] Review WikidataQuality DB schema
 ResolvedmatthiasmullieT109676 Add artificial primary key to flow_wiki_ref and flow_ext_ref
 ResolvedjcrespoT111084 Do backfill and final schema change for "Segregate Reference objects by source wiki."
 DuplicateNoneT113778 Run patch-reference_wiki-phase5.sql in production
 ResolvedjcrespoT117787 Make ref_id of flow_wiki_ref and flow_ext_ref not null and PK (DBA phase 2)
 ResolvedCatropeT117786 Run maintenance/FlowPopulateRefId.php on all production wikis
 ResolvedCatropeT117785 Deploy artificial primary key reference change to all production wikis simultaneously
 ResolvedjcrespoT117783 Add ref_id to flow_wiki_ref and flow_ext_ref (DBA phase 1)
 ResolvedTTOT123225 change_tag doesn't have a primary key
 Resolved• MarosteguiT147166 Apply change_tag and tag_summary primary key schema change to Wikimedia wikis
 Resolved• MarosteguiT130067 Add wl_id to watchlist tables on production dbs
 ResolvedjcrespoT138810 Set databases as read-only or switchover to secondary datacenter
 ResolvedAddshoreT125990 Add a wl_id field to the watchlist table
 ResolvedAddshoreT160062 Allow setting the watchlist table to read-only on a per-wiki basis
 StalledNoneT146571 Add a primary key to querycache table
 OpenNoneT174513 Maintenance script to cleanup querycache
 OpenNoneT146585 Add a primary key to user_newtalk
 OpenNoneT146586 Add a primary key to querycachetwo table
 Resolved• MarosteguiT146591 Add a primary key to l10n_cache
 Resolved• MarosteguiT150306 truncate l10n_cache table on WMF wikis
 ResolvedReedyT146570 Give user_properties a primary key
 ResolvedReedyT172514 Add new PKs to tables.sql (mysql and sqlite)
Restricted Task
 Resolved• MarosteguiT149819 Add primary keys to remaining Flow tables
 Resolved• MarosteguiT163912 Convert unique keys into primary keys for some wiki tables on s3-eqiad and s3-codfw
 Resolved• MarosteguiT163979 Convert unique keys into primary keys for some wiki tables on s6-eqiad and codfw
 Resolved• MarosteguiT164185 Convert unique keys into primary keys for some wiki tables on s1, s2, s4, s5 and s7 (eqiad)
 Resolved• MarosteguiT166204 Convert unique keys into primary keys for some wiki tables on s1
 Resolved• MarosteguiT166205 Convert unique keys into primary keys for some wiki tables on s2
 Resolved• MarosteguiT166206 Convert unique keys into primary keys for some wiki tables on s4
 ResolvedjcrespoT166207 Convert unique keys into primary keys for some wiki tables on s5
 Resolved• MarosteguiT166208 Convert unique keys into primary keys for some wiki tables on s7
 Resolved• MarosteguiT164399 Convert unique keys into primary keys for some wiki tables on all s* shards (codfw)
 ResolvedReedyT198811 Convert UNIQUE indexes to Primary Keys in MW Core Tables
 Resolved• MarosteguiT199368 Convert UNIQUE INDEX to PK in Production
 ResolvedSep 9 2018• MarosteguiT189101 Deploy schema change for adding numeric primary key to wbqc_constraints table
 Resolved• MarosteguiT201011 Apply schema change to translate_reviews in WMF
 DuplicateNoneT231946 user_newtalk table is missing a primary key
 ResolvedDannyS712T243073 Add primary key to pagetriage_page_tags
 ResolvedUmherirrenderT243985 Translate table `revtag` lacks a primary key
 ResolvedDannyS712T243986 CloseWikis table `closedwikis` lacks a primary key
 ResolvedDannyS712T243987 GlobalUsage table `globalimagelinks` lacks a primary key
 In ProgressDiesel_kapasuleT312247 Add primary key for database table cx_suggestions
 OpenNoneT312263 Add primary key for database table translate_tmf
 OpenNoneT312281 Add primary key for database table securepoll_lists
 OpenNoneT67251 Query-continue for imageinfo returns timestamp which is ambiguous if multiple files were uploaded within the same second under the same file name
 OpenNoneT67264 File storage uses (title,timestamp) as a unique key, but this is not unique
 OpenNoneT28741 Migrate file tables to a modern layout (image/oldimage; file/file_revision; add primary keys)
 ResolvedKrinkleT589 RFC: image and oldimage tables

Event Timeline

• bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:09 PM
• bzimport added a project: MediaWiki-Action-API.
• bzimport set Reference to bz24782.
• bzimport added a subscriber: Unknown Object (MLST).
daniel created this task.Aug 13 2010, 2:42 PM
Reedy added a comment.Aug 13 2010, 3:37 PM

I don't think we'd use 2 seperate attributes, the normal "api way" is to use either a globally unique thing, or a composite key "id|somefield|anotherfield" or something to that effect

Catrope added a comment.Aug 13 2010, 3:45 PM

(In reply to comment #0)

<query-continue> absultely MUST use a unique index.

Yes, and the fact that it doesn't is very bad.

The only unique index for
recentchanges is rcid - but is it guaranteed to increase monotonously?

I'm not sure.

Maybe
<query-continue> could use a compund index too, like <recentchanges
rcstart="2010-07-14T14:21:31Z" rcid="667455"/> ?

That would be the nicest solution, probably, but it requires adding rc_id to the indexes we use (I think there's only two so that wouldn't be too bad). It would be encoded differently though, as rcontinue="2010-07-14T14:21:31Z|667455", to be consistent with other modules paging on multiple fields.

The problem is also how to change this in a B/C fashion. In theory, the client
should simply use whatever attriubute it finds in
<query-continue><recentchanges>. But can we be sure non has the timestamp
hard-coded for that? Would it be ok to break clients that rely on rcstart being
a timestamp?

Yes, this is definitely OK. Clients are supposed to simply use whatever's in query-continue and pass it back verbatim. Nevertheless, we would still announce it, but not as a breaking change.

Reedy added a comment.Aug 13 2010, 7:58 PM

It's actually a widerspread problem.

Watchlist and blocks (for 2), do the same sort of just timestamp continue.

Category members if sorted by timestamp

Deletedrevs if !( $mode == 'all' || $mode == 'revs' )

Logevents, protected titles

ImageInfo

• bzimport added a comment.Oct 24 2010, 2:17 PM

Bryan.TongMinh wrote:

(In reply to comment #2)

The problem is also how to change this in a B/C fashion. In theory, the client
should simply use whatever attriubute it finds in
<query-continue><recentchanges>. But can we be sure non has the timestamp
hard-coded for that? Would it be ok to break clients that rely on rcstart being
a timestamp?

Yes, this is definitely OK. Clients are supposed to simply use whatever's in
query-continue and pass it back verbatim. Nevertheless, we would still announce
it, but not as a breaking change.

It is not necessary to break b/c here. If the new format is "$timestamp|$rcid", then we can make the API simply accept "$timestamp" as well, which would then default to "$timestamp|0"

Reedy added a comment.Jun 5 2011, 8:39 PM

Do we need to worry here that we don't have a rc_timestamp, rc_id index?

I'm presuming, that we shouldn't get that many results simultaneously for one second?

I know adding the index would be teh slow

We could use rc_timestamp, rc_title, but then what if a title gets 2 hits in a second. Bleh

Reedy added a comment.Jun 5 2011, 8:39 PM

Created attachment 8616
recentchanges

Is this actually even right? I'm getting tired, and can't really think

Attached:

bug_24782_recentchanges.patch2 KBDownload

Reedy added a comment.Jun 5 2011, 8:50 PM

(In reply to comment #3)

It's actually a widerspread problem.

Watchlist and blocks (for 2), do the same sort of just timestamp continue.

Category members if sorted by timestamp

Deletedrevs if !( $mode == 'all' || $mode == 'revs' )

Logevents, protected titles

ImageInfo

ImageInfo is fine (no idea why I said otherwise)

Deletedrevs if !( $mode == 'all' || $mode == 'revs' ) == ($mode == 'user')

Logevents, protected titles

Category members if sorted by timestamp

Watchlist and blocks

Reedy added a comment.Jun 5 2011, 8:59 PM

Blocks, we can just use timestamp and id

Watchlist is essentially the same as recent changes..

Category members if by timestamp....?

Log events use timestamp and id

Protected titles, timestamp and title?

Deleted revs?

edwardspec added a comment.Aug 8 2011, 2:39 PM

This is indeed very bad.

I just failed to download a ruwiki recent blocklog because of the adminbot that blocks proxies (>500 per second).

list=logevents API is unusable under these circumstances.

Catrope added a comment.Nov 20 2011, 2:52 PM

(In reply to comment #6)

Created attachment 8616 [details]
recentchanges

Is this actually even right? I'm getting tired, and can't really think

It's got the right idea, but it needs an index on (rc_timestamp, rc_id) or something else unique. Otherwise MySQL's stupidity makes the query slow.

Attached:

bug_24782_recentchanges.patch2 KBDownload

duplicatebug added a comment.Feb 18 2012, 6:07 PM
  • Bug 34029 has been marked as a duplicate of this bug. ***
duplicatebug added a comment.Apr 7 2012, 1:59 PM
  • Bug 35786 has been marked as a duplicate of this bug. ***
• bzimport added a comment.Jun 6 2012, 12:47 PM

sumanah wrote:

Are folks still experiencing this problem?

daniel added a comment.Sep 5 2012, 6:12 PM

Fixed the patch a bit and submitted it to gerrit as Icc43b62f

daniel added a comment.Sep 5 2012, 6:20 PM

Remaining issues:

As per comment #3 and #8, there are several more API modules with similar problems that still need to be addressed.

As per comment #2, rc_id is not part of the database index currently. Need to investigate whether that is an actual issue.

Anomie added a comment.Jan 23 2013, 5:14 PM

Regarding the indexes, it looks like it's sort of an issue.

Normally, MySQL will insist on filesorting due to the extra field in the ORDER BY, which is certainly an issue for recentchanges because it doesn't seem like it's smart enough to fetch just the first 50 by the index plus only those extra are "tied" for 50th place in the non-unique index.[1]

But with the way InnoDB indexes work, one index (usually the primary key) is effectively appended to every other index[2] and MySQL will take advantage of this when performing queries. So for the recentchanges changes table, since we use InnoDB and the primary key is (rc_id), the rc_timestamp index is secretly (rc_timestamp,rc_id) even though it is defined as just (rc_timestamp). So a query SELECT ... FROM recentchanges WHERE rc_timestamp > '...' ORDER BY rc_timestamp, rc_id LIMIT 50 will not have to filesort under these conditions, and the EXPLAIN output reflects this.

Whether we want to *rely* on this behavior, I don't know.

[1]: https://dev.mysql.com/doc/refman/5.0/en/limit-optimization.html
[2]: https://dev.mysql.com/doc/refman/5.0/en/innodb-index-types.html

Aklapper added a comment.Mar 25 2013, 9:15 AM

(In reply to comment #14)

Fixed the patch a bit and submitted it to gerrit as Icc43b62f

Patch committed on March 05th, 2013, removing keyword due to remaining issues in comment 15.

• MZMcBride added a comment.Apr 3 2013, 2:11 AM

Related: https://gerrit.wikimedia.org/r/57067.

gerritbot added a comment.Dec 24 2013, 9:00 PM

Change 103589 had a related patch set uploaded by Anomie:
API: Make more continuations unique

https://gerrit.wikimedia.org/r/103589

gerritbot added a comment.Apr 11 2014, 6:29 PM

Change 103589 merged by jenkins-bot:
API: Make more continuations unique

https://gerrit.wikimedia.org/r/103589

Anomie added a comment.Apr 11 2014, 7:28 PM

With the merge of Gerrit change 103589, this should be fixed now for all modules in core. If I've missed any, please let me know.

If this bug is present in any extensions, please open a new bug against the relevant extensions.

daniel added a subtask: T17441: Some tables lack unique or primary keys, may allow confusing duplicate data.Apr 8 2015, 11:49 AM
daniel added a subtask: T67251: Query-continue for imageinfo returns timestamp which is ambiguous if multiple files were uploaded within the same second under the same file name.
daniel mentioned this in T95194: Validate how the updater determines where it left off.Apr 8 2015, 11:51 AM
Anomie mentioned this in T131065: API: Contributions: Database query error.Mar 29 2016, 2:30 PM
whym mentioned this in T48508: Non-unique value is used for paging on some pages.Sep 1 2016, 3:28 AM
Liuxinyu970226 mentioned this in T67251: Query-continue for imageinfo returns timestamp which is ambiguous if multiple files were uploaded within the same second under the same file name.Mar 29 2017, 1:47 PM
Anomie mentioned this in T59176: ApiQueryExtLinksUsage::run query has crazy limit.Apr 27 2017, 2:45 PM
Aklapper removed subscribers: Anomie, • wikibugs-l-list.Oct 16 2020, 5:43 PM
LSobanski closed subtask T17441: Some tables lack unique or primary keys, may allow confusing duplicate data as Resolved.May 17 2021, 11:09 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL