Compute the commit order (inserts/deletes) on the entity level #10547

mpdude · 2023-02-27T10:53:34Z

Fixes some long-standing issues that have to do with commit order computation and cyclic associations.

When performing inserts or deletes of entities, the ORM tries to find a sequence of operations so that all foreign key constraints can be satisfied. Basically, before inserting an entity, all other entities it refers to have to be inserted. And before deleting an entity, all other entities that refer to it have to be deleted, or database-level SET NULL has to be used.

The current implementation for this works at the entity class level. It checks which entity class refers to which other class(es), and based on this information, computes an ordering of entity classes. Then, all entities of the first class will be processed, then those of the second class and so on. The same ordering is used for deletions.

In the PRs #10531 and #10532, two examples are given why this is not sufficient: There may be references between entities of the same class, so that we can only insert those in the database by looking at their associations at the entity level. Or, we may have references between entities of two different classes that require us to go back and forth – insert one entity of the first class, before we can insert one for the second class, before we can go back to the first class for a third entity.

Then, there is a shortcoming in the current CommitOrderCalculator: It does not detect cycles at all. This refers to situations where we cannot find a commit order because every single entity to be processed requires another one to come first. Cycles can be broken at NULLable foreign keys, where a NULL value can temporarily be used and the value updated at a later stage. This is called "extra updates". Places where such extra updates can be used to save the day can be found by backtracking in the commit order computation, in case cycles are found.

Last, the commit order for INSERTs and DELETEs needs to be based on different criteria: INSERTs can make use of extra updates and require NULLable foreign key definitions for this to work. DELETEs, however, currently do not make use of such extra updates. For them, whether the database can use "SET NULL" on foreign key deletions is the important factor.

To make it possible to review the changes, they were broken down into a series of independent PRs against a temporary branch. This PR here does a final merge of the result into 2.16.x.

The individual PRs are:

(Potentially) fixed issues/closed PRs:

Not fixed issues/PRs:

DDC-601: UnitOfWork commit order problem #5109 – this is about extra unique constraints and/or performing DELETE before INSERT, beyond what the ORM requires itself to make foreign keys work; similar in Flush could delete entities before inserting new ones #5742, oneToMany orphanRemoval with unique - constraint violation #6776
Cascade deletion complicate-referenced entities does not work [ForeignKeyConstraintViolationException] #5665, Failing test: UoW unable to break cycles when removing entities without DB-level cascade #10548 – those are about scheduling extra updates before DELETE, to null out foreign keys and not require a database-level SET NULL

(Probably) related issues/PR with unclear state and/or missing reproducers:

Definitely too big for this PR:

Performance: Persisters no longer being able to batch inserts
Performance: Optimizing the DFS to find a minimal feedback arc set/minimize the number of scheduled extra updates

rvanlaak · 2023-02-28T09:48:05Z

I am a bit rusty on the topic regarding the debugging of the commit order, but based on looking at the CommitOrderCalculator your suggested changes look extremely promising in solving the long standing commit order bug. 👏🏻

derrabus

I think I'll need to review this with more time than I have right now. Awesome work. 🤯

lib/Doctrine/ORM/Internal/CommitOrderCalculator.php

lib/Doctrine/ORM/UnitOfWork.php

mpdude · 2023-03-01T09:37:47Z

@derrabus Yes, this PR is much bigger than what I'd like to see to make reviews easy.

I did not find a good way to break it down into chunks that would make sense on their own and could be merged. Computing the order on the entity level with a broken CommitOrderCalculator does not work. Fixing the COC will break use cases that currently work as long as we don't consider individual entities.

What I could offer is that I try to break this down into pieces and guide you and everyone interested through reviews in dedicated PRs, with smaller scope each.

We'd need to merge all those PRs into a dedicated branch when they are approved. Tests for individual features probably can be working, but the full test suite won't pass until we're done.

In the end, the temporary branch could be merged and should roughly be equivalent to this PR here.

WDYT – would that help, or do you have other suggestions?

derrabus · 2023-03-01T11:16:11Z

I could also offer to setup a Google Meet call where you can guide me and maybe @greg0ire and @beberlei through the changes. That might be more effective.

jhony1104

This is no complete review, but since @mpdude asked if I could look at it:

