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/disable triggers #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

markgould
Copy link

In some of our legacy databases we have tables with delete triggers that insert into auditing tables. This is causing some issues with using Respawn.

I've wrapped the delete statement with disable/enable trigger and created a unit test for it.

I thought about adding a flag for this, but wasn't sure if that would be necessary. If customization is necessary in future, maybe the adapters could be made public so one could do something like:

checkpoint.Adapter = new SqlServerDbAdapter { DisableTriggers = true }; 

Not sure why it's messing with the spacing in the other tests.

Copy link
Owner

@jbogard jbogard left a comment

Choose a reason for hiding this comment

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

Is it possible to check a table has triggers instead of adding it to every delete? That triples overall execution time.

@markgould
Copy link
Author

I'm trying to figure out the best way to implement this.

One idea is to have the BuildTableCommandText return an additional column indicating if the table has triggers. Currently I have only implemented the delete trigger logic for MSSQL. The BuildTableCommandText for all the other implementations could simply return 0, if that's acceptable. A "HasTriggers" property could be added to the Table class so when BuildDeleteCommandText is called, I can check if the table has triggers before writing the disable/enable statements.

The other option is to try and do a whole bunch of extra SQL in the BuildDeleteCommandText but that would just slow everything down, so I don't see that as a good option.

Thoughts?

@jbogard
Copy link
Owner

jbogard commented May 28, 2019

I think that's fine for just MSSQL, I don't know what the trigger behavior is anyway for the other DBs.

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