-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
[15.0][FIX]database_cleanup: endless loop #2682
base: 15.0
Are you sure you want to change the base?
Conversation
9ea0b3e
to
76b5ecb
Compare
@@ -72,7 +72,9 @@ def purge(self): | |||
) | |||
|
|||
self.logger.info("Dropping table %s", line.name) | |||
self.env.cr.execute("DROP TABLE %s", (IdentifierAdapter(line.name),)) | |||
self.env.cr.execute( | |||
"DROP TABLE %s CASCADE", (IdentifierAdapter(line.name),) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks dangerous and I would never knowingly run this on a production database. Do you think it would work to drop all constraints for all tables in the line
loop in a first pass and then drop the tables in a second pass?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right :) I tried your proposition and it works, at least in my case. Updating the PR to this less dangerous solution ;) Thank you !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What used to be the case is that constraints between tables that were going to be dropped were removed explicitely. That can be considered safe, because the related tables were going to be dropped anyway. What happens now in the suggested change is that constraints from a table that is not going to be dropped to a table that is going to be dropped are being dropped explicitely.
I would not consider this safe, because the existence of such a constraint is an indication that it is not safe to drop the table in the first place. It also breaks dropping tables that still (only) depend on each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what would the safe solution be? Provide a bugfix just for the endless loop, but fail to drop the table?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that
- The current code in this PR is unsafe,
- My original suggestion is very much what is already implemented in the released version of the module,
- The issue occurs when dealing with a manually created
x_
model according to the traceback,
I would make an exception only when dealing with a manually created model (c.q. table) that starts with x_
I had a case where database_cleanup looped endlessly because of referential integrity. DROP TABLE was failing because some tables had foreign keys on tables which were not in the list of tables to delete. Dropping all contraints linked to the table, even if they are on table which won't be dropped seems to solve the issue.
76b5ecb
to
c8d1695
Compare
We have some cases where database_cleanup loop endlessly because of referential integrity.
In my case, the issue was caused by tables having foreign keys on tables which are not in the list of tables to delete. Updating the code to drop all constraints, even on tables which are not meant to be dropped, seems to solve the issue
[OUTDATED]
Here for example, DROP TABLE is failing because some tables have foreign keys on a studio table, and the error message suggest using DROP TABLE ... CASCADE, which seems indeed to fix the problem.
First introduced by damdam-s#1 in #2219
But lost when this PR has been superseded by #2547