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

fix: added type to createConnectionParams, removed safeguard [restore db conversions] #48666

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Delogon
Copy link

@Delogon Delogon commented Oct 11, 2024

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

Checklist

@WhiskyDelta
Copy link

/backport to stable30

@WhiskyDelta
Copy link

/backport to stable29

@Delogon
Copy link
Author

Delogon commented Oct 12, 2024

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

@joshtrichards
Copy link
Member

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
Copy link
Member

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

Copy link
Contributor

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.)

@susnux susnux requested a review from icewind1991 October 28, 2024 11:02
@joshtrichards joshtrichards changed the title fix: added type to createConnectionParams, removed safeguard fix: added type to createConnectionParams, removed safeguard [restore db conversions] Oct 28, 2024
@WhiskyDelta
Copy link

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

@SnejPro
Copy link

SnejPro commented Dec 13, 2024

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
Copy link

SnejPro commented Dec 13, 2024

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

@kesselb kesselb requested review from kesselb and removed request for kesselb December 13, 2024 21:57
@FlorianFritz
Copy link

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?

@karlsandreas
Copy link

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.

@HexagonCoder
Copy link

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!

@xalt7x
Copy link

xalt7x commented Dec 31, 2024

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...

Nils Wenninghoff and others added 2 commits December 31, 2024 14:50
@nullabsolut
Copy link

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 #45257 or create a new issue entirely?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: occ db:convert-type broken since PrimaryReadReplicaConnection support added