From 49cc31edb9a07554ecb4f21b6009a4abc2b1d00f Mon Sep 17 00:00:00 2001 From: Erik Darling <2136037+erikdarlingdata@users.noreply.github.com> Date: Sat, 18 Jul 2026 09:24:10 -0400 Subject: [PATCH] sp_PerfCheck: cut noise findings and flag bad config, not changed config The check set drowned real findings in priority-50 checklist noise and had some logic backwards. On a normal server this cut a 27-finding run to 10, all meaningful. Deleted, none of which indicate a problem: - 7005 ANSI Settings Require Review (fired on every database; the SESSION SET options the driver sends are what matter, not database-level ANSI) - 7007 Non-Default Target Recovery Time - 4107 Resource Governor Enabled (bare "it is on" informational) - 1000 Non-Default Configuration, the generic "changed from default" list -- it flagged correct MAXDOP / cost threshold / max memory values as findings Inverted the cost-threshold check (1004) to flag the BAD value instead of the changed one: it now fires when CTFP is under 50, escalating to High (20) at or below the terrible default of 5 and staying Low (40) between 6 and 49. Kept 7103 (log growth <> 64 MB) -- it looks like a nitpick but is a real SQL 2022 check: log auto-growth only gets instant file initialization at 64 MB. Reworked the harness config test to match: it now forces CTFP sane / default / low and asserts absence, presence, and the priority escalation bidirectionally. Co-Authored-By: Claude Opus 4.8 --- sp_PerfCheck/sp_PerfCheck.sql | 207 +++----------------------------- sp_PerfCheck/tests/run_tests.py | 174 +++++++++++++-------------- 2 files changed, 103 insertions(+), 278 deletions(-) diff --git a/sp_PerfCheck/sp_PerfCheck.sql b/sp_PerfCheck/sp_PerfCheck.sql index 86c10332..02b06787 100644 --- a/sp_PerfCheck/sp_PerfCheck.sql +++ b/sp_PerfCheck/sp_PerfCheck.sql @@ -1314,38 +1314,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. N'Resource Governor', N'Enabled'; - /* Add informational message about Resource Governor with query suggestion */ - INSERT INTO - #results - ( - check_id, - priority, - category, - finding, - details, - url - ) - SELECT - check_id = 4107, - priority = 50, /* Informational: may be intentional */ - category = N'Resource Governor', - finding = N'Resource Governor Enabled', - details = - N'Resource Governor is enabled on this instance. This affects workload resource allocation and may ' + - N'impact performance by limiting resources available to various workloads. ' + - N'For more details, run these queries to explore your configuration:' + NCHAR(13) + NCHAR(10) + - N'/* Resource Governor configuration */' + NCHAR(13) + NCHAR(10) + - N'SELECT c.* FROM sys.resource_governor_configuration AS c;' + NCHAR(13) + NCHAR(10) + - N'/* Resource pools and their settings */' + NCHAR(13) + NCHAR(10) + - N'SELECT p.* FROM sys.dm_resource_governor_resource_pools AS p;' + NCHAR(13) + NCHAR(10) + - N'/* Workload groups and their settings */' + NCHAR(13) + NCHAR(10) + - N'SELECT wg.* FROM sys.dm_resource_governor_workload_groups AS wg;' + NCHAR(13) + NCHAR(10) + - N'/* Classifier function (if configured) */' + NCHAR(13) + NCHAR(10) + - N'SELECT cf.* FROM sys.resource_governor_configuration AS gc' + NCHAR(13) + NCHAR(10) + - N'CROSS APPLY (SELECT OBJECT_NAME(gc.classifier_function_id) AS classifier_function_name) AS cf;', - url = N'https://erikdarling.com/sp_perfcheck/#ResourceGovernor' - FROM sys.resource_governor_configuration AS rgc - WHERE rgc.is_enabled = 1; END ELSE BEGIN @@ -3194,87 +3162,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. */ IF @azure_sql_db = 0 /* Skip these checks for Azure SQL DB */ BEGIN - /* Check for non-default configuration values */ - INSERT INTO - #results - ( - check_id, - priority, - category, - finding, - object_name, - details, - url - ) - SELECT - check_id = 1000, - priority = 50, /* Informational: non-default config */ - category = N'Server Configuration', - /* - Stable label; the setting name that varies per row goes in - object_name so this finding groups and the setting is filterable. - */ - finding = N'Non-Default Configuration', - object_name = c.name, - details = - N'Configuration option "' + c.name + - N'" has been changed from the default. Current: ' + - CONVERT(nvarchar(50), c.value_in_use) + - CASE - /* Configuration options from your lists */ - WHEN c.name = N'access check cache bucket count' THEN N', Default: 0' - WHEN c.name = N'access check cache quota' THEN N', Default: 0' - WHEN c.name = N'Ad Hoc Distributed Queries' THEN N', Default: 0' - WHEN c.name = N'ADR cleaner retry timeout (min)' THEN N', Default: 120' - WHEN c.name = N'ADR Cleaner Thread Count' THEN N', Default: 1' - WHEN c.name = N'ADR Preallocation Factor' THEN N', Default: 4' - WHEN c.name = N'affinity mask' THEN N', Default: 0' - WHEN c.name = N'affinity I/O mask' THEN N', Default: 0' - WHEN c.name = N'affinity64 mask' THEN N', Default: 0' - WHEN c.name = N'affinity64 I/O mask' THEN N', Default: 0' - WHEN c.name = N'cost threshold for parallelism' THEN N', Default: 5' - WHEN c.name = N'max degree of parallelism' THEN N', Default: 0' - WHEN c.name = N'max server memory (MB)' THEN N', Default: 2147483647' - WHEN c.name = N'max worker threads' THEN N', Default: 0' - WHEN c.name = N'min memory per query (KB)' THEN N', Default: 1024' - WHEN c.name = N'min server memory (MB)' THEN N', Default: 0' - WHEN c.name = N'optimize for ad hoc workloads' THEN N', Default: 0' - WHEN c.name = N'priority boost' THEN N', Default: 0' - WHEN c.name = N'query governor cost limit' THEN N', Default: 0' - WHEN c.name = N'recovery interval (min)' THEN N', Default: 0' - WHEN c.name = N'tempdb metadata memory-optimized' THEN N', Default: 0' - WHEN c.name = N'lightweight pooling' THEN N', Default: 0' - ELSE N', Default: Unknown' - END, - url = N'https://erikdarling.com/sp_perfcheck/#ServerSettings' - FROM sys.configurations AS c - WHERE - /* Access check cache settings */ - (c.name = N'access check cache bucket count' AND c.value_in_use <> 0) - OR (c.name = N'access check cache quota' AND c.value_in_use <> 0) - OR (c.name = N'Ad Hoc Distributed Queries' AND c.value_in_use <> 0) - /* ADR settings */ - OR (c.name = N'ADR cleaner retry timeout (min)' AND c.value_in_use NOT IN (0, 15, 120)) - OR (c.name = N'ADR Cleaner Thread Count' AND c.value_in_use <> 1) - OR (c.name = N'ADR Preallocation Factor' AND c.value_in_use NOT IN (0, 4)) - /* - Affinity masks (1008-1011), priority boost (1005), and lightweight - pooling (1006) have their own dedicated checks that fire on exactly - the same non-default condition, so they are excluded here - otherwise - each would be reported twice, once generically and once specifically. - */ - /* Common performance settings */ - OR (c.name = N'cost threshold for parallelism' AND c.value_in_use <> 5) - OR (c.name = N'max degree of parallelism' AND c.value_in_use <> 0) - OR (c.name = N'max server memory (MB)' AND c.value_in_use <> 2147483647) - OR (c.name = N'max worker threads' AND c.value_in_use <> 0) - OR (c.name = N'min memory per query (KB)' AND c.value_in_use <> 1024) - OR (c.name = N'min server memory (MB)' AND c.value_in_use NOT IN (0, 16)) - OR (c.name = N'optimize for ad hoc workloads' AND c.value_in_use <> 0) - OR (c.name = N'query governor cost limit' AND c.value_in_use <> 0) - OR (c.name = N'recovery interval (min)' AND c.value_in_use <> 0) - OR (c.name = N'tempdb metadata memory-optimized' AND c.value_in_use <> 0); - /* TempDB Configuration Checks (not applicable to Azure SQL DB) */ @@ -3721,8 +3608,14 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. ); END; - /* Cost Threshold for Parallelism check */ - IF @cost_threshold <= 5 + /* + Cost threshold for parallelism set too low. The default of 5 is far too + low for modern hardware: it shoves trivial queries into parallel plans, + burning CPU and worker threads on work that runs faster single-threaded. + Flag anything under 50 (a sane starting point), escalating when it is + still at or near the terrible default. + */ + IF @cost_threshold < 50 BEGIN INSERT INTO #results @@ -3737,12 +3630,18 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. VALUES ( 1004, - 40, /* Low: config recommendation */ + CASE + WHEN @cost_threshold <= 5 + THEN 20 /* High: still at or near the default of 5 */ + ELSE 40 /* Low: configured, but lower than recommended */ + END, N'Server Configuration', - N'Low Cost Threshold for Parallelism', + N'Cost Threshold for Parallelism Too Low', N'Cost threshold for parallelism is set to ' + CONVERT(nvarchar(10), @cost_threshold) + - N'. Low values can cause excessive parallelism for small queries.', + N'. Low values push trivial queries into parallel plans, wasting CPU ' + + N'and worker threads on work that runs faster single-threaded. A ' + + N'starting point of 50 is far more reasonable than the default of 5.', N'https://erikdarling.com/sp_perfcheck/#CostThreshold' ); END; @@ -4448,51 +4347,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. OR d.is_auto_update_stats_on = 0 ); - /* Check ANSI settings that might cause issues */ - INSERT INTO - #results - ( - check_id, - priority, - category, - finding, - database_name, - details, - url - ) - SELECT - check_id = 7005, - priority = 50, /* Informational */ - category = N'Database Configuration', - finding = N'ANSI Settings Require Review', - database_name = d.name, - details = - N'One or more ANSI settings differ from recommended best practices: ' + - CASE WHEN d.is_ansi_null_default_on = 0 THEN N'ANSI_NULL_DEFAULT OFF (recommended ON), ' ELSE N'' END + - CASE WHEN d.is_ansi_nulls_on = 0 THEN N'ANSI_NULLS OFF (recommended ON), ' ELSE N'' END + - CASE WHEN d.is_ansi_padding_on = 0 THEN N'ANSI_PADDING OFF (recommended ON), ' ELSE N'' END + - CASE WHEN d.is_ansi_warnings_on = 0 THEN N'ANSI_WARNINGS OFF (recommended ON), ' ELSE N'' END + - CASE WHEN d.is_arithabort_on = 0 THEN N'ARITHABORT OFF (recommended ON in many contexts), ' ELSE N'' END + - CASE WHEN d.is_concat_null_yields_null_on = 0 THEN N'CONCAT_NULL_YIELDS_NULL OFF (recommended ON), ' ELSE N'' END + - CASE WHEN d.is_numeric_roundabort_on = 1 THEN N'NUMERIC_ROUNDABORT ON (recommended OFF), ' ELSE N'' END + - CASE WHEN d.is_quoted_identifier_on = 0 THEN N'QUOTED_IDENTIFIER OFF (recommended ON), ' ELSE N'' END + - N'These settings may lead to inconsistent behavior, reduced feature compatibility, or unexpected query results ' + - N'if they do not align with recommended best practices.', - url = N'https://erikdarling.com/sp_perfcheck/#ANSISettings' - FROM #databases AS d - WHERE d.database_id = @current_database_id - AND - ( - d.is_ansi_null_default_on = 0 - OR d.is_ansi_nulls_on = 0 - OR d.is_ansi_padding_on = 0 - OR d.is_ansi_warnings_on = 0 - OR d.is_arithabort_on = 0 - OR d.is_concat_null_yields_null_on = 0 - OR d.is_numeric_roundabort_on = 1 - OR d.is_quoted_identifier_on = 0 - ); - /* Check Query Store Status */ INSERT INTO #results @@ -4889,33 +4743,6 @@ WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. END; END CATCH; - /* Check for non-default target recovery time */ - INSERT INTO - #results - ( - check_id, - priority, - category, - finding, - database_name, - details, - url - ) - SELECT - check_id = 7007, - priority = 50, /* Informational */ - category = N'Database Configuration', - finding = N'Non-Default Target Recovery Time', - database_name = d.name, - details = - N'Database target recovery time is ' + - CONVERT(nvarchar(20), d.target_recovery_time_in_seconds) + - N' seconds, which differs from the default of 60 seconds. This affects checkpoint frequency and recovery time.', - url = N'https://erikdarling.com/sp_perfcheck/#RecoveryTime' - FROM #databases AS d - WHERE d.database_id = @current_database_id - AND d.target_recovery_time_in_seconds <> 60; - /* Check transaction durability */ INSERT INTO #results diff --git a/sp_PerfCheck/tests/run_tests.py b/sp_PerfCheck/tests/run_tests.py index 97db6da9..9b132fc2 100644 --- a/sp_PerfCheck/tests/run_tests.py +++ b/sp_PerfCheck/tests/run_tests.py @@ -22,10 +22,11 @@ * Forced-condition (the high-value core): conditions this harness can safely create and reset, proven BIDIRECTIONALLY (finding absent when the condition is not present, finding present when it is): - - Server config: the "Non-Default Configuration" check (check_id 1000) - reads sys.configurations. For a small set of SAFE options the harness - forces each to a non-default value and back, and names the option in the - details. Every option's original value_in_use is captured first and + - Server config: the "Cost Threshold for Parallelism Too Low" check + (check_id 1004) reads sys.configurations. The harness forces CTFP to a + sane value (absent), the default of 5 (present, High), and a low-but- + not-default value (present, Low), proving both the finding and its + severity escalation. The original value_in_use is captured first and restored precisely in a finally block that runs even on assertion failure. The instance is never left reconfigured. - Database config: Auto-Shrink Enabled (7001) and Auto Update Statistics @@ -54,18 +55,15 @@ import sys -# The "Non-Default Configuration" check (check_id 1000) fires when an option's -# value_in_use differs from the listed default. These three are SAFE to flip on -# a test instance: none require a restart, none are dangerous (no max server -# memory, no affinity), and nothing the CI does depends on them. Each is: -# (option name, default value, forced non-default value) -# All three are "advanced" options, so 'show advanced options' must be 1 while -# they are configured; the harness sets it and restores it. -FORCED_OPTIONS = [ - ("cost threshold for parallelism", 5, 55), - ("optimize for ad hoc workloads", 0, 1), - ("access check cache bucket count", 0, 256), -] +# The Cost Threshold for Parallelism check (check_id 1004) fires when CTFP is +# set below 50, escalating to High (priority 20) at or below the default of 5 +# and staying Low (priority 40) between 6 and 49. CTFP is an advanced option +# that needs no restart, so it is safe to flip on a test instance; the harness +# sets 'show advanced options' while configuring it and restores both. +CTFP = "cost threshold for parallelism" +CTFP_SANE = 55 # >= 50: no finding +CTFP_DEFAULT = 5 # bad, and at the default: finding at High priority (20) +CTFP_LOW = 25 # bad, below 50 but above the default: finding at Low (40) # Column names expected in the #results header (shape-regression guard). RESULTS_COLUMNS = [ @@ -209,13 +207,11 @@ def parse_result_rows(stdout): return rows -def find_config_finding(rows, option_name): - """Return the #results row for a Non-Default Configuration finding on the - given option, or None. finding is a stable label; the option name lives in - object_name (column 6), so matching here also proves that convention holds.""" +def find_ctfp_finding(rows): + """Return the #results row for the Cost Threshold for Parallelism Too Low + finding (check_id 1004), or None.""" for r in rows: - if (r[4] == "Non-Default Configuration" - and len(r) > 6 and r[6] == option_name): + if r[4] == "Cost Threshold for Parallelism Too Low": return r return None @@ -314,8 +310,10 @@ def structural_tests(server, password, R): def forced_condition_tests(server, password, R): - """Bidirectional forced-condition assertions on the check_id 1000 - Non-Default Configuration check. Captures and restores exact originals.""" + """Bidirectional forced-condition assertions on the check_id 1004 Cost + Threshold for Parallelism check: ABSENT when CTFP is sane (>= 50), PRESENT + and High at the default of 5, PRESENT but only Low between 6 and 49. + Captures and restores the original value.""" # Flush any pre-existing pending config change (value <> value_in_use) before # baselining. Some images ship one: the SQL Server 2017 CI container has # 'clr strict security' configured on but not yet in use. The harness's own @@ -327,82 +325,82 @@ def forced_condition_tests(server, password, R): # Snapshot the entire config before touching anything. before = snapshot_config(server, password) - # Capture originals: 'show advanced options' plus every forced option. saw_advanced = get_value_in_use(server, password, "show advanced options") - originals = {} - for (name, _default, _forced) in FORCED_OPTIONS: - originals[name] = get_value_in_use(server, password, name) + original_ctfp = get_value_in_use(server, password, CTFP) def restore_all(): - # Restore every touched option to its captured original, then restore - # 'show advanced options'. One RECONFIGURE at the end promotes them all. - for (name, _d, _f) in FORCED_OPTIONS: - _sqlcmd(server, password, - "SET NOCOUNT ON; EXECUTE sys.sp_configure '%s', %d; RECONFIGURE;" - % (_esc(name), originals[name])) + # Restore CTFP, then 'show advanced options'. RECONFIGURE promotes both. + _sqlcmd(server, password, + "SET NOCOUNT ON; EXECUTE sys.sp_configure '%s', %d; RECONFIGURE;" + % (_esc(CTFP), original_ctfp)) _sqlcmd(server, password, "SET NOCOUNT ON; EXECUTE sys.sp_configure 'show advanced options', %d; RECONFIGURE;" % saw_advanced) + grp = "Config[cost threshold for parallelism]" try: - # All chosen options are advanced; make sure they are configurable. err = set_option(server, password, "show advanced options", 1) R.check("Config", "setup: 'show advanced options' configurable", not err, str(err)) - for (name, default_value, forced_value) in FORCED_OPTIONS: - grp = "Config[%s]" % name - - # ---- ABSENT at the default value ---------------------------- - out, e = run_perfcheck_with_option(server, password, name, default_value) - combined = out + "\n" + e - R.check(grp, "no severe error on default-value run", - not find_sql_errors(combined), str(find_sql_errors(combined))) - # positive control: the check machinery actually ran and produced - # output, so "absent" is not a vacuous pass. - R.check(grp, "positive control: #server_info populated at default", - SERVER_INFO_MARKER in out, "Run Date not found") - rows = parse_result_rows(out) - R.check(grp, "finding ABSENT when option = default (%d)" % default_value, - find_config_finding(rows, name) is None, - "unexpected finding present at default") - - # ---- PRESENT when forced to a non-default value ------------- - out, e = run_perfcheck_with_option(server, password, name, forced_value) - combined = out + "\n" + e - R.check(grp, "no severe error on forced-value run", - not find_sql_errors(combined), str(find_sql_errors(combined))) - rows = parse_result_rows(out) - row = find_config_finding(rows, name) - R.check(grp, "finding PRESENT when option = forced (%d)" % forced_value, - row is not None, "finding not emitted when forced") - if row is not None: - well_formed = (row[0] == "1000" and row[1] == "50" - and row[3] == "Server Configuration" - and row[4] == "Non-Default Configuration" - and row[6] == name) - R.check(grp, "forced finding row well-formed " - "(stable finding, setting name in object_name)", - well_formed, - "check_id=%s finding=%r object_name=%r" - % (row[0], row[4], row[6])) - details = row[7] if len(row) > 7 else "" - names_option = (name in details - and ("Current: %d" % forced_value) in details) - R.check(grp, "forced finding details names the option and value", - names_option, "details=%r" % details[:160]) - else: - R.check(grp, "forced finding row well-formed " - "(check_id 1000, priority 50, Server Configuration)", - False, "no row to inspect") - R.check(grp, "forced finding details names the option and value", - False, "no row to inspect") - - # ---- Restore THIS option to its exact original -------------- - set_option(server, password, name, originals[name]) - now = get_value_in_use(server, password, name) - R.check(grp, "option restored to original value_in_use (%d)" % originals[name], - now == originals[name], "value_in_use is now %d" % now) + # ---- ABSENT when CTFP is sane (>= 50) ------------------------------- + out, e = run_perfcheck_with_option(server, password, CTFP, CTFP_SANE) + combined = out + "\n" + e + R.check(grp, "no severe error on sane-value run", + not find_sql_errors(combined), str(find_sql_errors(combined))) + # positive control: the check machinery ran and produced output, so + # "absent" is not a vacuous pass. + R.check(grp, "positive control: #server_info populated at sane value", + SERVER_INFO_MARKER in out, "Run Date not found") + rows = parse_result_rows(out) + R.check(grp, "finding ABSENT when CTFP >= 50 (%d)" % CTFP_SANE, + find_ctfp_finding(rows) is None, + "unexpected CTFP finding at a sane value") + + # ---- PRESENT and HIGH at the default of 5 -------------------------- + out, e = run_perfcheck_with_option(server, password, CTFP, CTFP_DEFAULT) + combined = out + "\n" + e + R.check(grp, "no severe error on default-value run", + not find_sql_errors(combined), str(find_sql_errors(combined))) + rows = parse_result_rows(out) + row = find_ctfp_finding(rows) + R.check(grp, "finding PRESENT when CTFP = default (%d)" % CTFP_DEFAULT, + row is not None, "finding not emitted at the default") + if row is not None: + well_formed = (row[0] == "1004" and row[1] == "20" + and row[3] == "Server Configuration" + and row[4] == "Cost Threshold for Parallelism Too Low") + R.check(grp, "default finding well-formed " + "(check_id 1004, High priority 20, Server Configuration)", + well_formed, + "check_id=%s priority=%s finding=%r" + % (row[0], row[1], row[4])) + details = row[7] if len(row) > 7 else "" + R.check(grp, "default finding details name the value", + ("set to %d" % CTFP_DEFAULT) in details, + "details=%r" % details[:160]) + else: + R.check(grp, "default finding well-formed " + "(check_id 1004, High priority 20, Server Configuration)", + False, "no row to inspect") + R.check(grp, "default finding details name the value", + False, "no row to inspect") + + # ---- PRESENT but only LOW between 6 and 49 ------------------------- + out, e = run_perfcheck_with_option(server, password, CTFP, CTFP_LOW) + rows = parse_result_rows(out) + row = find_ctfp_finding(rows) + R.check(grp, "finding PRESENT when CTFP low-but-not-default (%d)" % CTFP_LOW, + row is not None, "finding not emitted at a low value") + R.check(grp, "priority is Low (40), not High, when CTFP is 6-49", + row is not None and row[1] == "40", + "priority=%s" % (row[1] if row else "no row")) + + # ---- Restore CTFP to its exact original ---------------------------- + set_option(server, password, CTFP, original_ctfp) + now = get_value_in_use(server, password, CTFP) + R.check(grp, "option restored to original value_in_use (%d)" % original_ctfp, + now == original_ctfp, "value_in_use is now %d" % now) finally: # Safety net: restore everything even if an exception was raised. restore_all()