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

FEATURE: Speedup content cache flush by using cte in findAncestorNodeAggregateIds #5261

Open
wants to merge 11 commits into
base: 9.0
Choose a base branch
from

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Sep 24, 2024

Followup to #5221

ContentGraph::findParentNodeAggregates becomes slower on bigger datasets. Due to the mass on executions on a cr:replay, this sums up very quickly. Via #5268 the query will be improved but this pr introduces ContentGraph::findAncestorNodeAggregateIds to make this operation as performant as possible.

  • Adds comment for CacheFlushingStrategy strategies

  • Introduces ContentGraphInterface::findAncestorNodeAggregateIds using native sql cte to speedup cache flushing. (see comment)

  • Move test which creates illegal state to content graph package and use native sql to create the state to not run any catchup hooks. Previously we needed to handle the case of infinite loops to not crash:

    Prevent infinite loops
    NOTE: Normally, the content graph cannot contain cycles. However, during the
    testcase "Features/ProjectionIntegrityViolationDetection/AllNodesAreConnectedToARootNodePerSubgraph.feature"
    and in case of bugs, it could have actually cycles.
    The content cache catchup hook leverage this method and would otherwise be hanging up in an endless loop.
    That's why we track the seen NodeAggregateIds to be sure we don't travers them multiple times.

  • fixes a bug where you cannot replay because the workspace is "missing" and no content graph exists

Upgrade instructions

Review instructions

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign marked this pull request as draft September 24, 2024 19:19
@mhsdesign mhsdesign marked this pull request as ready for review September 25, 2024 11:43
@@ -188,6 +189,19 @@ public function findParentNodeAggregates(
return $this->mapQueryBuilderToNodeAggregates($queryBuilder);
}

public function findAncestorNodeAggregateIds(NodeAggregateId $entryNodeAggregateId): NodeAggregateIds
Copy link
Contributor

Choose a reason for hiding this comment

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

As far a I understood, this function needs to be more complex and return a set of pathways with ancestor nodes.
https://neos-project.slack.com/archives/C04PYL8H3/p1725605274115779?thread_ts=1725460465.658809&cid=C04PYL8H3

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm but to get that right, does that mean that if implemented more complex also other node ids will be returned or just that all the node ids will be nicely ordered by dimension in a special value object. The latter i consider just an improvement and not critical. The idea is to improve the performance of the method via native sql at first. Its marked as @internal so we can improve the structure later?

Copy link
Contributor

@dlubitz dlubitz Sep 25, 2024

Choose a reason for hiding this comment

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

the node ids will be nicely ordered by dimension in a special value object

Yes this one

Hmm but to get that right, does that mean that if implemented more complex also other node ids will be returned or just that all the node ids will be nicely ordered by dimension in a special value object. The latter i consider just an improvement and not critical. The idea is to improve the performance of the method via native sql at first. Its marked as @internal so we can improve the structure later?

But you don't improve that with this PR.
And even if it is marked as internal, i think we shouldn't put that into an interface if we already know, that we will change that.

Copy link
Member Author

@mhsdesign mhsdesign Sep 25, 2024

Choose a reason for hiding this comment

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

But you don't improve that with this PR.

i could try to do that ... emphasis on try :D or ask bernhard. But i get your point. Lets see how we can continue.

cc @nezaniel @bwaidelich @kitsunet

edit: and we def need behat tests for the method i forgot :D

Copy link
Member Author

Choose a reason for hiding this comment

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

added behat tests and introduced cte query to improve performance. Its now ordered desc by the parentnodeanchor

@mhsdesign mhsdesign marked this pull request as draft September 25, 2024 17:33
@mhsdesign mhsdesign changed the title TASK: content cache flusher followup FEATURE: Speedup content cache flush by using cte in findAncestorNodeAggregateIds Sep 25, 2024
@mhsdesign mhsdesign marked this pull request as ready for review September 26, 2024 09:17
In the case of the content cache flusher we do not care about the order and ordering it by parentnodeanchor and position (for siblings) is slower and not even correct in all situations as the parentnodeanchor is just an autoincrement without meaning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants