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

[6.x] Revert cascading truncations #34527

Conversation

blakethepatton
Copy link
Contributor

This PR reverses #26389 which introduces cascading truncations in postgres. The spirit of the original fix was to address a constraint of postgres in that disabling foreign key constrains does not allow a developer to a truncate a table that has foreign keys. However, the proposed and accepted solution results in all tables that have a column that references the table under question for truncation to be also truncated and further any tables that referenced the children of the first table. This can and has created a problematic situation when you discover that truncating a small table results in the loss of your entire database.

As that is not expected behavior for truncation of a table, I would propose that the word cascading be removed from truncation and that documentation be created in the laravel docs indicating that this was the behavior in postgres between version [initial release version] and [this release version].

Issue: #29506

As @crazyscience brought up yesterday:

This code causes EVERY table to be truncated that has a related constraint to the one referenced in DB::truncate().
So, if you follow a typical design pattern where all of your tables are constrained with foreign keys, DB::truncate('anything') results in large-scale data destruction. That's super dangerous and isn't expected behavior when someone is doing DB::truncate('just-one-table').

https://www.postgresql.org/docs/9.1/sql-truncate.html

CASCADE

Automatically truncate all tables that have foreign-key references to any of the named tables, or to any tables added to the group due to CASCADE.

As a note, this behavior OVERRIDES any ON DELETE that you may have set in the database.

This was merged yesterday in #34496 and then reverted without explanation in #34497. If this is supposed to be expected behavior I would urge that the docs be updated to reflect that in postgres, truncating a table will use a cascading truncate meaning that any related tables will be truncated without warning. I would also suggest that the cascading behavior be added to other sql truncate grammars so that truncations are at least consistent between sql languages and can provide that pull request.

If requested, I can create an example environment for demonstrating the behavior.

@crazyscience
Copy link

I would say that if we really need to have support for cascading truncations, we should add a method called truncateCascading() that fills that need with a method name that adequately describes the action the method will invoke. This will leave it to the developer to decide if they want to truncate just the single table or if they'd like to truncate all related and descendant tables.

@taylorotwell
Copy link
Member

I don't think we should revert 2 year old PRs on a current release.

@blakethepatton
Copy link
Contributor Author

So should I title it differently then? Should I introduce this on a different branch? That gives very little guidance on how to proceed.

@crazyscience
Copy link

@taylorotwell Couldn't an argument be made that every bug fix is reversion of some PR regardless of its age?

IMO, this is a critical bug fix because:

  1. It doesn't follow assumed behavior.
  2. It doesn't follow the Laravel documentation.
  3. It has a high risk of occurrence, especially in projects that follow the constrained database pattern, which are more likely to be higher-impact, integrity-sensitive applications and less hobbyist-level applications.
  4. It has a devastating and potentially permanently damaging outcome.

This PR isn't fixing a typo or removing some feature because it doesn't fit one individual's use case. It's a critical, high-risk bug. And the consequences aren't just a 500 error. The consequences are unintended large-scale data destruction.

A use case that we encountered was a seeder. The seeder was for a simple lookup table. The table had just a few rows in it. We wanted to update these rows, so we updated the seeder's input, pushed to production and ran the seeder. The first thing the seeder did was DB::truncate() the table. Then it did an insert. The problem was that a few other tables referenced those values, and when the table was truncated it emptied those corresponding tables. Then, there were other tables that referenced those other tables, and those were truncated too.

The truncations didn't stop until the constraints stopped. Years of data were destroyed instantly all because we trusted that DB::truncate() would only truncate the table that was passed as the parameter to the method. This is what the documentation describes. The first parameter is the table to be truncated.

I don't understand the notion that the passage of time somehow excuses this particular bug. It's not as if the bug has gone unnoticed. There have been issues opened here in GitHub discussing the issue. I'm sure it's been discussed on StackOverflow and elsewhere. For some reason, there just hasn't been a PR to fix it. This is that PR, and it's been denied.

I've removed all constraints from my database and established an administrative control to ensure that my team never uses constraints in Laravel projects, so I am no longer affected by the risks this bug presents. However, out of concern for my fellow developers and the potential for developers losing their jobs or ruining their businesses, I'm still here arguing the case.

I beg of you to please reconsider. We were able to reconstruct after losing 60% of our data. Others might not be so lucky. I can't continue to argue my case any further, so if you indeed consider this and decide that cascading truncations is indeed the best way to handle things, so be it. I did my part for the OSS community.

@devcircus
Copy link
Contributor

The point that pushes me over to team revert, is the fact that the Postgres version behaves differently than the others. At least that is what is claimed. If, when using Postgres, all related tables are also truncated when calling Model::truncate(), but when using MySQL, the related tables are not truncated, then something needs to be fixed.

@blakethepatton
Copy link
Contributor Author

Made an example repo: https://github.com/blakethepatton/pg-truncate-example

@kdocki
Copy link
Contributor

kdocki commented Nov 2, 2020

Hey @taylorotwell ... can we make this so that if a user wants to force the truncate cascade they have a special way to do that?Having the cascade delete as the default behavior of truncate() method is going to roll a lot of heads that are using postgresql.

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

Successfully merging this pull request may close these issues.

5 participants