Page MenuHomePhabricator

Add primary key to pagetriage_page_tags
Closed, ResolvedPublic

Description

pagetriage_page_tags lacks a primary key

CREATE TABLE `pagetriage_page_tags` (
  `ptrpt_page_id` int(10) unsigned NOT NULL,
  `ptrpt_tag_id` int(10) unsigned NOT NULL,
  `ptrpt_value` varbinary(255) NOT NULL,
  UNIQUE KEY `ptrpt_page_tag_id` (`ptrpt_page_id`,`ptrpt_tag_id`),
  KEY `ptrpt_tag_id_value` (`ptrpt_tag_id`,`ptrpt_value`)
) ENGINE=InnoDB DEFAULT CHARSET=binary

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 17 2020, 2:08 PM

Can ptrpt_page_tag_id be converted from a UNIQUE KEY to a primary key?

Can ptrpt_page_tag_id be converted from a UNIQUE KEY to a primary key?

Yes, it can be done by adding the PK and dropping the UNIQUE KEY. But it still requires the usual procedure to change tables.sql to reflect that in the repo and so on.

Restricted Application added a project: User-DannyS712. · View Herald TranscriptJan 27 2020, 3:44 AM

Change 567390 had a related patch set uploaded (by DannyS712; owner: DannyS712):
[mediawiki/extensions/PageTriage@master] Convert ptrpt_page_tag_id from unique index to primary key

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

DannyS712 added a comment.EditedJan 27 2020, 4:13 AM

@Marostegui would you mind taking a look at the patch? I don't understand why Jenkins says Can't DROP 'ptrpt_page_tag_id'; check that column/key exists

@Marostegui would you mind taking a look at the patch? I don't understand why Jenkins says Can't DROP 'ptrpt_page_tag_id'; check that column/key exists

I am not familiar with the schema change procedure from MW's patches point of view, so I am not really sure if you need to change something from the patch side.
What I can see on jenkis is:

04:11:45 Traceback (most recent call last):
04:11:45   File "/usr/local/bin/quibble", line 11, in <module>
04:11:45     load_entry_point('quibble==0.0.40', 'console_scripts', 'quibble')()
04:11:45   File "/usr/local/lib/python3.5/dist-packages/quibble/cmd.py", line 467, in main
04:11:45     cmd.execute(plan)
04:11:45   File "/usr/local/lib/python3.5/dist-packages/quibble/cmd.py", line 289, in execute
04:11:45     command.execute()
04:11:45   File "/usr/local/lib/python3.5/dist-packages/quibble/commands.py", line 465, in execute
04:11:45     mwdir=self.mw_install_path
04:11:45   File "/usr/local/lib/python3.5/dist-packages/quibble/mediawiki/maintenance.py", line 63, in install
04:11:45     'Install failed with exit code: %s' % p.returncode)
04:11:45 Exception: Install failed with exit code: 1
04:11:45 INFO:backend.MySQL:Terminating MySQL

From what I can see: https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/PageTriage/+/567390/4/sql/PageTriagePageTagsPatch-pk.sql - the syntax is correct and makes the expected result on the current table definition (at least the one we have in production).

Not sure if someone more familiar with the schema change patch procedure from MW side can help you out here, maybe @Anomie @Ladsgroup or @Reedy (who have recently submitted patches) could help you.

Forgot to also paste:

04:11:24 Error: 1091 Can't DROP 'ptrpt_page_tag_id'; check that column/key exists (localhost:/workspace/db/quibble-mysql-fry1yr9q/socket)

@Marostegui: Our CI generally doesn't test the actual schema changes, as that would need an "old" schema provided to upgrade from. So it's creating the table with the new schema that already lacks the key, then fails when it tries to remove it because the schema change wasn't registered with a correct check to see whether it's needed.

@Marostegui: Our CI generally doesn't test the actual schema changes, as that would need an "old" schema provided to upgrade from. So it's creating the table with the new schema that already lacks the key, then fails when it tries to remove it because the schema change wasn't registered with a correct check to see whether it's needed.

Solved with DROP INDEX IF EXISTS, but then there were issues with adding a primary key when one already exists
I couldn't figure out how to check if the primary key already exists, so: only apply the patch when the old index exists

@Marostegui: Our CI generally doesn't test the actual schema changes, as that would need an "old" schema provided to upgrade from. So it's creating the table with the new schema that already lacks the key, then fails when it tries to remove it because the schema change wasn't registered with a correct check to see whether it's needed.

Solved with DROP INDEX IF EXISTS, but then there were issues with adding a primary key when one already exists
I couldn't figure out how to check if the primary key already exists, so: only apply the patch when the old index exists

reedy@deploy1001:~$ mwscript eval.php enwiki
> $db = wfGetDb( DB_REPLICA );

> var_dump( $db->indexExists( 'interwiki', 'PRIMARY' ) );
bool(true)

Change 567390 merged by jenkins-bot:
[mediawiki/extensions/PageTriage@master] Convert ptrpt_page_tag_id from unique index to primary key

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

DannyS712 closed this task as Resolved.Jan 28 2020, 12:38 AM
DannyS712 removed a project: Patch-For-Review.

Task for schema change for wmf wikis: T243804: Apply: Add primary key to pagetriage_page_tags

@Marostegui: Our CI generally doesn't test the actual schema changes, as that would need an "old" schema provided to upgrade from. So it's creating the table with the new schema that already lacks the key, then fails when it tries to remove it because the schema change wasn't registered with a correct check to see whether it's needed.

Thanks for taking the time to explain that. Much appreciated as always!

Aklapper removed a subscriber: Anomie.Oct 16 2020, 5:38 PM