Skip to content

2026-07-02 testing Codex code reviews - #4028

Closed
BrentOzar wants to merge 221 commits into
mainfrom
dev
Closed

2026-07-02 testing Codex code reviews#4028
BrentOzar wants to merge 221 commits into
mainfrom
dev

Conversation

@BrentOzar

Copy link
Copy Markdown
Member

I'm not actually going to merge this yet, but I wanna trigger a Codex code review for the changes in this month's release, since it's pretty wide-ranging.

dave-pendleton and others added 30 commits April 7, 2026 18:57
…s table and its columns to 'MS_Description'. This will allow them to be recognized by SSMS and other property-aware third party tools.
Remove where 1 = 1.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove semicolon even though I think Copilot is drunk here

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Remove extra semicolon

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ed-Properties

Rename the 'Description' extended properties to 'MS_Description'
Ranks all queries (procs and ad-hoc) by how frequently they appear in
deadlocks, with victim vs. survivor breakdown. Groups by sql_handle +
statement offsets from the execution stack, deduplicates to one frame
per participant to avoid double-counting from nested calls. Shows each
query's percentage of total deadlocks with a 10% significance cutoff.

Closes #3899

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…lock-query

sp_BlitzLock: add top deadlock query finding (check 17)
Adding info about how to enable last actual plans.
20260413 readme info about last actual plans
…gh-low-configs

sp_Blitz: exclude SQL Server version-part configs from Non-Default Server Config findings
…-component-warnings

sp_Blitz: stop reporting deprecated FRK components as “Component Outdated”
…a-file-thresholds

sp_Blitz: replace fixed TempDB file-count warning with CPU-relative threshold
…t-server-config-details

sp_Blitz: shorten “Non-Default Server Config” details to start with “Default value:”
erikdarlingdata and others added 21 commits June 2, 2026 13:01
…indings

New @SkipExecutionPlans bit parameter (default 0) lets callers bypass the
execution plans result set and jump straight to the findings rollup.

When set to 1, the client-output path skips building #available_plans /
#dm_exec_query_stats and the sys.dm_exec_text_query_plan calls, and skips
the execution plans result set, returning only the deadlock results and the
findings rollup. Default behavior is unchanged.

Also documented in @Help and surfaced in the @debug parameter dump.

