server icon indicating copy to clipboard operation
server copied to clipboard

fix: added type to createConnectionParams, removed safeguard

Open Delogon opened this issue 1 year ago • 3 comments

  • Resolves: #45257

Summary

This should fix the db conversion bug. $type was added as a parameter to createConnectionParams in the ConnectionFactory.php. There is also a check if the type value is set if not the default is loaded from the config. There is also an additional type check to make sure mysql.utf8mb4 is only used with mysql. The parameters of createConnectionParams in Connection Factory has also been updated, and the safeguard has been removed. The solution was provided in the issue comments by @Queuecumber, @normen, @lightonflux and me.

TODO

  • [ ] An error occurs when using group folders because the table 'oc_group_folders_acl' is missing in MariaDB see #45257

Checklist

Delogon avatar Oct 11 '24 22:10 Delogon

/backport to stable30

WhiskyDelta avatar Oct 12 '24 10:10 WhiskyDelta

/backport to stable29

WhiskyDelta avatar Oct 12 '24 10:10 WhiskyDelta

@juliushaertl @joshtrichards cloud you review this? Or do you know who could?

Delogon avatar Oct 12 '24 19:10 Delogon

This is a proposed alternative to #46908 (not yet reviewed - just noting since that one seemed to have blockers which this will need to make sure it's addressing).

joshtrichards avatar Oct 25 '24 23:10 joshtrichards

@Delogon Please stop rebasing / re-merging with master. It's unnecessary at this juncture and just keeps resetting all the tests. :)

joshtrichards avatar Oct 26 '24 00:10 joshtrichards

Hello there, Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

github-actions[bot] avatar Oct 26 '24 01:10 github-actions[bot]

Is this going to be fixed or should we maybe correct the documentation and remove this feature? Converting Database Type - Nextcloud documentation

WhiskyDelta avatar Dec 08 '24 12:12 WhiskyDelta

Is this going to be fixed or should we maybe correct the documentation and remove this feature? Converting Database Type - Nextcloud documentation

I think removing this feature is a very bad option. The officially suggested method to deploy Nextcloud is with AIO right now. But for the migration it is mandatory to convert the database from mysql to postgres. So if this feature is removed in the future, it should be announced a certain time in advance to give users the opportunity to migrate. I know the issue is already there, but for now i do not see another option than solving it.

But there should be an information in the documentation that this feature is currently broken and therefore disabled.

SnejPro avatar Dec 13 '24 13:12 SnejPro

I used this PR to migrate my installation from mariadb to postgres a few hours ago. For now everything works fine.

SnejPro avatar Dec 13 '24 20:12 SnejPro

Hi, I tried to convert my mariadb to pgsql with this code today. It failed with following exceptions when converting the "oc_accounts" table

In ExceptionConverter.php line 62:

[Doctrine\DBAL\Exception\UniqueConstraintViolationException (1062)]
An exception occurred while executing a query: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0f97f6aa-3975-4b81-935f-de4a1bac7a2d' for ke
y 'PRIMARY'

In Exception.php line 28:

[Doctrine\DBAL\Driver\PDO\Exception (1062)]
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0f97f6aa-3975-4b81-935f-de4a1bac7a2d' for key 'PRIMARY'

In Statement.php line 130:

[PDOException (23000)]
SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '0f97f6aa-3975-4b81-935f-de4a1bac7a2d' for key 'PRIMARY'

When I check my mysql database there is only one occurance of the uid.

