Page MenuHomePhabricator

Add a wl_id field to the watchlist table
Closed, ResolvedPublic5 Estimated Story Points

Description

A wl_id field should be added to the watchlist table and probably used as the Primary Key

Code can likely be extracted from https://gerrit.wikimedia.org/r/#/c/245881/

This has been extracted from my comment at T124752#1998193

Event Timeline

Addshore raised the priority of this task from to Needs Triage.
Addshore updated the task description. (Show Details)
Addshore subscribed.
Addshore set Security to None.

Change 271435 merged by jenkins-bot:
Add id field to watchlist db table

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

Resolved as this has been merged.

For using the field please see T127954
For the blocker of this field being added to wikimedia dbs please see T130067

( I wonder why i got notified about this change. not a subscriber, doesn't have operations tag, not a watcher of the projects afaict)

According to https://www.mediawiki.org/wiki/Manual:Watchlist_table that table used to have a primary key? But I cannot trace that to the source code, up to 2004? With low priority, could someone double-check that documentation?

@jcrespo: Just wondering if you had an updated ETA on rolling this out on production.

According to https://www.mediawiki.org/wiki/Manual:Watchlist_table that table used to have a primary key? But I cannot trace that to the source code, up to 2004? With low priority, could someone double-check that documentation?

user_id,namespace,title was the PRIMARY. That's how we issue batched deletes to it already.

...or did you mean "that table did NOT used to have a primary key"? I looked at that and could not find, for the life of me, anything in the code indicating that it didn't have a PRIMARY KEY. Even the 2003 revision by Lee Crocker was:

CREATE TABLE watchlist (
  wl_user int(5) unsigned NOT NULL,
  wl_namespace tinyint(2) unsigned NOT NULL default '0',
  wl_title varchar(255) binary NOT NULL default '',
  UNIQUE KEY (wl_user, wl_namespace, wl_title)
) TYPE=MyISAM PACK_KEYS=1;

I think https://www.mediawiki.org/w/index.php?title=Manual:Watchlist_table&oldid=60882 was just a mistake.

@aaron, I do not know about the code, but there is currently no PRIMARY KEY on production. UNIQUE != PRIMARY. Different thing is if you used that as if it was a PRIMARY KEY.

@kaldari for this one we really need a full dc depool. I can apply it to codfw at any moment, but not to eqiad, which is the active dc. I asked to other ops and I was told it was going to take 6 months for the next potential depool, 6 months ago. Lately I was told that next quarter is going to be complicated with ops offsite and if it is close to Christmas, so next date may be early January?

I remember to push for this whenever there is a discussion about it, I am the #1 interested on this to be applied. Sadly, moving around a primary key is not that easy and the dc dates are out of my control. Once we have a stable PK, subsequent changes will be way easier. Track T130067 and see how it is blocked by T138810. Complain there!

Isn't the first UNIQUE index the PRIMARY KEY if none is explicitly otherwise assigned? How do those table not have PKs?

Isn't the first UNIQUE index the PRIMARY KEY if none is explicitly otherwise assigned?

No that is used internally by InnoDB (if none of the columns are null) for index clustering/row identifier; at SQL level, the table still lacks primary key. To give you an idea why that matters- row based replication will not know about that internal primary key, and only the latest versions will try to use a secondary key in a smart way. There are also some subtle issues with having such an id implied on certain query plans. Other, external tools may be confused, too. We also not use InnoDB exclusively (sadly).

How do those table not have PKs?

You can create a table without a primary key (even if InnoDB does have one); nobody here said it was a good idea :-)