Support for triggers no rollback#30

eldadts · 2021-08-19T06:50:25Z

Here is a working version of trigger support.
3 new flag added:

include-triggers - When true, the triggers (if exist) will be created on the new table
trigger-suffix
remove-trigger-suffix-if-exists - Removal (instead of addition) of trigger suffix if it exists at the end of the original trigger name.

As agreed, we create a ghost trigger instead of a complete trigger migration.

One thing I'm confuse about is the rollback: Incase there is a rollback and the ghost table is either not in use or dropped, then why do we need to do any rollback on the ghost triggers?

shlomi-noach

Thank you for this work! I am told this already runs well in your production environment?

I do have a few comments on both logic and coding style, see inline.

One thing I'm confuse about is the rollback: Incase there is a rollback and the ghost table is either not in use or dropped, then why do we need to do any rollback on the ghost triggers?

I'm assuming by "rollback on the ghost triggers" you mean "dorpping" them. And right now I don't find a good case why we'd have to drop them.

go/base/context.go

go/cmd/gh-ost/main.go

go/base/context.go

go/mysql/utils.go

go/logic/migrator.go

go/logic/applier.go

eldadts · 2021-11-14T15:25:36Z

I've made all the necessary changes.
Will do some tests and then let it run in Prod for a while before submitting

Support for triggers

shlomi-noach

Other than minor comment, and pending further production testing, this looks good to me!

go/base/context.go

shlomi-noach · 2021-11-29T08:14:49Z

CI fails on gofmt. Please run gofmt -s -w go/ and commit changes.

Support for triggers

eldadts · 2021-12-01T15:59:10Z

Done!

shlomi-noach · 2021-12-15T11:10:58Z

Please see unit tests failure:

Running unit tests
+ echo 'Running unit tests'
+ go test ./go/...
--- FAIL: TestGetTriggerNames (0.00s)
Error:     spec.go:38: Expected:
        [[[my_trigger_gho]]]
        - got:
        [[[my_trigger]]]

change trigger suffix logic

eldadts · 2021-12-16T11:36:42Z

Please see unit tests failure:

Running unit tests
+ echo 'Running unit tests'
+ go test ./go/...
--- FAIL: TestGetTriggerNames (0.00s)
Error:     spec.go:38: Expected:
        [[[my_trigger_gho]]]
        - got:
        [[[my_trigger]]]

Sorry about that. Forgot to add the test file to the PR.
In any case, the guys here requested to preserve the previous triggerSuffix logic (which made sense once they explained it to me)

I've added back the && strings.HasSuffix(triggerName, this.TriggerSuffix) in GetGhostTriggerName.
The use case is if the suffix does not exists in the original trigger name then to add it, otherwise remove it.

br,
Eldad

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach · 2022-01-10T13:08:46Z

Pushed a gofmt change to let CI pass

shlomi-noach · 2022-01-10T15:11:43Z

Past due time, this now merged! Great work everyone and congrats!

yakirgb · 2023-12-14T13:05:02Z

@shlomi-noach i see that this change not in https://github.com/github/gh-ost
I think that i'll make PR to bring this change into https://github.com/github/gh-ost too .

