Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert Unique to PK for compatibility with Percona XtraDB #20934

Conversation

bklang
Copy link

@bklang bklang commented May 11, 2020

This PR makes two changes:

  1. Modifies an old migration changing a unique ID on the collres_accesscache table to a Primary key
  2. Creates a new migration for installations which have already run the previous migration to sync them up

I realize that it is an unusual step to modify an old migration, but in this case it is necessary. NextCloud upgrades are failing with Percona XtraDB due to a missing primary key on this table. Because the breakage occurs in an existing migration, the fix must also occur in the existing migration. To ensure that we do not cause any existing installations to diverge, this commit also introduces a new pair of migrations that looks for the non-PK version of the table and updates those as well. See #16311 for additional details. I did attempt to do the unique -> primary key conversion in a single step, but I kept running into errors. I was able to test the 2-step migration successfully to convert a NextCloud 18 database.

Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The indentation is off in the migration files. We use tabs - could you check to unify them in those files? Except that it seems to be fine to run those migrations. They might be expensive, but should be okay. Or do we just want to drop them during the migration and then add them to the "add-missing-indices" command? cc @nickvergessen @rullzer

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews bug labels May 12, 2020
@MorrisJobke MorrisJobke added this to the Nextcloud 20 milestone May 12, 2020
@bklang bklang force-pushed the fix/collres_accesscache_percona_xtradb_compat branch from eaf0d1d to 52b2266 Compare May 12, 2020 12:07
@bklang
Copy link
Author

bklang commented May 12, 2020

Whitespace issues addressed, thanks for the feedback

This PR makes two changes:
1. Modifies an old migration changing a unique ID on the `collres_accesscache` table to a Primary key
2. Creates a new migration for installations which have already run the previous migration to sync them up

I realize that it is an unusual step to modify an old migration, but in this case it is necessary. [NextCloud upgrades are failing with Percona XtraDB](nextcloud#16311) due to a missing primary key on this table. Because the breakage occurs in an existing migration, the fix must also occur in the existing migration. To ensure that we do not cause any existing installations to diverge, this commit also introduces a new migration that looks for the non-PK version of the table and updates those as well. See nextcloud#16311 for additional details.

Signed-off-by: Ben Klang <bklang@powerhrg.com>
@bklang bklang force-pushed the fix/collres_accesscache_percona_xtradb_compat branch from 52b2266 to 8bafd22 Compare May 12, 2020 12:10
@bklang bklang requested a review from MorrisJobke May 12, 2020 14:55
@nickvergessen
Copy link
Member

Yeah well we can't really add them lazy, as requiring the uniqueness is an important part

@apircalabu
Copy link

Just found this issue after looking into a stock 18.0.4 install with the following apps installed:
sudo -u apache php occ app:list
Enabled:

  • accessibility: 1.4.0
  • activity: 2.11.0
  • apporder: 0.9.0
  • bruteforcesettings: 1.6.0
  • calendar: 2.0.3
  • cloud_federation_api: 1.1.0
  • comments: 1.8.0
  • contacts: 3.3.0
  • dav: 1.14.0
  • deck: 1.0.0
  • federatedfilesharing: 1.8.0
  • federation: 1.8.0
  • files: 1.13.1
  • files_pdfviewer: 1.7.0
  • files_rightclick: 0.15.2
  • files_sharing: 1.10.1
  • files_trashbin: 1.8.0
  • files_versions: 1.11.0
  • files_videoplayer: 1.7.0
  • firstrunwizard: 2.7.0
  • groupfolders: 6.0.6
  • logreader: 2.3.0
  • lookup_server_connector: 1.6.0
  • mail: 1.3.4
  • nextcloud_announcements: 1.7.0
  • notes: 3.3.0
  • notifications: 2.6.0
  • oauth2: 1.6.0
  • onlyoffice: 4.1.4
  • password_policy: 1.8.0
  • photos: 1.0.0
  • privacy: 1.2.0
  • provisioning_api: 1.8.0
  • recommendations: 0.6.0
  • serverinfo: 1.8.0
  • settings: 1.0.0
  • sharebymail: 1.8.0
  • support: 1.1.0
  • survey_client: 1.6.0
  • systemtags: 1.8.0
  • tasks: 0.13.0
  • text: 2.0.0
  • theming: 1.9.0
  • twofactor_backupcodes: 1.7.0
  • updatenotification: 1.8.0
  • user_external: 0.9.1
  • viewer: 1.2.0
  • workflowengine: 2.0.0
    Disabled:
  • admin_audit
  • documentserver_community
  • encryption
  • files_external
  • spreed
  • user_ldap

Here are the tables without primary key:

mysql> select tab.table_schema as database_name, tab.table_name from information_schema.tables tab left join information_schema.table_constraints tco on tab.table_schema = tco.table_schema and tab.table_name = tco.table_name and tco.constraint_type = 'PRIMARY KEY' where tco.constraint_type is null and tab.table_schema not in('mysql', 'information_schema', 'performance_schema', 'sys') and tab.table_type = 'BASE TABLE' order by tab.table_schema, tab.table_name;
+---------------+-----------------------------+
| database_name | table_name |
+---------------+-----------------------------+
| nextcloud | oc_collres_resources |
| nextcloud | oc_comments_read_markers |
| nextcloud | oc_federated_reshares |
| nextcloud | oc_filecache_extended |
| nextcloud | oc_notifications_pushtokens |
| nextcloud | oc_systemtag_object_mapping |
+---------------+-----------------------------+
6 rows in set (0.09 sec)

@MorrisJobke
Copy link
Member

This will be done as part of 838b02c

Thanks for your contribution anyways and sorry that we didn't picked it up earlier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants