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

Include ON DELETE CASCADE associations in the delete order computation #10913

Merged
merged 6 commits into from
Jan 12, 2024

Conversation

mpdude
Copy link
Contributor

@mpdude mpdude commented Aug 17, 2023

In order to resolve #10348, some changes were included in #10547 to improve the computed delete order for entities.

One assumption was that foreign key references with ON DELETE SET NULL or ... CASCADE need not need to be taken into consideration when planning the deletion order, since the RDBMS would unset or cascade-delete such associations by itself when necessary. Only associations that do not use RDBMS-level cascade handling would be sequenced, to make sure the referring entity is deleted before the referred-to one.

This assumption is wrong for ON DELETE CASCADE. The following examples give reasons why we need to also consider such associations, and in addition, we need to be able to deal with cycles formed by them.

In the following diagrams, odc means ON DELETE CASCADE, and ref is a regular foreign key with no extra ON DELETE semantics.

graph LR;
C-->|ref| B;
B-->|odc| A;
Loading

In this example, C must be removed before B and A. If we ignore the B->A dependency in the delete order computation, the result may not to be correct. ACB is not a working solution.

graph LR;
A-->|odc| B;
B-->|odc| A;
C-->|ref| B;
Loading

This is the situation in #10912. We have to deal with a cycle in the graph. C must be removed before A as well as B. If we ignore the B->A dependency (e.g. because we set it to "optional" to get away with the cycle), we might end up with an incorrect order ACB.

graph LR;
A-->|odc| B;
B-->|odc| A;
A-->|ref| C;
C-->|ref| B;
Loading

This example has no possible remove order. But, if we treat odc edges as optional, A -> C -> B would wrongly be deemed suitable.

graph LR;
A-->|ref| B;
B-->|odc| C;
C-->|odc| B;
D-->|ref| C;
Loading

Here, we must first remove A and D in any order; then, B and C in any order. If we treat one of the odc edges as optional, we might find the invalid solutions ABDC or DCAB.

Solution implemented in this PR

First, build a graph with a node for every to-be-removed entity, and edges for ON DELETE CASCADE associations between those entities. Then, use Tarjan's algorithm to find strongly connected components (SCCs) in this graph. The significance of SCCs is that whenever we remove one of the entities in a SCC from the database (no matter which one), the DBMS will immediately remove all the other entities of that group as well.

For every SCC, pick one (arbitrary) entity from the group to represent all entities of that group.

Then, build a second graph. Again we have nodes for all entities that are to be removed. This time, we insert edges for all regular (foreign key) associations and those with ON DELETE CASCADE. ON DELETE SET NULL can be left out. The edges are not added between the entities themselves, but between the entities representing the respective SCCs.

Also, for all non-trivial SCCs (those containing more than a single entity), add dependency edges to indicate that all entities of the SCC shall be processed after the entity representing the group. This is to make sure we do not remove a SCC inadvertedly by removing one of its entities too early.

Run a topological sort on the second graph to get the actual delete order. Cycles in this second graph are a problem, there is no delete order.

Fixes #10912.

@mpdude
Copy link
Contributor Author

mpdude commented Aug 23, 2023

X-ref #9376

@Dallas62
Copy link

Dallas62 commented Dec 3, 2023

Hi @mpdude,
Do you have an ETA on the landing of this fix ?
Thanks !

@mpdude mpdude force-pushed the on-delete-cascade-order branch from 49b9ee7 to fb9ff08 Compare January 2, 2024 18:43
@mpdude mpdude changed the base branch from 2.16.x to 2.17.x January 2, 2024 18:43
@mpdude mpdude marked this pull request as ready for review January 2, 2024 18:43
@mpdude
Copy link
Contributor Author

mpdude commented Jan 2, 2024

@Dallas62 Could you please try the change suggested in this PR?

@mpdude mpdude force-pushed the on-delete-cascade-order branch from 4dbc2f8 to b95529d Compare January 3, 2024 08:53
@mpdude
Copy link
Contributor Author

mpdude commented Jan 3, 2024

Feedback addressed

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍
More than that, it was a very interesting read! Thanks for all the effort you put in this!

@mpdude mpdude merged commit c6b3509 into doctrine:2.17.x Jan 12, 2024
58 checks passed
@mpdude mpdude deleted the on-delete-cascade-order branch January 12, 2024 21:44
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.

Order of referenced child deletions not correct when referencing to itself
3 participants