MariaDB [nextcloud]> SELECT * FROM oc_accounts; [...] | 0f97f6aa-3975-4b81-935f-de4a1bac7a2d | {"displayname":{"value":"nextcloud","scope":"v2-federated", [...] [...]

So '0f97f6aa-3975-4b81-935f-de4a1bac7a2d' is the 'uid' of the "nextcloud" user.

When I count this 'uid' I only get one result (as expected)

MariaDB [nextcloud]> SELECT count() FROM oc_accounts WHERE uid="0f97f6aa-3975-4b81-935f-de4a1bac7a2d"; +----------+ | count() | +----------+ | 1 | +----------+ 1 row in set (0.000 sec)

Is this related or should I open a new issue?

FlorianFritz avatar Dec 22 '24 09:12 FlorianFritz

For those in need to convert their db. I used this PR on 30.0.2 to migrate from MySQL to PostgreSQL without any problems.

karlsandreas avatar Dec 28 '24 08:12 karlsandreas

Can someone please review and fix this so it can be merged? Bug #45257 has been in need of fixing for many months. Earlier this year I needed to convert a NC DB from MariaDB to Postgres and had to put in a lot of extra time and effort to work around this bug, and now, when I am converting the DB of another NC instance, I am shocked to find that it still hasn't been fixed.

Please, this is an important feature. Can someone take care of it soon?

I don't want to sound demanding or ungrateful, I really appreciate everyone's time they put into developing FOSS, I just want to stress how important this feature is and how frustrating it is to users that the bug still hasn't been fixed.

Thank you and happy new year everyone!

HexagonCoder avatar Dec 28 '24 16:12 HexagonCoder

I've tried mySQL > PostgreSQL conversion on the freshly installed Nextcloud instance

Environment: Debian 12 Mariadb 10.11.6-MariaDB-0+deb12u1 (Debian package) PostgreSQL 15+248 (Debian package), also tried PostgreSQL 17 Nextcloud v30.0.4 + patches from this PR

sudo -u www-data php /var/www/nextcloud/occ db:convert-type --all-apps --password="${old_dbpassword}" pgsql oc_nextcloud 127.0.0.1 nextcloud_database
  1. If the installation of “recommended” apps was skipped, the database converts succesfully!
  2. If the "Talk" app was previously installed - script informs

The following tables will not be converted: oc_talk_commands Continue with the conversion (y/n)?

I've tried different options with this: a. If I agree to continue (y) conversion proceeds without visible errors and server starts with PostgreSQL b. If I disable Talk app, remove it and restart server script still complains c. If I remove app and try to drop oc_talk_commands table it breaks conversion

  • oc_talk_rooms 0/2 [>---------------------------] 0% < 1 sec/< 1 sec In ExceptionConverter.php line 68:

    An exception occurred while executing a query: SQLSTATE[42703]: Undefined column: 7 ERROR:
    column "last_activity" of relation "oc_talk_rooms" does not exist
    LINE 1: ...pe", "password", "active_since", "active_guests", "last_acti...

If after that I try to drop oc_talk_rooms - conversion breaks due to duplicated keys

An exception occurred while executing a query: SQLSTATE[23505]: Unique violation: 7 ERROR: duplicate key value violates unique c
onstraint "oc_storages_pkey"
DETAIL: Key (numeric_id)=(1) already exists.

d. If prior to running script I remove Talk app and drop both oc_talk_commands and oc_talk_rooms tables conversion proceeds without visible errors and server starts with PostgreSQL.


So I guess, it can be re-enabled for instances without apps. As for the "Talk" and other apps, I'm not entirely sure...

xalt7x avatar Dec 31 '24 09:12 xalt7x

Unsuccessfully tried migration from MariaDB to PostgreSQL on an actively used NC instance with Talk installed, with this PR, for the same reason as @xalt7x .

Environment: Ubuntu 24.04.1 LTS Mariadb 11.4.4-MariaDB (Mariadb repo) (utf8mb4) PostgreSQL 16.6 (Ubuntu 16.6-0ubuntu0.24.04.1) (UTF8) Nextcloud v30.0.4 + patches from this PR

Installed Apps
Enabled:
- activity: 3.0.0
- app_api: 4.0.3
- bookmarks: 15.0.5
- calendar: 5.0.9
- circles: 30.0.0
- cloud_federation_api: 1.13.0
- comments: 1.20.1
- contacts: 6.1.3
- contactsinteraction: 1.11.0
- dashboard: 7.10.0
- dav: 1.31.1
- deck: 1.14.3
- federatedfilesharing: 1.20.0
- federation: 1.20.0
- files: 2.2.0
- files_downloadlimit: 3.0.0
- files_pdfviewer: 3.0.0
- files_reminders: 1.3.0
- files_sharing: 1.22.0
- files_trashbin: 1.20.1
- files_versions: 1.23.0
- firstrunwizard: 3.0.0
- logreader: 3.0.0
- lookup_server_connector: 1.18.0
- nextcloud_announcements: 2.0.0
- notes: 4.11.0
- notifications: 3.0.0
- notify_push: 1.0.0
- oauth2: 1.18.1
- password_policy: 2.0.0
- photos: 3.0.2
- privacy: 2.0.0
- provisioning_api: 1.20.0
- recommendations: 3.0.0
- related_resources: 1.5.0
- serverinfo: 2.0.0
- settings: 1.13.0
- sharebymail: 1.20.0
- spreed: 20.1.3
- support: 2.0.0
- systemtags: 1.20.0
- tasks: 0.16.1
- text: 4.1.0
- theming: 2.5.0
- twofactor_backupcodes: 1.19.0
- twofactor_totp: 12.0.0-dev
- updatenotification: 1.20.0
- user_status: 1.10.0
- viewer: 3.0.0
- weather_status: 1.10.0
- webhook_listeners: 1.1.0-dev
- workflowengine: 2.12.0
Disabled:
- admin_audit: 1.20.0
- bruteforcesettings: 3.0.0 (installed 2.0.1)
- encryption: 2.18.0 (installed 2.18.0)
- files_external: 1.22.0
- survey_client: 2.0.0 (installed 1.9.0)
- suspicious_login: 8.0.0
- twofactor_admin: 4.7.1 (installed 4.7.1)
- twofactor_nextcloud_notification: 4.0.0
- user_ldap: 1.21.0

On first conversion try ran into this issue, but managed to fix with the solution mentioned in the thread: Update to 31.0.0.7 failed - reason: theming app - SQL issue with duplicate entry (1062) (delete from oc_preferences where userid = "<username from error message>" and appid = "theming" and configkey = "primary_color";) I did not find an issue for this btw, did I overlook it or does this need its own issue?

Then started conversion again with a fresh database and ran into the same issue with oc_talk_rooms.

  • oc_talk_rooms 0/11 [>---------------------------] 0% < 1 sec/< 1 sec
    In ExceptionConverter.php line 68: An exception occurred while executing a query: SQLSTATE[42703]: Undefined column: 7 ERROR: column "last_activity" of relation "oc_talk_rooms" does not exist LINE 1: ...pe", "password", "active_since", "active_guests", "last_acti... ^ In Exception.php line 28: SQLSTATE[42703]: Undefined column: 7 ERROR: column "last_activity" of relation "oc_talk_rooms" does not exist LINE 1: ...pe", "password", "active_since", "active_guests", "last_acti... ^ In Statement.php line 130: SQLSTATE[42703]: Undefined column: 7 ERROR: column "last_activity" of relation "oc_talk_rooms" does not exist LINE 1: ...pe", "password", "active_since", "active_guests", "last_acti... ^
Converted apps/tables
Creating schema in new database
 - dav
 - user_ldap
 - twofactor_nextcloud_notification
 - nextcloud_announcements
 - files_versions
 - user_status
 - dashboard
 - logreader
 - recommendations
 - app_api
 - workflowengine
 - serverinfo
 - federatedfilesharing
 - oauth2
 - tasks
 - theming
 - provisioning_api
 - suspicious_login
 - files_external
 - twofactor_admin
 - activity
 - support
 - weather_status
 - text
 - circles
 - admin_audit
 - viewer
 - contactsinteraction
 - webhook_listeners
 - sharebymail
 - contacts
 - updatenotification
 - files_pdfviewer
 - notify_push
 - files_reminders
 - calendar
 - notes
 - notifications
 - survey_client
 - federation
 - encryption
 - lookup_server_connector
 - bookmarks
 - firstrunwizard
 - privacy
 - twofactor_totp
 - files_trashbin
 - cloud_federation_api
 - systemtags
 - spreed
 - photos
 - password_policy
 - bruteforcesettings
 - deck
 - files
 - files_sharing
 - twofactor_backupcodes
 - related_resources
 - settings
 - files_downloadlimit
 - comments
The following tables will not be converted:
oc_file_metadata
oc_memories
oc_memories_livephoto
oc_memories_mapclusters
oc_memories_places
oc_memories_planet
oc_news_feeds
oc_news_folders
oc_news_items
oc_passman_credentials
oc_passman_delete_vault_request
oc_passman_files
oc_passman_revisions
oc_passman_share_request
oc_passman_sharing_acl
oc_passman_vaults
oc_passwords_challenge
oc_passwords_entity_challenge
oc_passwords_entity_folder
oc_passwords_entity_folder_revision
oc_passwords_entity_keychain
oc_passwords_entity_password
oc_passwords_entity_password_revision
oc_passwords_entity_registration
oc_passwords_entity_session
oc_passwords_entity_share
oc_passwords_entity_tag
oc_passwords_entity_tag_revision
oc_passwords_folder
oc_passwords_folder_rv
oc_passwords_keychain
oc_passwords_password
oc_passwords_password_rv
oc_passwords_pw_tag_rel
oc_passwords_registration
oc_passwords_relation_password_tag
oc_passwords_session
oc_passwords_share
oc_passwords_tag
oc_passwords_tag_rv
oc_talk_commands
Continue with the conversion (y/n)? [n] y

Is this oc_talk_rooms error still relevant to this PR, as the PR appears to fix something entirely different? Should I rather add this info to https://github.com/nextcloud/server/issues/45257 or create a new issue entirely?

nullabsolut avatar Jan 20 '25 22:01 nullabsolut

Thanks for all the work! I created a PR in the org repo in order for all tests to pass: https://github.com/nextcloud/server/pull/50530. I also fixed another issue I found while testing this PR (which wasn't caused by these changes though).

provokateurin avatar Jan 29 '25 11:01 provokateurin