-
-
Notifications
You must be signed in to change notification settings - Fork 836
Invoke-DbaDbDataMasking - Improve performance #9671
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
base: development
Are you sure you want to change the base?
Conversation
public/Invoke-DbaDbDataMasking.ps1
Outdated
$uniqueData = Invoke-DbaQuery -SqlInstance $server -SqlCredential $SqlCredential -Database tempdb -Query $query | ||
} catch { | ||
Stop-Function -Message "Something went wrong getting the unique data" -Target $query -ErrorRecord $_ | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why using "continue" here (you could use the -Continue parameter of Stop-Function) and "return" a few rows later? We need a general way to handle errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take a look at that and get it worked out the other way.
This is a big change and not easy to review. Best would be to let @sanderstad have a look at this and approve it. Sander, do you have the time? |
As #9664 is merged, you should update your branch to resolve the conflict. Then we can continue to review this PR. |
Sorry, I didn't see that you tagged me until now. I could've done the check, but it's already merged |
@sanderstad this perf change isn't merged in - was another fix. Let us know what you think 😄 |
I looked at the code and I know there was a reason why I did it column by column for a single table. @AdamJKoehler Your approach could work, but I'm very much interested in your results from testing. The code looks good for the rest of it, but I would like to see the results from a run with that amount of records (200.000 +). |
@sanderstad, I do have some old ones. I'll get a fresh set in the next few days and send it over. |
Switch from column-based masking to row-based masking.
Type of Change
Invoke-ManualPester
)Purpose
To improve the speed at which Invoke-DbaDbDataMasking masks data in large tables
Approach
This change switches from Column by column masking to row by row masking, allowing for a batch of rows to be done at a single time, instead of having to iterate over each column to be masked in the table for each row.