T187226: Do not use count(*) for reading list size limits requires a new column to be added to wikishared.reading_list on x1. It is a single shared table used by all wikis, the code handling it is not exposed to traffic yet so it's practically empty (400 rows) and probably gets approximately zero requests per minute.
The required change is
ALTER TABLE reading_list ADD COLUMN rl_size INTEGER UNSIGNED NOT NULL DEFAULT 0;
This change does not affect existing code and can be deployed at any time.
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | Tgr | T187226 Do not use count(*) for reading list size limits | |||
| Resolved | • jcrespo | T188048 Deploy ReadingLists schema change for efficient count(*) handling |
We normally wait till all the patches are merged.
Another question, you said you can handle this on your own. Do you have ALTER privileges?
The table is certainly pretty small indeed and this could be run directly on the master and leave it replicate.
Do you have ALTER privileges
All deployers have alter privileges, but they shouldn't use them except in an emergency, for the reasons expresed here: https://wikitech.wikimedia.org/wiki/Schema_changes#Dangers_of_schema_changes . Using them on x1, not matter how small they are, can create huge problems for issues like metadata locking. Please let us handle that.
I asked Tgr to make those changes due to bad performance and promised in exchange schema change priority- I can handle this quickly.
Note, all the above is for the addition of the column, which is simple. The index changes need additional scrutiny, as it is the first time I know of them (they should be on a separate index).
Once you are in production, things have to be treated as formally as other parts- if schema isn't stable, I recommend undeploy and redeploy once it is stable.
OK, let me split this up into two tasks then.
Once you are in production, things have to be treated as formally as other parts- if schema isn't stable, I recommend undeploy and redeploy once it is stable.
The schema isn't stable because the use cases keep changing, which is normal for a new feature that has just been deployed to production and is being seen by QA, beta testers etc. I don't think it's realistic to figure out the eventual schema without making products work with real data.
Anyway, I don't think the index changes are particularly urgent. They enforce a new uniqueness criteria on the DB level, but the code will enforce it anyway so it's mostly just schema hygiene.
Note I am not saying you should undeploy and redeploy, I was just justifying that, once this are in production, they become hardere to change. No issue with the new indexes, but please file them separately, needs review (unlike the new column, that was asked by me).
Separate tasks for the same table are normally deployed at the same time, do not worry about that :-)
The other task is T188120: Deploy ReadingLists schema change for unique list names; I'm not adding the schema change tag yet, if it cannot easily be deployed the same time as the corresponding patch I should probably update the patch to behave a bit nicer without the change, so let's leave that off the table for now.
This has been applied to production:
$ grep -v dbstore1001 dbtools/x1.hosts | while read host port; do echo "$host::$port"; mysql -BN -h $host -P $port wikishared -e "SHOW CREATE TABLE reading_list\G"; done dbstore2001.codfw.wmnet::3320 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary dbstore2002.codfw.wmnet::3320 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary db2034.codfw.wmnet::3306 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary db2033.codfw.wmnet::3306 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary dbstore1002.eqiad.wmnet::3306 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary db1056.eqiad.wmnet::3306 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary db1055.eqiad.wmnet::3306 *************************** 1. row *************************** reading_list CREATE TABLE `reading_list` ( `rl_id` int(10) unsigned NOT NULL AUTO_INCREMENT, `rl_user_id` int(10) unsigned NOT NULL, `rl_is_default` tinyint(4) NOT NULL DEFAULT '0', `rl_name` varbinary(255) NOT NULL, `rl_description` varbinary(767) NOT NULL DEFAULT '', `rl_date_created` binary(14) NOT NULL DEFAULT '19700101000000', `rl_date_updated` binary(14) NOT NULL DEFAULT '19700101000000', `rl_deleted` tinyint(4) NOT NULL DEFAULT '0', `rl_size` int(10) unsigned NOT NULL DEFAULT '0', PRIMARY KEY (`rl_id`), UNIQUE KEY `rl_user_deleted_name_id` (`rl_user_id`,`rl_deleted`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_deleted_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), UNIQUE KEY `rl_user_name_id` (`rl_user_id`,`rl_name`,`rl_id`), UNIQUE KEY `rl_user_updated_id` (`rl_user_id`,`rl_deleted`,`rl_date_updated`,`rl_id`), KEY `rl_user_default` (`rl_user_id`,`rl_is_default`), KEY `rl_deleted_updated` (`rl_deleted`,`rl_date_updated`) ) ENGINE=InnoDB AUTO_INCREMENT=4224 DEFAULT CHARSET=binary
But I think there is a problem, HEAD https://phabricator.wikimedia.org/diffusion/ERLS/browse/master/sql/readinglists.sql doesn't seem to have that extra column.
Thanks!
No problem, the patch adding that will be merged soon. I didn't want to merge it before the change is applied in production (which admittedly does not make much sense in hindsight).