Fixes #4013

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
sp_BlitzLock: add @SkipExecutionPlans to skip plans and return only findings (#4013)
Code:
  - Remove unused @nl variable.
  - Remove unused @ProductMajor variable (no version-dependent branches use it).
  - Quote and normalize @LinkedServer before injection into the 4-part EXEC.
    Strips outer brackets, re-wraps with QUOTENAME so names with periods,
    spaces, or other special characters produce a valid identifier and the
    dynamic SQL can't be tampered with via a maliciously-named linked server.
  - Harden the linked-server CallStack parser. Trim trailing CR/LF/TAB/space
    that some clients append, then only strip the closing "';" if it's
    actually present. If the suffix doesn't match, leave @RemoteXml NULL so
    the existing RemoteEmpty error path fires instead of silently mangling
    the payload.
  - Clarify SpillCount comment: count() returns 0 (not NULL) so the row is
    always emitted; 0 vs 0 matches as "Same" and gets suppressed; non-zero
    deltas surface via the exact-match carve-out in the diff filter.

README:
  - Fix hex literal examples for @QueryPlanHash from 7-byte (14 hex digits)
    to 8-byte (16 hex digits) so the samples run without padding surprises.
  - Drop "parallel index ops" from the sp_configure allowlist list - it's
    not a real sp_configure option and isn't in #ConfigAllowlist.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Keeps it out of Install-All-Scripts.sql and Install-Azure.sql, which
build their merged bundles from sp_*.sql at the repo root only (the
Get-ChildItem call in 'Build Installation Scripts.ps1' is
non-recursive, so anything under OptionalScripts/ is skipped
automatically).

Users who want sp_BlitzPlanCompare install OptionalScripts/sp_BlitzPlanCompare.sql
directly. README now spells that out at the top of the proc's section.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds an Installation callout at the top of the sp_BlitzPlanCompare
section explaining the proc lives under OptionalScripts/ and is not
part of Install-All-Scripts.sql / Install-Azure.sql, so users know
they need to install it explicitly.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Azure Data Studio is retired; SSMS is the only client we should be
pointing users at in fresh docs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…optional

Move sp_BlitzPlanCompare.sql to OptionalScripts/
…kieStore

Fixes #4010. Check 8's "More Info - Query" finding for stored-procedure
deadlocks used to emit an EXECUTE sp_BlitzQueryStore call - that proc is
deprecated and no longer maintained. Per the issue discussion the right
replacement is Erik Darling's sp_QuickieStore
(https://erikdarling.com/sp_quickiestore/), which has the same shape:
a Query Store explorer keyed by database + procedure name.

Mapping:
  sp_BlitzQueryStore @DatabaseName    -> sp_QuickieStore @database_name
  sp_BlitzQueryStore @StoredProcName  -> sp_QuickieStore @procedure_name

The Query Store availability guard (@ProductVersionMajor >= 13 OR @Azure = 1)
stays - sp_QuickieStore also requires Query Store, which is SQL 2016+.

Also updated:
  - the section comment at line 2809
  - the debug RAISERROR message at line 2894
so future readers don't see stale "BlitzQueryStore" references.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot review on PR #4017 flagged that ds.database_name comes from
PARSENAME(ds.proc_name, 3), which returns NULL whenever the proc_name
isn't a 3-part identifier. QUOTENAME(NULL, '''') propagates NULL
through the rest of the string concat, so the entire finding becomes
NULL and the user gets nothing to copy/paste.

Switched to dow.database_name (derived from dow.database_id at
deadlock-capture time, already used 4 lines up for the output's
database_name column). Same data, more reliable source, stays
consistent with the rest of the SELECT.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…erystore

sp_BlitzLock: replace deprecated sp_BlitzQueryStore call with sp_QuickieStore
…ile suffix

Replace PATINDEX('%_[0-9][0-9]%', REVERSE(...)) with a CASE expression that
checks the last underscore position. When the underscore is <=7 chars from the
end (indicating a 1-2 digit split index like _1.bak or _10.bak), strip from
there. Otherwise strip only the file extension (.bak/.trn).

This prevents garbled datetime extraction from non-split filenames where digit
pairs inside the datetime portion (e.g. _190000) were incorrectly matched as
split-file suffixes.

Affected: sp_DatabaseRestore.sql, Install-All-Scripts.sql
…ck, revert Install-All-Scripts.sql

- Remove stale PATINDEX line in _[0-9][0-9].bak @StoPAt DELETE block that was
  left alongside the new CASE WHEN line, causing a T-SQL parse error (AND after ;)
- Drop the non-striped .bak DELETE block added by mistake: it reintroduces the
  same broken PATINDEX pattern and hardcodes .bak instead of @FileExtensionBak;
  not among the 13 locations listed in #4019
- Revert Install-All-Scripts.sql to pre-PR state — it is auto-generated by the
  build process and will pick up the sp_DatabaseRestore.sql fix after merge
…tion

Fix DIFF backup silently skipped when filename has no numeric split-file suffix
Fixes from the Copilot review on #4002:

- Skip the timing recalc when NodeId values are not unique across the
  document. NodeId is only unique per statement, so on multi-statement
  plans //p:RelOp[@nodeid=...] would blend operators from different
  statements and corrupt the rewritten timings. Detected via
  count(distinct-values(//p:RelOp/@nodeid)) vs count(//p:RelOp).
  Attribute/element stripping and whitespace minification still apply.

- NULL-safe SqlHandle/QueryHash matching when writing ai_query_plan
  back to ##BlitzCacheProcs. Parent procedure/batch rows carry a NULL
  QueryHash, so the previous equality match silently discarded their
  minimized text.

- Aggregate ActualExecutions (summed across threads) into the
  synthesized per-operator summary instead of hardcoding 1, preserving
  the how-many-times-did-this-run signal for nested loop inner sides.

- Count failed .modify() steps per plan and report via RAISERROR when
  @debug >= 1 instead of swallowing them invisibly.

- Document why the 500-RelOp recalc cap exists and what plans over the
  cap still get.

- Extend the @reanalyze schema-drift guard to all five ai_* columns,
  not just ai_query_plan.

Verified on Azure SQL DB: full-proc run shows parent rows (NULL
QueryHash) now get ai_query_plan populated and multi-statement proc
plans keep their original per-thread blocks; standalone harness run of
the verbatim minimizer block confirms ActualExecutions="8" (3+5 across
threads) on a DOP-2 nested-loop inner side, CPU summed, elapsed maxed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Per Copilot round-two review on #4002. (SqlHandle, QueryHash) is not
unique in ##BlitzCacheProcs when the same query hash has multiple
distinct plans, so the minimizer's UPDATE stamped the last-fetched
plan's minimized text onto every row sharing the pair. Add PlanHandle
to the cursor key, both FETCH statements, and the NULL-safe UPDATE
predicate, mirroring the AI-call cursor's matching.

Verified on Azure with two rows sharing (SqlHandle, QueryHash) but
different PlanHandles and distinguishable plans: each row's
ai_query_plan now contains its own plan's operators only.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…-1a291a

sp_BlitzCache: minimize query plan XML sent to AI (#3862)
For latest CUs and GDRs, prepping for release.
@BrentOzar BrentOzar self-assigned this Jul 2, 2026
Copilot AI review requested due to automatic review settings July 2, 2026 12:22
@BrentOzar BrentOzar added this to the 2026-07 Release milestone Jul 2, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@BrentOzar

Copy link
Copy Markdown
Member Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0c1340da82

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread sp_DatabaseRestore.sql
WHERE BackupFile LIKE N'%[_][0-9][0-9].bak'
WHERE BackupFile LIKE N'%[_][0-9][0-9].' + @FileExtensionBak
AND BackupFile LIKE N'%' + @Database + N'%'
AND (REPLACE( RIGHT( REPLACE( BackupFile, RIGHT( BackupFile, CASE WHEN PATINDEX( '%[_]%', REVERSE( BackupFile ) ) <= 7 THEN PATINDEX( '%[_]%', REVERSE( BackupFile ) ) ELSE CHARINDEX( '.', REVERSE( BackupFile ) ) END ), '' ), 18 ), '_', '' ) > @StopAt);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Remove the extra terminated predicate

This new predicate ends the DELETE statement with a semicolon, leaving the following AND (...) as a standalone statement. The procedure body will fail to parse with a syntax error near AND during ALTER PROCEDURE, regardless of whether @StopAt is used at runtime.

Useful? React with 👍 / 👎.

Comment thread Documentation/Development/Build Installation Scripts.ps1
Comment thread sp_BlitzFirst.sql
Comment thread sp_BlitzWho.sql
r.blocking_session_id <> 0
OR blocked.session_id IS NOT NULL
OR wt.wait_info IS NOT NULL
OR (s.status = ''sleeping'' AND COALESCE(s.open_transaction_count, r.open_transaction_count, blocked.open_tran, 0) >= 1)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Include sleeping problem sessions before filtering

With @OnlyProblems = 1 and the default @ShowSleepingSPIDs = 0 path used by sp_BlitzFirst @ExpertMode = 3, sleeping sessions with open transactions are already removed by the earlier @ShowSleepingSPIDs predicate before this new OR can match them. That makes the advertised “sleeping with an open transaction” problem case invisible unless callers also know to enable sleeping SPIDs, so this predicate should either bypass or adjust the earlier sleeping-session filter.

Useful? React with 👍 / 👎.

IF @DatabaseName IS NULL OR @DatabaseName = @CurrentDbName
BEGIN
/* Current-DB resolution. OBJECT_ID parses bare / 2-part / 3-part. */
SET @ResolvedProcObjectId = OBJECT_ID(@StoredProcName);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Scope three-part procedure lookups

When a caller follows the help text and passes [db].[schema].[proc] without also setting @DatabaseName, this resolves the object_id but leaves @DatabaseName NULL, so the later plan-cache predicate filters only on st.objectid. Object IDs are unique only within a database, so a cached plan from another database with the same object_id can be selected or create spurious ambiguity; parse the database from the three-part name or add the dbid predicate for that case.

Useful? React with 👍 / 👎.

Comment thread sp_DatabaseRestore.sql
a period, space, or other special character still produce a valid identifier
and the dynamic SQL can't be tampered with via a maliciously-named linked server. */
DECLARE @LinkedServerSafe SYSNAME = REPLACE(REPLACE(@LinkedServer, N'[', N''), N']', N'');
SET @sql = N'EXEC ' + QUOTENAME(@LinkedServerSafe) + N'.master.dbo.sp_BlitzPlanCompare @QueryPlanHash = @hash;';

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Pass the database scope to the remote compare

In linked-server mode, even when the local lookup was scoped with @DatabaseName, this remote call sends only the local query hash and drops that database scope. If the same query text exists in multiple databases on the remote server, the remote side can return an ambiguity error or compare against a plan from the wrong database; include the requested database scope in the remote invocation.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.