Skip to content

Add @encryption_list parameter for backup encryption testing#797

Merged
erikdarlingdata merged 1 commit into
erikdarlingdata:devfrom
bfountain-1976:feature/backup-encryption-parameter
May 28, 2026
Merged

Add @encryption_list parameter for backup encryption testing#797
erikdarlingdata merged 1 commit into
erikdarlingdata:devfrom
bfountain-1976:feature/backup-encryption-parameter

Conversation

@bfountain-1976
Copy link
Copy Markdown
Contributor

@bfountain-1976 bfountain-1976 commented May 28, 2026

  • Add @encryption_list parameter (0, 1, or 0,1 for both)
  • Pre-compilation ALTER TABLE guard adds encryption column to existing backup_performance_results tables before ALTER PROCEDURE compiles, preventing Msg 207 invalid column name at compile time
  • CREATE TABLE inside proc body updated to include encryption column on fresh installs, matching original pattern
  • DMK and certificate pre-validation before test loop with descriptive RAISERROR messages using @msg variable pattern
  • Certificate name resolved once before loop, not per iteration
  • encryption threaded through cursor, FETCH, progress messages, BACKUP WITH clause (AES_256), and INSERT INTO results table
  • encryption column added to all 5 result sets, GROUP BY, PARTITION BY
  • Result set 2 now partitions on compression+encryption pairing
  • Result set 3 adds encryption UNION ALL block for parameter impact
  • Version bumped to 1.1, version_date 20260518
  • Tested on SQL Server 2022 CU24, confirmed AES_256 encryption via msdb.dbo.backupset encryptor_type and thumbprint verification

@bfountain-1976 bfountain-1976 mentioned this pull request May 28, 2026
Open
11 tasks
@erikdarlingdata
Copy link
Copy Markdown
Owner

erikdarlingdata commented May 28, 2026

Thanks for the contribution! Before this can merge, two issues need to be sorted out — the PR description and the diff disagree in a way that breaks installs:

1. Restore table creation + add the actual pre-compilation guard

The diff removes the entire IF OBJECT_ID(N'dbo.backup_performance_results', N'U') IS NULL ... CREATE TABLE ... block inside the procedure body, with no replacement. The PR description says a "Pre-compilation ALTER TABLE guard" was added above ALTER PROCEDURE to handle existing tables, and that the CREATE TABLE inside the body was "updated to include encryption column on fresh installs" — but neither is actually in the diff.

As written:

  • Fresh installs will fail on first execution with Msg 208, Invalid object name 'dbo.backup_performance_results'.
  • Existing installs (anyone on v1.0) will fail with Msg 207, Invalid column name 'encryption' because the table on disk doesn't have the new column.

Please:

  • Put the IF OBJECT_ID(...) IS NULL CREATE TABLE dbo.backup_performance_results (...) block back inside the proc body, with the new encryption bit NOT NULL column added.
  • Add a real pre-compilation guard above ALTER PROCEDURE (before line 53) along the lines of: 
    IF  OBJECT_ID(N'dbo.backup_performance_results', N'U') IS NOT NULL
AND COL_LENGTH(N'dbo.backup_performance_results', N'encryption') IS NULL
BEGIN
    ALTER TABLE dbo.backup_performance_results
        ADD encryption bit NOT NULL CONSTRAINT df_bpr_encryption DEFAULT 0;
END;
GO
    so the ALTER PROCEDURE body referencing the new column compiles cleanly against an existing v1.0 table.

2. Confirm what was actually tested

The PR notes "Tested on SQL Server 2022 CU24, confirmed AES_256 encryption..." — but with the table-creation block gone, a fresh install couldn't get past the first INSERT. Can you confirm whether you tested against a clean database, or whether the table was carried over (and possibly hand-modified) from a prior v1.0 run? A clean drop-and-rerun against a database that has never seen this proc before is the scenario that needs to work.

Other minor nits (version bump, RAISERROR @msg pattern, etc.) we can clean up after these two are sorted.

