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

Versioned dev/build performance with large datasets #8958

Open
caffeineinc opened this issue May 2, 2019 · 5 comments
Open

Versioned dev/build performance with large datasets #8958

caffeineinc opened this issue May 2, 2019 · 5 comments

Comments

@caffeineinc
Copy link

caffeineinc commented May 2, 2019

Affected Version

3.7.x

Description

During a dev/build Silverstripe calls an extension augmentDatabase() on Versioned DataObjects. During this function a cleanup query is made which was supposed to cleanup instances where the uniqueness constraint was failed. Unfortunately this query is extremely slow for large data sets, and from the comments, it appears the original bug was fixed.

The issue seems to have manifested in few different ways, which were resolved differently. Although i think the function may have been previously required, i'm not sure it now needs to run on every dev/build.

Instead of running this every time, it might more sense to provide the cleanup as a task, so that it can be run if necessary, while large datasets don't have the performance impact of running this each dev/build

Steps to Reproduce

  • Create a versioned DataObject with 5-8 million rows
  • Run a /dev/build

Related:
#7944
silverstripe/silverstripe-versioned#194

caffeineinc pushed a commit to caffeineinc/silverstripe-framework that referenced this issue May 2, 2019
…e datasets

During a dev/build Silverstripe calls an extension augmentDatabase()
on Versioned DataObjects. During this function a cleanup query is
made which was supposed to cleanup instances where the uniqueness
constraint can't be used.

This results in terrible performance as it's executed on every build.

This fix moves that functionality to a task, so it can be cleaned up
and added to a cron should it be deemed necessary. It's not currently
known if the bug still manifests.

Also removed some commented code.
@caffeineinc
Copy link
Author

This looks like it was resolved in 4.x.x after versioned 1.2.0, since it was abstracted to a module - and you can flag disable the feature.

That said, i still don't think flag disabling it is a great idea, since it still runs every build. I guess the bigger question is if it's still required. Larger websites might not run dev/build for a while, and having slow queries in there still don't make sense as many might not know about some hidden feature flag unless it is better documented.

To me it feels like something that really should be be in a task like [this] (caffeineinc@8b14e21) pr, then it can be run at times when a slow query isn't so blocking..

@maxime-rainville
Copy link
Contributor

I think this part of Versionned is now handling this uniqueness problem in SS4: https://github.com/silverstripe/silverstripe-versioned/blob/2d19b8bca39d73de221ca8cfc78100100172d828/src/Versioned.php#L892-L929

@maxime-rainville
Copy link
Contributor

I'm not sure this would be high impact enough to warrant fixing in SS3. Unfortunately, SilverStripe 3 has entered limited support in June 2018. This means we'll only be fixing critical bugs and security issues for SilverStripe 3 going forward.

We should take some time to make sure this logic is performant in SS4.

@caffeineinc
Copy link
Author

caffeineinc commented Jun 5, 2019

Yeah, it's more the point here:

It'll just be a flag to allow this orphan check to be disabled.

The resolution for SS4 was to add a undocumented flag but the task is still executed each dev/build which means the problem still affects the majority of deployments, and every dev/build with a large versioned set, multiplied by every versioned subclass.

I would say, by default the change should be opt in since the issue was handled as you say Then we can document the flag, and if users still encounter issues, they can enable it if required.

My question is that high impact enough to warrant a change like above for silverstripe/versioned ?

@caffeineinc
Copy link
Author

caffeineinc commented Jun 5, 2019

Ok. so i looked closely at the new code in silverstripe versioned and confirmed there's no change required for SS4.

It doesn't have the same performance impact like the fix for SS3.

Could be worth merge but not a release ? I dunno

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

No branches or pull requests

4 participants