dnnsoftware
[Enhancement]: Stop SQL script execution on error
Is there an existing issue for this?
- [X] I have searched the existing issues
Description of problem
Currently SQL scripts with batching (scripts separated by 'GO' statements) get run to completion even if there is an error in one of the batches. The error is reported, but often later pieces of scripts rely on previous ones being executed first. Therefore continuing after an error can leave the database in a bad state.
Description of solution
I propose adding break statement in the catch block of the loop. This will prevent the rest of the script from running if there an error.
Description of alternatives considered
Considered leaving the implementation as is, and engineering my scripts to either not use batching or to be safe if a batch in the middle fails. This has proved to be very cumbersome and not always possible.
Anything else?
This method in question: https://github.com/dnnsoftware/Dnn.Platform/blob/c3f4725b6445d9ed6ea495b9cded76606a3d2418/DNN%20Platform/Library/Data/SqlDataProvider.cs#L213
Do you be plan to contribute code for this enhancement?
- [X] Yes
Would you be interested in sponsoring this enhancement?
- [ ] Yes
Code of Conduct
- [X] I agree to follow this project's Code of Conduct
Historically the expectation is that SQL Server scripts should be re-runnable, changing this behavior to stop-on-error would be a major breaking change that would impact installation, upgrade, and extension installation/upgrade.
Although I understand the desire here, I'm not sure that the downstream impacts would be worth the change. Just my $0.02 though.
I don't see how this would this prevent a script from being re-runnable. In fact I believe it would make it more so, because instead of running statements out of order this would stop execution and allow whatever problem there was to be resolved before continuing.
I do agree this would be a breaking change, and should probably be held until a major release.
@DanielBolef this wouldn't prevent it from being re-runnable, but it would change the expectation of what is completed or what is not yet completed.
The "rollback" solution for a failure is often to run everything again anyway, so maybe it isn't a big deal, but if we stop abruptly at the first error it could result in many issues with incompatible scripts without manual intervention.
My concern is mostly with upgrade or other issues that right, or wrong, there could be errors. if the scripts stop someone has to manually edit and re-run if we change the behavior, where as the current behavior allows for everything to execute and you can debug. This is a larger issue on upgrades due to possible data issues or otherwise and if certain later scripts don't run the site might not be functional.
@mitchelsellers would your experience tell you that continuing to run dependent scripts after an error is less likely to leave the site in an unrecoverable state than stopping after an unexpected error? I can see scenarios either way.
Personally, I would prefer that the process stop after an error so that it can be investigated, rather than continuing through with the strong possibility that underlying issues are present but not addressed.
@bdukes Yes, I would say the current behavior has a better change of leaving you able to recover, compared to others. A few examples, one third-party module updated some permissions that didn't exist so the script failed, but they also later in the script updated the table structures for all of their data. Since those scripts ran successfully, all of the data was available and the site was not impacted. It was possible to review the failure fix the script and move on. If that script stopped, the site would not have been able to see any content, and in certain circumstances wouldn't even have come online.
We also see this with DNN upgrades, especially on sites with "questionable" pasts where again its 99.9% ok, but that one script messes up. If the site fails to load it is a lot of manual work to re-run via SSMS due to the usage of {objectQualifier} and {databaseOwner}
I normally prefer fail-early as well, it would be ideal if there was a transaction scope that would rollback in the case of a failure.
Could we add some opt-in attributes to the script component to run scripts fail-fast and/or in a transaction?
@bdukes that would be my sugggestion as well, keeping the existing behaviour, if the flag is not set and being able to stop on error, if the flag is set.
in general, it is good practise not to rely on a previous execution but add a check into table modifying commands, e.g by using a WHERE statement or an IF.
