Install-DbaMaintenanceSolution: defaults reported by documentation are incorrect

[ReeceGoding]

Verified issue does not already exist?

I have searched and found no existing issue

What error did you receive?

No error. The wrong thing is done silently.

Steps to Reproduce

Firstly, read the Install-DbaMaintenanceSolution documentation and observe our claims that:

• When -Compress is not specified, we use "the SQL Server instance’s default compression setting".
• Both -Verify and -CheckSum "Defaults to enabled (Y) if not specified".

Secondly, spin up an instance with backup compression defaulted to off

# Run after creating the instance
Set-DbaSpConfigure -SqlInstance $YourMachine -ConfigName 'DefaultBackupCompression' -Value 0

and run

$splatInstall = @{
	SqlInstance      = $YourMachine
	InstallJobs      = $true
	ReplaceExisting  = $true
}
Install-DbaMaintenanceSolution @splatInstall

Thirdly, note that the above definitely did not set -Compress, -Verify, or -CheckSum.

Fourthly, observe that the produced jobs do not turn on what they say they should. This

(Get-DbaAgentJobStep -SqlInstance $YourMachine -Job "DatabaseBackup - USER_DATABASES - FULL").Command

returns

EXECUTE [dbo].[DatabaseBackup]
@Databases = 'USER_DATABASES',
@Directory = N'/var/opt/mssql/data',
@BackupType = 'FULL',
@Verify = 'N',
@CleanupTime = NULL,
@CheckSum = 'N',
@LogToTable = 'N',
@Compress = 'N'

which explicitly does the right thing for @Compress, but the wrong thing for -Verify and -CheckSum.

On a version from last week, I get

EXECUTE [dbo].[DatabaseBackup]
@Databases = 'USER_DATABASES',
@Directory = N'/var/opt/mssql/data',
@BackupType = 'FULL',
@Verify = 'Y',
@CleanupTime = NULL,
@Checksum = 'Y',
@LogToTable = 'N'

which does the right thing for -Verify and -CheckSum, but is not explicit about -Compress.

From this, it is quite clear that my #10172 has a lot of the blame. By making the off switches work, I made the documentation a liar.

Please confirm that you are running the most recent version of dbatools

I'm using the current Dev and a version of it from a week ago.

Other details or mentions

Tomorrow, I intend to put in a PR adding tests for these cases as well as our claimed defaults for -BackupLocation and -StartTime. However, it is not clear how the code should be fixed. We cannot make -Verify default to $true because that would be a very naughty way to code a switch and it would break calls that do not attempt to create jobs. Equally, we cannot make -Verify default to $false without changing documented behaviour. So what should we do?

My inclination is that we should change the documentation and make both -Verify and -CheckSum opt in.

What PowerShell host was used when producing this error

PowerShell Core (pwsh.exe)

PowerShell Host Version

N/A

SQL Server Edition and Build number

N/A

.NET Framework Version

N/A

[andreasjordan]

let me copy my comment from the pr to this issue in case we what to discuss here:

I think:

• "Verify" and "Checksum" are switch parameters, so have a default of $false.
• "Verify" is an extra to the backup job that should not be enabled by default as it adds time and maybe costs.
• "Checksum" should be set to $true at the instance level with just one setting an not be included in every backup job.
• So they both should not be included in the command by default.
• "Checksum" should not be included based on the instance setting.
• They should only be included and set to "Y" if the switch parameter is used.

I am not a fan of using three states of a switch "$true / $false / not used" as it makes things like Install-DbaMaintenanceSolution -Verify:$verify -Checksum:$checksum impossible.

[ReeceGoding]

I largely agree with @andreasjordan; switches should be for off-by-default behaviour. My only disagreement is that I don't have any strong objection to "Checksum" appearing in the job texts. The commit that introduced these parameters looks to have been this one, so perhaps @potatoqualitee can give us some context?

[potatoqualitee]

Thank you both for your patience. I missed the alert. My preference is to follow vendor defaults first, then environmental defaults. No surprises, no overrides unless explicitly set.

Ola defaults both @Verify and @Checksum to 'N', so -Verify and -CheckSum should be opt-in, only passing 'Y' when explicitly specified and omitting them otherwise. -Compress should respect the instance default when not specified, which the PR shows is also broken.

I agree with Reece that the docs should change and opt-in is the right call. I also agree with Andreas that these should not be included in the command by default. The tests will need to be updated to reflect the new correct behavior too. Please do fix then we can merge 👍🏼

[ReeceGoding]

Ola's default is to have Verify and Checksum on. It can be seen here

'EXECUTE [dbo].[DatabaseBackup]' + CHAR(13) + CHAR(10) + '@Databases = ''USER_DATABASES'',' + CHAR(13) + CHAR(10) + CASE WHEN @BackupURL IS NOT NULL THEN '@URL = N''' + REPLACE(@BackupURL,'''','''''') + '''' ELSE '@Directory = ' + ISNULL('N''' + REPLACE(@BackupDirectory,'''','''''') + '''','NULL') END + ',' + CHAR(13) + CHAR(10) + '@BackupType = ''FULL'',' + CHAR(13) + CHAR(10) + '@Verify = ''Y'',' + CHAR(13) + CHAR(10) + '@CleanupTime = ' + ISNULL(CAST(@CleanupTime AS nvarchar),'NULL') + ',' + CHAR(13) + CHAR(10) + '@Checksum = ''Y'',' + CHAR(13) + CHAR(10) + '@LogToTable = ''' + @LogToTable + '''',

If our goal is to both respect his defaults and to have switches only control opt-in behaviour, then we need to get rid of -Verify and make it -NoVerify instead. Same for -Checksum. Can I make that breaking change?

-Compress should not require as much trouble. That just needs to be "don't add it to the job if it is not asked for, but do if it is". However, what would somebody who wants to disable compression do if they do not want to change the instance-level default?

[andreasjordan]

Maybe we can have both -Verify and -NoVerify. If none is used we don't touch the sql and use the defaults from Ola. If one is set, we change the code to force that setting. We can do the same for Checksum and Compression. A lot of parameters, but also a lot of flexibility and a clear functionality.

[ReeceGoding]

We could, but isn't that just an enum with extra steps? There are only four things that anyone could ask for:

• Turn it off explicitly in the job's text
• Turn it on explicitly in the job's text
• Make no change to the job's text (respect Ola's defaults)
• Do not mention it in the job's text, even if Ola does by default (not sure why anyone would want this)

That could easily be a ValidateSet. Something like [ValidateSet('Default', 'ForceOn', 'ForceOff', 'Remove')].

[andreasjordan]

"(not sure why anyone would want this)" - I forgot this case and I'm a fan of only the needed parameters. I prefer to set checksum and compression at the instance level and do not verify after the backup. So I would like to have a job without those parameters.

Mentioning the validateset: So you suggest using string parameters instead of switch parameters? I think that's a good idea.

[ReeceGoding]

Yeah. I can't think of anything better. Should I try making that breaking change?

I wonder if we have enough context for Claude to just do it. It would need the tests from #10184 as a starting point but would also have to re-write them quite heavily.

[andreasjordan]

@claude we want to change the parameters -Compress, -Verifyand -CheckSum to string parameters with [ValidateSet('Default', 'ForceOn', 'ForceOff', 'Remove')] like Reece suggested in #10183 (comment)
Please create a pull request with the needed changes or ask for information that you need first.