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

Add Statement Batching For PtOnlineSchemaChangeConnection #36

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

Watercycle
Copy link
Contributor

@Watercycle Watercycle commented Nov 10, 2023

Overview

See #26. This PR makes it so that every individual statement generated as part of ZeroDowntimeSchema::table gets passed along to pt-online-schema-change in one go, which is both more performant and seemingly in-line with how native Schema-based migrations work.

Thoughts

  1. I plan to battle test this a little more; but, was curious if anyone has any initial concerns with this approach? (which was originally laid out by @dbhynds in the linked issue)
  2. If we're concerned about overriding Blueprint, perhaps we could make it an opt-in feature by adding in a Laravel version check, a zero-downtime.php + ['batched' => true] config option, and/or a static variable would de-risk the change? The most immediate concern that comes to mind is that this will be incompatible with other libraries overriding Blueprint.

Testing

I haven't extensively manually tested this change yet; but, as a smoke-test, it works with several of my company's migrations and has been batching groups of $table->dropForeign and $table->foreign calls correctly.

@Daursu
Copy link
Owner

Daursu commented Nov 12, 2023

Thanks for submitting this PR. I do like the approach, and I'm good with overriding the Blueprint class when importing this package, as we are actually falling back onto the original behavior when using a connection that doesn't support batching.

I'll try to setup a GitHub workflow that automatically tests the package against all supported versions of Laravel & PHP versions to ensure we are not introducing any breaking changes.

@Watercycle Watercycle force-pushed the add-statement-batching branch from 00940f2 to c9182e1 Compare November 14, 2023 00:22
@Watercycle
Copy link
Contributor Author

The test matrix is the most sure-fire way; but, I think this should be PHP 7.0 compatible and Laravel 5 (src) compatible, now.

@Watercycle Watercycle marked this pull request as ready for review November 14, 2023 00:29
@Daursu
Copy link
Owner

Daursu commented Dec 10, 2023

I've added integration tests against both Percona and gh-ost on the main branch. I think we need to rebase your PR against it to force the tests to run. I also dropped support for older versions of PHP and Laravel in future releases, as they are no longer maintained.

@Watercycle
Copy link
Contributor Author

Awesome! I'll give this another pass in the coming days.

@Watercycle Watercycle force-pushed the add-statement-batching branch from c9182e1 to be5afef Compare December 14, 2023 23:27
@Watercycle
Copy link
Contributor Author

Thank you for taking the time to bump the dependencies and expand the tests! It looks like the tests passed on my fork. Is there anything else you'd like me to follow-up with?

@Daursu
Copy link
Owner

Daursu commented Dec 15, 2023

Looks good to me. I'll merge it in and will create a new release for it

@Daursu
Copy link
Owner

Daursu commented Dec 15, 2023

Thank you for contributing, this was on the TODO list for a while 😄

@Daursu Daursu merged commit 4ebfa5c into Daursu:master Dec 15, 2023
14 checks passed
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.

2 participants