Only looking at the CommitOrderCalculator, unlike #8349, #7866, #8067, #7180, #9377 and others this PR correctly deals with circles using backtracing and if needed ignores nullable relations. Unlike some of the listet PRs it does not only work reliably for (simple) testcases, but I have also testet it in our product with ~130 tables (where for example #8349 didn't work).

Merging this would allow us to start using unmodified doctrine.

…oming from doctrine/orm#10547

…doctrine/orm#10547

See doctrine/orm#10547

- order item creation depends on the order of autoincrement ids - the way in which order the inserts are executed changed in 2.16.0 - see doctrine/orm#10547 and doctrine/orm#10864

* fix: add conflict on doctrine/orm:2.16.0 See doctrine/orm#10547 * fix: extend above 2.16 as 2.17 is bugged too * fix(maxdepthEager): relying on node existance instead of the full representation * fix: correction for more insert order checks * fix: correction for more insert order checks * fix: typo

This PR prevents reentrant calls to `UnitOfWork::commit()` with a clear exception message. Reentrance happens, for example, when users call `EntityManager::flush()` from within `postPersist` or similar event listeners. Since doctrine#10547, it causes strange-looking, non-helpful error messages (doctrine#10869). Instead of treating this as a bug and trying to fix it, my suggestion is to fail with a clear exception message instead. Reasons: I assume the UoW was never designed to support reentrant `flush()` calls in the first place. So, trying to make (or keep) this working might be a rabbit hole. The assumption is based on the following: * Not a single test in the ORM test suite covers or even triggers `flush()` reentrance – otherwise, such a test would have failed with the changes suggested here. * Documentation for e. g. [`preFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) or [`postFlush`](https://www.doctrine-project.org/projects/doctrine-orm/en/2.16/reference/events.html#preflush) explicitly mentions that `flush()` must not be called again. I don't know why this is not also stated for e. g. `postPersist`. But why would it be safe to call `flush()` during `postPersist`, in the middle of a transaction, when there are reasons against calling it in `postFlush`, just before final cleanups are made? * Last but not least, entity insertions, computed changesets etc. are kept in fields like `UnitOfWork::$entityChangeSets` or `UnitOfWork::$originalEntityData`. It's all to easy to mess up these states when we re-compute changesets mid-way through a `flush()` – and again, if that were anticipated, I'd assume to find any kind of test coverage.

This PR addresses the issue brought up in doctrine#10869. It happens when users use event listeners – `postPersist` in particular – to make changes to entities and/or persist new entities and finally call `EntityManager::flush()`, while the `UnitOfWork` is currently within the commit phase. There is a discussion in doctrine#10900 whether this kind of reentrance should be deprecated in 2.x and prevented in 3.x. But, in order to prevent complete breakage with the 2.16.0 update, this PR tries to apply a band-aid 🩹. A few things changed inside the UoW commit implementation in 2.16, and for sure this PR does not restore all details of the previous behavior. Take it with a grain of salt. Here's the details. The issue appears when `UoW::commit()` is performing entity insertions, and `postPersist` listener causes `commit()` reentrance when there are pending insertions left. In that situation, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. The entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to the reported failure down the road. The mitigation is to check for this condition and skip such entities. Before doctrine#10547 (pre-2.16), things worked a bit differently: The UoW did not build a list of entities (objects) as the commit order, but a sequence of _classes_ to process. For every entity _class_, it would find all entity instances in `UoW::$entityInsertions` and pass them to the persister in a batch. The persister, in turn, would execute the `INSERT` statements for _all_ those entities _before_ it dispatched the `postInsert` events. That means when a `postInsert` listener was notified pre-2.16, the UoW was in a state where 1) all insertions for a particular class had been written to the database already and 2) the UoW had not yet gathered the next round of entities to process.

