[14.0][IMP] openupgrade_framework: remove FKs of obsolete tables/columns

[MiquelRForgeFlow]

It was time.

[MiquelRForgeFlow]

I will backport it to some previous versions.

[StefanRijnhart]

Isn't this more appropriate in https://github.com/OCA/server-tools/tree/13.0/database_cleanup? (or well, its later ports that are still pending)

[MiquelRForgeFlow]

I see database_cleanup more to delete columns/tables instead of FKs. And not everybody uses that module. I mean, OU should not depend of the existence of database_cleanup.

[pedrobaeza]

Indeed, database_cleanup is an optional tool for cleaning obsolete data, and it directly removes the SQL column/table, thus automatically lifting constraints.

But if you don't want to perform such cleaning yet, or you are doing data manipulation in the progress of the migration, there's a chance that these columns/tables interfere with it. Lifting these constraints allows to keep data, but not requiring extra controls/actions on your migration scripts or on normal activity after migration.

My only doubt right now is if the field is re-added by a third-party module, if the constraints are restored by the ORM.

As a related note, Odoo enterprise migration deletes directly the columns/tables (like performing database_cleanup) when you get the migrated DB.

[pedrobaeza]

Maybe you can mention on the message that only removing FKs, but it's optional.

[pedrobaeza]

@MiquelRForgeFlow let's finish this putting that message.

[StefanRijnhart]

No, I first created that module in order to clean up after migration, to prevent having do that in OpenUpgrade, especially because it would have to be an unsupervised process in OpenUpgrade whereas it is supervised in database_cleanup.
Here too, the lack of supervision is a problem. It might just be harmful to drop FK constraints.
What is your actual use case? I usually run my migrated databases for months before apply database_cleanup, and I never ran into problems with FK constraints related to obsolete data models.

[pedrobaeza]

Why is harmful? Please expose your use case to say that is harmful. I will prepare cases where you find the problem. I have right now in mind when trying to remove in v9 accounts of type view (it was work-arounded other way).

[MiquelRForgeFlow]

@sbidoul could you exclude this repo from the runboat? thanks

[StefanRijnhart]

I'm most concerned about the loss of information and losing referential integrity. We keep these tables and columns for a reason.

[legalsylvain]

Hi,

At first sight, I don't see why remove the foreign keys, so I agree with stefan to keep as much obsolete data / FK / etc. as possible, while waiting for the use of database_cleanup.

[sbidoul]

@sbidoul could you exclude this repo from the runboat? thanks

done

[legalsylvain]

/ocabot rebase
(to use up to date CI).

Moreover, I'm in favor to close this PR.
Clean up should be done in the according module. (except if there is case when the migration is broken)

[OCA-git-bot]

@legalsylvain The rebase process failed, because command git push --force ForgeFlow tmp-pr-2989:14.0-fix-openupgrade_framework-drop-relation-constrains failed with output:

remote: Permission to ForgeFlow/OpenUpgrade.git denied to OCA-git-bot.
fatal: unable to access 'https://github.com/ForgeFlow/OpenUpgrade/': The requested URL returned error: 403

[pedrobaeza]

One example of a problem due to old foreign keys is:

OpenUpgrade/addons/account/migrations/9.0.1.1/post-migration.py

Line 308 in 30b6c22

