Do you foresee the need of having an index (or replacing the current ones) with those new columns?
Also subscribing @Anomie here as he's been working on the migrations mentioned.

Do you have a gerrit link with the patch where all these columns are included to link it on this task? We do not execute schema changes until the gerrit patch is merged. Also listing the gerrit link on the task makes it easier to see it, now and specially in the future (just in case).

OOI, how deterministic is two columns using AFTER cuc_private in the same schema change? Does it insert one after cuc_private, then then insert the second, pushing the first column further away from cuc_private?

Is that the intended format?

In T233004#5495636, @Reedy wrote:

OOI, how deterministic is two columns using AFTER cuc_private in the same schema change? Does it insert one after cuc_private, then then insert the second, pushing the first column further away from cuc_private?

Is that the intended format?

is copy and paste miss :

I updated description
ADD COLUMN cuc_comment_id bigint unsigned NOT NULL DEFAULT 0 AFTER cuc_private;
to
ADD COLUMN cuc_comment_id bigint unsigned NOT NULL DEFAULT 0 AFTER cuc_comment;

There is actually not cuc_private column from what I can see on enwiki.
Am I missing something?

root@cumin1001:~# mysql.py -hdb1089 enwiki -e "show create table cu_changes" | grep -i private
root@cumin1001:~#

In T233004#5495664, @Marostegui wrote:

There is actually not cuc_private column from what I can see on enwiki.
Am I missing something?

root@cumin1001:~# mysql.py -hdb1089 enwiki -e "show create table cu_changes" | grep -i private
root@cumin1001:~#

O_o I guess last db schema change patch does not applied WMF production environment IMHO...

rECHU /archives/patch-cu_changes_privatedata.sql

In T233004#5495665, @Rxy wrote:

O_o I guess last db schema change patch does not applied WMF production environment IMHO...

rECHU /archives/patch-cu_changes_privatedata.sql

Apparently not: https://phabricator.wikimedia.org/rECHUede4937ffbd887530c01785a95be349236990ecb there is no ticket for that from what I can see.
And to prevent issues like that, that's why we request all the schema changes to follow this template: https://wikitech.wikimedia.org/wiki/Schema_changes#Workflow_of_a_schema_change although at the time things were different :-)
Hence my request on this ticket to also link a gerrit patch :-)

@Rxy If that column is merged but not in production, I guess it is no used as the change is from 2012, and that code can be cleaned up?

I guess it's not a feature we use with $wgCUPublicKey being "" by default.

So for the wikis without the column, we're not trying to read it, hence it not being noticed in production

Have to wonder if we'll ever want to do so ;)

Most of the newer wikis should have the column though

In T233004#5495718, @Reedy wrote:

Most of the newer wikis should have the column though

Correct, the new ones do have it

root@cumin1001:~# mysql.py -hdb1075 atjwiki -e "show create table cu_changes\G" | grep -i private
  `cuc_private` mediumblob,

root@cumin1001:~# mysql.py -hdb1109 wikidatawiki -e "show create table cu_changes\G" | grep -i private
  `cuc_private` mediumblob,

The usual mess with half applied schema changes :(

In T233004#5495722, @Marostegui wrote:

The usual mess with half applied schema changes :(

Well, not quite. It's just any wikis that were created with the cu_changes.sql file *after* it was merged... And in the last 7 years, will be quite a few wikis at this point

It doesn't help that https://github.com/wikimedia/mediawiki-extensions-CheckUser/commit/ede4937ffbd887530c01785a95be349236990ecb has no reference to a task or anything, so it's not immediately obvious why or for whom this change was made. So I'm slightly reluctant to just strip it out without finding who/what/where/why etc, but at the same time, it's extra work (though, slightly less so if you're already making alters to those tables) for the DBAs, which may end up being reverted at some point

I can easily do an ADD COLUMN IF NOT EXISTS or DROP COLUMN IF EXISTS and create/drop that cuc_private column as part of these schema changes if we find out whether we really need it or not.

In T233004#5495619, @Marostegui wrote:

Do you foresee the need of having an index (or replacing the current ones) with those new columns?
Also subscribing @Anomie here as he's been working on the migrations mentioned.

There seems to be a fair bit of difference between the extension's schemas and the production databases. It might be useful to do a systematic comparison.

As far as the proposed change goes,

• Looking at the existing indexes in the extension, I'd guess they'd want indexes on (cuc_actor, cuc_ip, cuc_timestamp) and (cul_actor, cul_timestamp).
  • I see on enwiki there's also an index KEY `cuc_user_time` (`cuc_user`,`cuc_timestamp`), which would want a corresponding index on (cuc_actor, cuc_timestamp).
• They'd presumably also want to alter cul_user, cul_user_text, and cul_reason to have defaults (0, '', and '', respectively).
  • I note this is already the case on enwiki, but may not be everywhere.

Also, from the description,

5. If the schema change has been tested already on some of the test/beta wikis: It appears to have been auto-deployed to Beta. Things seem to be working in some quick testing, and if anyone has complained about things breaking I haven't heard it.

Looks like someone copied from T174569 and didn't update everything. There's no merged patch here to have been auto-deployed or tested, nor does the enwiki DB on Beta Cluster show the changes proposed here.

Change 537256 had a related patch set uploaded (by Rxy; owner: Rxy):
[mediawiki/extensions/CheckUser@master] Schema change: Add columns cuc_log_id and ActorMigrate, CommentStore related

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

Once the patch is merged and this ticket is ready to go, please re-add  DBA and Blocked-on-schema-change tags
Thanks!

@kchapman - This was being worked on by a volunteer a year ago, but never made it through code review and is now stalled. At the very least we are going to need CheckUser to use the actor table in order to move forward with IP masking. Is this something that Core Platform could pick up and carry across the finish line?

I don't think Kate is the right person these days.

If you want Platform, I guess you might want to ask Will, Mandy or possibly Naïké.

AHT could potentially be better placed with currently/recently working on CU...

@WDoranWMF @AMooney - This was being worked on by a volunteer a year ago, but never made it through code review and is now stalled. At the very least we are going to need CheckUser to use the actor table in order to move forward with IP masking. Is this something that Core Platform could pick up and carry across the finish line?