….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users that call `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and UoW::$entityInsertions being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.

….0 (Take 2) The changes from doctrine#10547, which landed in 2.16.0, cause problems for users calling `EntityManager::flush()` from within `postPersist` event listeners. * When `UnitOfWork::commit()` is re-entered, the "inner" (reentrant) call will start working through all changesets. Eventually, it finishes with all insertions being performed and `UoW::$entityInsertions` being empty. After return, the entity insertion order, an array computed at the beginning of `UnitOfWork::executeInserts()`, still contains entities that now have been processed already. This leads to a strange-looking SQL error where the number of parameters used does not match the number of parameters bound. This has been reported as doctrine#10869. * The fixes made to the commit order computation may lead to a different entity insertion order than previously. `postPersist` listener code may be affected by this when accessing generated IDs for other entities than the one the event has been dispatched for. This ID may not yet be available when the insertion order is different from the one that was used before 2.16. This has been mentioned in doctrine#10906 (comment). This PR suggests to address both issues by dispatching the `postPersist` event only after _all_ new entities have their rows inserted into the database. Likewise, dispatch `postRemove` only after _all_ deletions have been executed. This solves the first issue because the sequence of insertions or deletions has been processed completely _before_ we start calling event listeners. This way, potential changes made by listeners will no longer be relevant. Regarding the second issue, I think deferring `postPersist` a bit until _all_ entities have been inserted does not violate any promises given, hence is not a BC break. In 2.15, this event was raised after all insertions _for a particular class_ had been processed - so, it was never an "immediate" event for every single entity. doctrine#10547 moved the event handling to directly after every single insertion. Now, this PR moves it back a bit to after _all_ insertions.

* fix: add conflict on doctrine/orm:2.16.0 See doctrine/orm#10547 * fix: extend above 2.16 as 2.17 is bugged too * fix(maxdepthEager): relying on node existance instead of the full representation * fix: correction for more insert order checks * fix: correction for more insert order checks * fix: typo

This PR changes a detail in the commit order computation for depended-upon entities. We have a parent-child relationship between two entity classes. The association is parent one-to-many children, with the child entities containing the (owning side) back-reference. Cascade-persist is not used, so all entities have to be passed to `EntityManager::persist()`. Before v2.16.0, two child entities C1 and C2 will be inserted in the same order in which they are passed to `persist()`, and that is regardless of whether the parent entity was passed to `persist()` before or after the child entities. As of v2.16.0, passing the parent entity to `persist()` _after_ the child entities will lead to an insert order that is _reversed_ compared to the order of `persist()` calls. This PR makes the order consistent in both cases, as it was before v2.16.0. #### Cause When the parent is passed to `persist()` after the children, commit order computation has to re-arrange the entities. The parent must be inserted first since it is referred to by the children. The implementation of the topological sort from doctrine#10547 processed entities in reverse `persist()` order and unshifted finished nodes to an array to obtain the final result. That leads to dependencies (parent → before c1, parent → before c2) showing up in the result in the reverse order of which they were added. This PR changes the topological sort to produce a result in the opposite order ("all edges pointing left"), which helps to avoid the duplicate array order reversal. #### Discussion * This PR _does not_ change semantics of the `persist()` so that entities would (under all ciscumstances) be inserted in the order of `persist()` calls. * It fixes an unnecessary inconsistency between versions before 2.16.0 and after. In particular, it may be surprising that the insert order for the child entities depends on whether another referred-to entity (the parent) was added before or after them. * _Both_ orders (c1 before or after c2) are technically and logically correct with regard to the agreement that `commit()` is free to arrange entities in a way that allows for efficient insertion into the database. Fixes doctrine#11058.

…ion (#10913) 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. ```mermaid graph LR; C-->|ref| B; B-->|odc| A; ``` 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. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; C-->|ref| B; ``` 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. ```mermaid graph LR; A-->|odc| B; B-->|odc| A; A-->|ref| C; C-->|ref| B; ``` This example has no possible remove order. But, if we treat `odc` edges as optional, A -> C -> B would wrongly be deemed suitable. ```mermaid graph LR; A-->|ref| B; B-->|odc| C; C-->|odc| B; D-->|ref| C; ``` 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](https://en.wikipedia.org/wiki/Tarjan%27s_strongly_connected_components_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 changed the title ~~commit order entity level~~ Compute the commit order (inserts/deletes) on the entity level Feb 27, 2023

mpdude force-pushed the commit-order-entity-level branch 7 times, most recently from 5bb392b to c653f98 Compare February 27, 2023 17:25

mpdude force-pushed the commit-order-entity-level branch from c5e27d3 to f9a8d11 Compare February 27, 2023 18:51

This was referenced Feb 27, 2023

Doctrine tries to delete entities in incorrect order #9376

Closed

commit order must consider non-nullability of join columns as prioritised over nullable join columns: Take 2 #7180

Closed

mpdude force-pushed the commit-order-entity-level branch 5 times, most recently from 19066e6 to ba8c3dd Compare February 28, 2023 00:00

mpdude mentioned this pull request Feb 28, 2023

Failing test: UoW unable to break cycles when removing entities without DB-level cascade #10548

Open

derrabus requested changes Feb 28, 2023

View reviewed changes

lib/Doctrine/ORM/Internal/CommitOrderCalculator.php Outdated Show resolved Hide resolved

lib/Doctrine/ORM/UnitOfWork.php Outdated Show resolved Hide resolved

ostrolucky force-pushed the 2.14.x branch from b7bb13f to a28e2d8 Compare March 5, 2023 20:21

jhony1104 previously approved these changes Mar 7, 2023

View reviewed changes

mpdude dismissed jhony1104’s stale review via a20f2fd March 11, 2023 11:25

GregoireHebert mentioned this pull request Aug 1, 2023

Creation order parent-children inversed #10864

Closed

GregoireHebert added a commit to GregoireHebert/core that referenced this pull request Aug 1, 2023

fix: BC Break probably coming from doctrine/orm#10547

7fceb74

GregoireHebert added a commit to GregoireHebert/core that referenced this pull request Aug 1, 2023

fix: add conflict on doctrine/orm:2.16.0 to avoid BC Break probably c…

8fad745

…oming from doctrine/orm#10547

GregoireHebert mentioned this pull request Aug 1, 2023

fix: change tests to not rely on order of insertion api-platform/core#5709

Merged

GregoireHebert added a commit to GregoireHebert/core that referenced this pull request Aug 1, 2023

fix: add conflict on doctrine/orm:2.16.0 to avoid BC Break coming from …

e05b1dc

…doctrine/orm#10547

GregoireHebert added a commit to GregoireHebert/core that referenced this pull request Aug 1, 2023

fix: add conflict on doctrine/orm:2.16.0

6501e03

See doctrine/orm#10547

grossmannmartin mentioned this pull request Aug 2, 2023

doctrine/orm 2.16.0 and higher are now marked as conflicting shopsys/shopsys#2691

Merged

mpdude mentioned this pull request Aug 3, 2023

2.16.0 - Issue with persisting entities #10869

Closed

olivier1980 mentioned this pull request Aug 9, 2023

Commit order reversed? #10897

Closed

mpdude mentioned this pull request Aug 9, 2023

RFC: Prevent UnitOfWork::commit() reentrance? #10900

Closed

1 task

mpdude mentioned this pull request Aug 17, 2023

Mitigate problems with EntityManager::flush() reentrance since 2.16.0 #10906

Closed

greg0ire mentioned this pull request Aug 17, 2023

Flush could delete entities before inserting new ones #5742

Open

mpdude mentioned this pull request Aug 21, 2023

Mitigate problems with EntityManager::flush() reentrance since 2.16.0 (Take 2) #10915

Merged

mpdude mentioned this pull request Aug 21, 2023

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

Merged

franmomu mentioned this pull request Sep 3, 2023

Remove conflict with doctrine/orm >= 2.16.0 doctrine-extensions/DoctrineExtensions#2687

Merged

greg0ire mentioned this pull request Oct 7, 2023

Remove CommitOrderCalculator and related classes #10966

Closed

mpdude mentioned this pull request Dec 21, 2023

Avoid an inconsistency in topological sort result order #11086

Merged

mitelg mentioned this pull request Mar 6, 2024

Make newest version of doctrine/orm work shopware5/shopware#2633

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compute the commit order (inserts/deletes) on the entity level #10547

Compute the commit order (inserts/deletes) on the entity level #10547

mpdude commented Feb 27, 2023 •

edited

Loading

rvanlaak commented Feb 28, 2023 •

edited

Loading

derrabus left a comment

mpdude commented Mar 1, 2023

derrabus commented Mar 1, 2023

jhony1104 left a comment •

edited

Loading

Compute the commit order (inserts/deletes) on the entity level #10547

Compute the commit order (inserts/deletes) on the entity level #10547

Conversation

mpdude commented Feb 27, 2023 • edited Loading

rvanlaak commented Feb 28, 2023 • edited Loading

derrabus left a comment

Choose a reason for hiding this comment

mpdude commented Mar 1, 2023

derrabus commented Mar 1, 2023

jhony1104 left a comment • edited Loading

Choose a reason for hiding this comment

mpdude commented Feb 27, 2023 •

edited

Loading

rvanlaak commented Feb 28, 2023 •

edited

Loading

jhony1104 left a comment •

edited

Loading