DROP CONSTRAINT account_account_parent_id_fkey"""

Although this one wouldn't be bypassed with this extra code, as the cleanup is done at the end.

But I would say that the main reason is the performance: having FK with ondelete=cascade or ondelete=restrict for tables that are no longer used is draining the performance on each delete operation, but also the referential integrity is draining it on each update.

You may want to keep the tables during some time after the migration, but you don't want them to affect your performance.

[legalsylvain]

TLDR :

• 👍 for the code, nice improvment
• 👍 to move this code into database_cleanup.

But I would say that the main reason is the performance: having FK with ondelete=cascade or ondelete=restrict for tables that are no longer used is draining the performance on each delete operation, but also the referential integrity is draining it on each update.
You may want to keep the tables during some time after the migration, but you don't want them to affect your performance.

That's the main point.

In my opinion :

Here is the perimeter of openupgrade as I understand it :

• Prevent the "update all" failing (of course)
• Do not lose data (of course also)
• Keep old data, so that, if necessary, custom operations can be performed after the migration. (conservative attitude)
• have an optimized and fast migration process. (for that reason, we create SQL request to replace slow computation of computed fields by the ORM)

this is what is outside the scope of openupgrade :

• improve performance of the instance, when running it in production after.

1. The current documentation says so. (https://oca.github.io/OpenUpgrade/after_migration.html) mentions database_cleanup module, and postgresql operation like vaccum.)
2. If the code is out of the openupgrade project, it can be used at other times, not only during an upgrade. (in our current use case, clean obsolete FK can be required out of a openupgrade process)
3. the code is maintained by more people, if it is in a standard module, like database_cleanup. (the openupgrade project is maintained by a small team of motivated people, but we are few!) the strategy I promote is to reduce the code base to be maintained as much as possible.

Indeed, database_cleanup is an optional tool for cleaning obsolete data,

i don't think it's optional. When I explain how openugprade works, I always mention the fact of executing this module.

As a related note, Odoo enterprise migration deletes directly the columns/tables (like performing database_cleanup) when you get the migrated DB.

I see it less modular.

@pedrobaeza : what's wrong with doing openupgrade + database_cleanup ?

[pedrobaeza]

You can't move this outside of OpenUpgrade because you don't know which are the FKs that can be really deleted. It's a post-mortem analysis where not everything can be deduced.

And I don't agree on your assessment about not having to care about performance after running OpenUpgrade. One of the main goals of OpenUpgrade is to adapt database schema and information to the state expected by the new version. If we don't remove extra DB schema things, like obsolete FK constraints (the same we have done manually in some occasions), we are not getting the same DB schema as if we create the DB from scratch in this new version. Preserving old data is not the same as preserving old DB scheme.

[legalsylvain]

You can't move this outside of OpenUpgrade because you don't know which are the FKs that can be really deleted. It's a post-mortem analysis where not everything can be deduced.

I have to think about that point. I thought that it was possible, to deduce valid and invalid foreign keys, based on the code. if it's not the case, I'll have to reconsider my PoV on that topic.

And I don't agree on your assessment about not having to care about performance after running OpenUpgrade. One of the main goals of OpenUpgrade is to adapt database schema and information to the state expected by the new version. If we don't remove extra DB schema things, like obsolete FK constraints (the same we have done manually in some occasions), we are not getting the same DB schema as if we create the DB from scratch in this new version. Preserving old data is not the same as preserving old DB scheme.

Well, it's not "my" assessment. it's just how openupgrade and database_cleanup are designed. For the time being,

• vaccum are not integrated in openupgrade, so it certainly reduces the performance
• old legacy fields and table remains after an upgrade, so the schema of a migrated database is not the same as a database created from scratch

If you want to promote an "all-in-one" openupgrade tools so that the database is optimized to work in production and make the DB scheme after upgrade the same as a fresh DB, we have a lot of things to do, not only that point. Is it what you want ? If yes, we can open a distinct issue to talk about that point.

[pedrobaeza]

vaccum are not integrated in openupgrade, so it certainly reduces the performance

This depends on the way you perform the migration. A dump + restore already VACUUM the DB. And you need to do the same VACUUM on the same Odoo version regularly. It's a DB maintenance operation, not OpenUpgrade nor database_cleanup operation.

old legacy fields and table remains after an upgrade, so the schema of a migrated database is not the same as a database created from scratch

About schema I refer the rest of the things. It's clear that if we want to preserve old data, we have to preserve the part of the schema that supports this old data, which are tables/columns, but not the rest of the artifacts around them.

If you want to promote an "all-in-one" openupgrade tools so that the database is optimized to work in production and make the DB scheme after upgrade the same as a fresh DB, we have a lot of things to do, not only that point. Is it what you want ? If yes, we can open a distinct issue to talk about that point.

No and yes. I want to do in OpenUpgrade what is the most suitable to be done and to not force to install and run a foreigner thing for finishing what I consider part of a good migration: old things not affecting the behavior of the new version. And this includes both constraints that blocks proper operations (we remove these constraints manually in migration scripts when detected), and the rest of the deprecated constraints that don't block the operation, but impact them on the speed to perform them.