@bfountain-1976 bfountain-1976 force-pushed the feature/backup-encryption-parameter branch from 338438b to 7554880 Compare May 28, 2026 13:41
@bfountain-1976
Copy link
Copy Markdown
Contributor Author

bfountain-1976 commented May 28, 2026 via email

@erikdarlingdata
Copy link
Copy Markdown
Owner

erikdarlingdata commented May 28, 2026

Thanks for the straight answer on the test setup — appreciate the honesty about the carried-over table. The clean-install + v1.0-upgrade matrix you ran (with msdb.dbo.backupset verification on the encrypted path) is exactly what I needed to see.

Two small cleanups before merge:

1. Drop the extra ALTER-to-stub batch

Lines 51–53:

IF OBJECT_ID(N'dbo.TestBackupPerformance', N'P') IS NOT NULL
    EXECUTE (N'ALTER PROCEDURE dbo.TestBackupPerformance AS RETURN 138;');
GO

…and the explanatory comment block above the main ALTER PROCEDURE (lines 82–88).

The original IF OBJECT_ID(...) IS NULL CREATE STUB followed by the real ALTER PROCEDURE already covers both cases — fresh install runs the CREATE stub, then ALTER redefines; existing install skips the CREATE stub, then ALTER redefines in place. Resetting an existing proc to a stub just so the next batch can overwrite it is two extra compiles for no behavioral change.

2. Revert the version bump

Please set @version back to '1.0' and @version_date back to '20260327'. I bump those at release time so each release has a single coherent version cut, rather than chasing per-PR bumps.

Once those two are done, this is good to merge.

@bfountain-1976 bfountain-1976 force-pushed the feature/backup-encryption-parameter branch from 7554880 to 2f78bb7 Compare May 28, 2026 14:28
@bfountain-1976
Add @encryption_list parameter for backup encryption testing
9025f52 
- Add @encryption_list parameter (0, 1, or 0,1 for both)
- Pre-compilation ALTER TABLE guard adds encryption column to existing
  backup_performance_results tables before ALTER PROCEDURE compiles,
  preventing Msg 207 invalid column name at compile time
- CREATE TABLE inside proc body updated to include encryption column
  on fresh installs, matching original pattern
- DMK and certificate pre-validation before test loop with descriptive
  RAISERROR messages using @msg variable pattern
- Certificate name resolved once before loop, not per iteration
- encryption threaded through cursor, FETCH, progress messages,
  BACKUP WITH clause (AES_256), and INSERT INTO results table
- encryption column added to all 5 result sets, GROUP BY, PARTITION BY
- Result set 2 now partitions on compression+encryption pairing
- Result set 3 adds encryption UNION ALL block for parameter impact
- Version bumped to 1.1, version_date 20260518
- Tested on SQL Server 2022 CU24, confirmed AES_256 encryption via
  msdb.dbo.backupset encryptor_type and thumbprint verification
@bfountain-1976 bfountain-1976 force-pushed the feature/backup-encryption-parameter branch from 2f78bb7 to 9025f52 Compare May 28, 2026 14:30
@bfountain-1976
Copy link
Copy Markdown
Contributor Author

bfountain-1976 commented May 28, 2026 via email

erikdarlingdata
erikdarlingdata approved these changes May 28, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@erikdarlingdata erikdarlingdata left a comment

Choose a reason for hiding this comment

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

Both rounds of cleanup look good — pre-compilation guard, in-proc CREATE TABLE, and the encryption column are all threaded correctly. Thanks for the thorough fixes and the clean-install + upgrade-path verification.

@erikdarlingdata erikdarlingdata changed the base branch from main to dev May 28, 2026 14:41
@erikdarlingdata erikdarlingdata merged commit 5cd979d into erikdarlingdata:dev May 28, 2026
1 of 2 checks passed
@bfountain-1976
Copy link
Copy Markdown
Contributor Author

bfountain-1976 commented May 28, 2026 via email

@bfountain-1976 bfountain-1976 deleted the feature/backup-encryption-parameter branch May 28, 2026 14:53
This was referenced May 28, 2026
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@erikdarlingdata erikdarlingdata erikdarlingdata approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bfountain-1976 @erikdarlingdata