* Add trigger support to gh-ost based on openark#30 * Add comprehensive test cases for trigger support functionality * Fix trigger-basic test by adding required --trigger-suffix parameter and remove fail-trigger-unsupported test * Update trigger suffix in local test configurations Modify extra_args files for trigger-complex, trigger-multiple, and trigger-self-reference tests to use different trigger suffixes * Add --remove-trigger-suffix-if-exists to local test configurations Update extra_args files for trigger tests to include the new --remove-trigger-suffix-if-exists option, ensuring consistent trigger handling across different test scenarios * Fix trigger-long-name test by reducing trigger name length to be valid * Standardize trigger drop statements in local test configurations Update create.sql files across trigger test scenarios to: - Consistently drop both original and ghost triggers - Ensure clean slate before creating triggers - Align with recent trigger suffix changes This change improves test reliability and consistency by explicitly dropping all potential trigger variations before test setup. * Consolidate and enhance trigger test configurations Refactor local test scenarios for triggers by: - Merging multiple trigger test configurations into comprehensive test cases - Updating create.sql files with more complex and diverse trigger scenarios - Standardizing trigger and table setup across different test configurations - Removing redundant test directories while preserving test coverage This change simplifies the trigger testing infrastructure and provides more robust test coverage for gh-ost's trigger handling capabilities. * Add debug logging for ghost trigger validation process Enhance ghost trigger existence check by adding detailed debug logging to: - Log the ghost trigger name being searched - Log the database schema and query details - Log when an existing ghost trigger is found This change improves visibility into the trigger validation process, making troubleshooting easier during migration scenarios. * Improve ghost trigger validation with enhanced logging and verification Modify validateGhostTriggersDontExist() to: - Add a direct query to log all triggers in the database schema - Refactor trigger existence check to use count-based query - Improve debug logging for trigger validation process - Provide more detailed error reporting for existing ghost triggers This change enhances the robustness and observability of the trigger validation mechanism in gh-ost's migration process. * Refactor ghost trigger validation to improve logging and error detection Simplify and enhance the validateGhostTriggersDontExist() method by: - Removing redundant direct query logging - Streamlining trigger existence check - Improving debug logging for trigger validation - Consolidating trigger existence detection logic The changes provide more concise and focused trigger validation with clearer error reporting. * Simplify ghost trigger validation query and reduce logging verbosity Refactor validateGhostTriggersDontExist() to: - Streamline trigger existence check query - Remove redundant debug logging statements - Use a more concise approach to detecting existing triggers The changes reduce code complexity while maintaining the core validation logic for ghost triggers. * Enhance ghost trigger validation query to include table name filter Modify validateGhostTriggersDontExist() to: - Add table name filter to trigger existence check query - Improve specificity of ghost trigger detection - Prevent false positives from similarly named triggers in different tables The change ensures more precise ghost trigger validation by incorporating the original table name into the query criteria. * Enhance trigger test configuration with advanced features and consolidated test scenarios Update trigger-advanced-features test configuration to: - Add new column 'color' and 'modified_count' to test table - Implement more complex trigger logic for color and count tracking - Consolidate trigger test scenarios with richer data transformations - Modify event to test both numeric and color-based updates - Remove redundant trigger-basic-features directory The changes provide a more comprehensive and nuanced test suite for gh-ost's trigger handling capabilities, demonstrating advanced trigger behaviors and self-referencing updates. * Fix lint errors * Update trigger test configuration with suffix change Modify the extra_args file to use '_ght' trigger suffix instead of '_gho', maintaining consistency with recent trigger test configuration updates. * Remove gh-ost-ci-env submodule Clean up repository by removing the gh-ost-ci-env submodule, which appears to be no longer needed in the project structure. * Update create.sql --------- Co-authored-by: Yakir Gibraltar <yakir.g@taboola.com>

eldadts added 2 commits July 22, 2021 09:55

Trigger support - Not working

5fe0cb3

Trigger support -Working - No rollback

d291bf6

shlomi-noach requested changes Oct 27, 2021

View reviewed changes

eldadts and others added 3 commits November 14, 2021 15:31

add trigger struct, resolve comments in pr

c91d9d7

go fmt

0a271d0

Merge pull request #1 from xtrimf/support_for_triggers

e049a6c

Support for triggers

eldadts marked this pull request as ready for review November 15, 2021 11:48

eldadts requested a review from shlomi-noach November 15, 2021 11:51

shlomi-noach approved these changes Nov 29, 2021

View reviewed changes

go/base/context.go Outdated Show resolved Hide resolved

eldadts and others added 3 commits December 1, 2021 15:38

go fmt

0f97a08

reuse GetGhostTriggerName

1e4bbcb

Merge pull request #2 from xtrimf/support_for_triggers

d30bc0d

Support for triggers

eldadts and others added 2 commits December 16, 2021 11:27

change trigger suffix logic

0138458

Merge pull request #3 from xtrimf/support_for_triggers

96cd286

change trigger suffix logic

gofmt

765a1df

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit 277dfa3 into openark:master Jan 10, 2022

yakirgb pushed a commit to yakirgb/gh-ost that referenced this pull request Mar 2, 2025

Add trigger support to gh-ost based on openark#30

94fa782

yakirgb mentioned this pull request Mar 3, 2025

Backport openark/gh-ost/pull/30 to this repo - add triggers support github/gh-ost#1502

Closed

Comments

Conversation

eldadts commented Aug 19, 2021

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eldadts commented Nov 14, 2021

Uh oh!

shlomi-noach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

shlomi-noach commented Nov 29, 2021

Uh oh!

eldadts commented Dec 1, 2021

Uh oh!

shlomi-noach commented Dec 15, 2021

Uh oh!

eldadts commented Dec 16, 2021

Uh oh!

shlomi-noach commented Jan 10, 2022

Uh oh!

shlomi-noach commented Jan 10, 2022

Uh oh!

yakirgb commented Dec 14, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants