diff --git a/.github/workflows/pr-profiler-report.yml b/.github/workflows/pr-profiler-report.yml index d14dd5770..1e0130d2d 100644 --- a/.github/workflows/pr-profiler-report.yml +++ b/.github/workflows/pr-profiler-report.yml @@ -1,7 +1,10 @@ name: PR Performance Report -# Privileged reporting only. No PR checkout, builds, or artifact execution here. +# Same-repo PRs may exercise their formatter directly. Forks use trusted base code. on: + pull_request: + branches: [main] + types: [opened, synchronize, reopened, ready_for_review] pull_request_target: branches: [main] types: [opened, synchronize, reopened, ready_for_review] @@ -16,12 +19,17 @@ concurrency: jobs: report: + if: >- + (github.event_name == 'pull_request' && + github.event.pull_request.head.repo.full_name == github.repository) || + (github.event_name == 'pull_request_target' && + github.event.pull_request.head.repo.full_name != github.repository) runs-on: ubuntu-latest timeout-minutes: 230 steps: - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 with: - ref: ${{ github.event.pull_request.base.sha }} + ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.event.pull_request.base.sha }} persist-credentials: false - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 with: diff --git a/eng/pipelines/pr-validation-pipeline.yml b/eng/pipelines/pr-validation-pipeline.yml index 99507c8e6..b3461d8a5 100644 --- a/eng/pipelines/pr-validation-pipeline.yml +++ b/eng/pipelines/pr-validation-pipeline.yml @@ -13,6 +13,7 @@ variables: jobs: - job: CodeQLAnalysis displayName: 'CodeQL Security Analysis' + condition: false pool: vmImage: 'ubuntu-latest' @@ -51,6 +52,7 @@ jobs: - job: pytestonwindows displayName: 'Windows x64' + condition: false timeoutInMinutes: 160 pool: vmImage: 'windows-latest' @@ -558,6 +560,7 @@ jobs: - job: PytestOnMacOS displayName: 'macOS x86_64' + condition: false timeoutInMinutes: 90 pool: vmImage: 'macos-latest' @@ -752,22 +755,6 @@ jobs: sqlServerImage: 'mcr.microsoft.com/mssql/server:2025-latest' useAzureSQL: 'false' profilerLeg: 'Linux-SQL2025' - ${{ if ne(variables['AZURE_CONNECTION_STRING'], '') }}: - Ubuntu_AzureSQL: - dockerImage: 'ubuntu:24.04' - distroName: 'Ubuntu-AzureSQL' - sqlServerImage: '' - useAzureSQL: 'true' - Debian: - dockerImage: 'debian:12' - distroName: 'Debian' - sqlServerImage: 'mcr.microsoft.com/mssql/server:2022-latest' - useAzureSQL: 'false' - Debian_SQL2025: - dockerImage: 'debian:12' - distroName: 'Debian-SQL2025' - sqlServerImage: 'mcr.microsoft.com/mssql/server:2025-latest' - useAzureSQL: 'false' steps: - checkout: self @@ -1061,6 +1048,7 @@ jobs: - job: PytestOnLinux_ARM64 displayName: 'Linux ARM64' + condition: false pool: vmImage: 'ubuntu-latest' @@ -1300,6 +1288,7 @@ jobs: - job: PytestOnLinux_RHEL9 displayName: 'Linux RedHat x86_64' + condition: false pool: vmImage: 'ubuntu-latest' @@ -1501,6 +1490,7 @@ jobs: - job: PytestOnLinux_RHEL9_ARM64 displayName: 'Linux RedHat ARM64' + condition: false pool: vmImage: 'ubuntu-latest' @@ -1717,6 +1707,7 @@ jobs: - job: PytestOnLinux_Alpine displayName: 'Linux Alpine x86_64' + condition: false pool: vmImage: 'ubuntu-latest' @@ -1953,6 +1944,7 @@ jobs: - job: PytestOnLinux_Alpine_ARM64 displayName: 'Linux Alpine ARM64' + condition: false pool: vmImage: 'ubuntu-latest' @@ -2421,6 +2413,7 @@ jobs: - job: CodeCoverageReport displayName: 'Full Code Coverage Report in Ubuntu x86_64' + condition: false pool: vmImage: 'ubuntu-latest' diff --git a/eng/profiler_benchmarks/README.md b/eng/profiler_benchmarks/README.md index 8192be78b..94a5ab876 100644 --- a/eng/profiler_benchmarks/README.md +++ b/eng/profiler_benchmarks/README.md @@ -30,10 +30,10 @@ Partial results never produce a verdict. Two environments publish raw samples: Unix on Ubuntu with SQL Server 2022/2025. Routine Windows and macOS profiling is intentionally excluded because neutral PRs showed platform variance above the regression threshold, while both platforms -remain covered by functional CI. The privileged publisher runs -trusted base code, selects the exact PR-head ADO build, and validates bounded -artifacts as data. It publishes as soon as both profiler artifacts exist, -without waiting for unrelated matrix legs. After build completion, missing +remain covered by functional CI. Same-repository PRs run their formatter directly; +fork PRs retain the trusted-base publisher. Both select the exact PR-head ADO build +and validate bounded artifacts as data. Publication begins as soon as both profiler +artifacts exist, without waiting for unrelated matrix legs. After build completion, missing artifacts receive a two-minute propagation grace before a partial result is published. A failed aggregate build can still publish usable profiler artifacts. Exact-head reports may finalize after merge; stale heads are ignored. Missing, diff --git a/eng/profiler_benchmarks/report.py b/eng/profiler_benchmarks/report.py index cfe19bb88..6188894cc 100644 --- a/eng/profiler_benchmarks/report.py +++ b/eng/profiler_benchmarks/report.py @@ -42,6 +42,7 @@ MAX_BYTES = 8 * 1024 * 1024 MAX_COMMENT_CHARS = 60000 MAX_DIAGNOSTIC_ROWS = 20 +MAX_FINGERPRINT_TASKS = 4 MARKER = "" THRESHOLD = 0.20 MIN_DELTA_MS = 1.0 @@ -376,52 +377,43 @@ def render(reports, head, build_id, issues=()): ] missing = len(LEGS) - len(completed) - if len(regressions) == 1: - leg, row = regressions[0] - opening = ( - f"This PR consistently slows {TASK_NAMES[row['name']].lower()} on " - f"{environment_name(leg)} by {row['change_pct']:.1f}%." - ) - elif regressions: + highlighted = [ + (leg, row) for leg, (_, rows) in completed.items() for row in rows if row["status"] != "ok" + ] + if regressions: tasks = len({row["name"] for _, row in regressions}) environments = len({leg for leg, _ in regressions}) opening = ( - f"This PR has {len(regressions)} consistent slowdown signals across " - f"{tasks} database tasks and {environments} environments." + f"{tasks} database task{'s' if tasks != 1 else ''} consistently slowed down across " + f"{environments} measured environment{'s' if environments != 1 else ''}." ) + verdict = "⚠️ Performance regression detected" elif noisy: - if len(noisy) == 1: - leg, row = noisy[0] - opening = ( - f"{TASK_NAMES[row['name']]} was slower on {environment_name(leg)}, " - "but the repeated comparisons were inconsistent." - ) - else: - tasks = len({row["name"] for _, row in noisy}) - environments = len({leg for leg, _ in noisy}) - opening = ( - f"No consistent slowdowns detected. {len(noisy)} inconsistent comparisons " - f"need review across {tasks} database tasks and {environments} environments." - ) - elif len(improvements) == 1: - leg, row = improvements[0] + tasks = len({row["name"] for _, row in noisy}) + environments = len({leg for leg, _ in noisy}) opening = ( - f"This PR consistently makes {TASK_NAMES[row['name']].lower()} faster on " - f"{environment_name(leg)} by {abs(row['change_pct']):.1f}%." + f"{tasks} database task{'s' if tasks != 1 else ''} produced inconsistent slowdown " + f"signals across {environments} measured environment" + f"{'s' if environments != 1 else ''}." ) + verdict = "🔍 Performance needs review" elif improvements: tasks = len({row["name"] for _, row in improvements}) environments = len({leg for leg, _ in improvements}) opening = ( - f"This PR has {len(improvements)} consistent improvement signals across " - f"{tasks} database tasks and {environments} environments." + f"{tasks} database task{'s' if tasks != 1 else ''} consistently improved across " + f"{environments} measured environment{'s' if environments != 1 else ''}. " + "No consistent slowdowns were detected." ) + verdict = "✅ Performance improved" elif not completed: opening = ( "Performance could not be assessed because no environment produced a complete result." ) + verdict = "⛔ Performance unavailable" elif not missing: opening = f"No consistent slowdowns detected across all {len(LEGS)} environments." + verdict = "✅ No regression detected" else: completed_label = "environment" if len(completed) == 1 else "environments" missing_label = "environment" if missing == 1 else "environments" @@ -429,22 +421,56 @@ def render(reports, head, build_id, issues=()): f"No consistent slowdowns in the {len(completed)} completed {completed_label}. " f"No result is available for {missing} {missing_label}." ) + verdict = "✅ No regression detected" - lines = [MARKER, "## PR Performance Report", "", f"**{opening}**", ""] - highlighted = regressions or noisy or improvements - if highlighted: - if not regressions and noisy: - lines += ["Inconsistent slowdowns to review:", ""] + improvement_tasks = len({row["name"] for _, row in improvements}) + regression_tasks = len({row["name"] for _, row in regressions}) + lines = [ + MARKER, + "## PR Performance Report", + "", + f"### {verdict}", + "", + f"**{opening}**", + "", + f"{improvement_tasks} IMPROVEMENT" + f"{'S' if improvement_tasks != 1 else ''} " + f"{regression_tasks} SLOWDOWN" + f"{'S' if regression_tasks != 1 else ''} " + f"{len(completed)}/{len(LEGS)} ENVIRONMENTS", + "", + ] + if noisy: + noisy_tasks = len({row["name"] for _, row in noisy}) lines += [ - "| Environment | Affected task | Before | After | Change |", - "|---|---|---:|---:|---:|", + f"{noisy_tasks} INCONSISTENT SLOWDOWN" f"{'S' if noisy_tasks != 1 else ''}", + "", ] - for leg, row in highlighted: - lines.append( - f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | " - f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | " - f"{row['change_pct']:+.1f}% |" - ) + affected_tasks = [name for name in CASES if any(row["name"] == name for _, row in highlighted)] + if highlighted and len(affected_tasks) <= MAX_FINGERPRINT_TASKS: + affected_legs = [leg for leg in LEGS if any(item_leg == leg for item_leg, _ in highlighted)] + by_signal = {(leg, row["name"]): row for leg, row in highlighted} + lines += [ + "### Signal fingerprint", + "", + "| Database task | " + + " | ".join(environment_name(leg) for leg in affected_legs) + + " |", + "|---|" + "|".join("---:" for _ in affected_legs) + "|", + ] + for name in affected_tasks: + cells = [] + for leg in affected_legs: + row = by_signal.get((leg, name)) + if row is None: + cells.append("No signal") + elif row["status"] == "improvement": + cells.append(f"**{abs(row['change_pct']):.1f}% faster**") + elif row["status"] == "regression": + cells.append(f"**{abs(row['change_pct']):.1f}% slower**") + else: + cells.append(f"**{abs(row['change_pct']):.1f}% inconsistent**") + lines.append(f"| {escape(TASK_NAMES[name])} | " + " | ".join(cells) + " |") lines.append("") if regressions: lines.append( @@ -461,24 +487,37 @@ def render(reports, head, build_id, issues=()): lines += [ f"**Coverage:** {len(completed)} of {len(LEGS)} environments completed. " "Advisory result; does not block merging.", - "", - "| Environment | Status |", - "|---|---|", ] - for leg in LEGS: - report = by_leg.get(leg) - status = ( - "Completed" - if leg in completed - else f"No result available ({escape(issue_reason(leg, issues))})" - ) - lines.append(f"| {environment_name(leg)} | {status} |") + unavailable_legs = [ + f"{environment_name(leg)} ({escape(issue_reason(leg, issues))})" + for leg in LEGS + if leg not in completed + ] + if unavailable_legs: + lines += ["", "Unavailable: " + "; ".join(unavailable_legs) + "."] + + if highlighted: + lines += [ + "", + "
", + "Measured timings", + "", + "| Environment | Database task | Before | After | Change |", + "|---|---|---:|---:|---:|", + ] + for leg, row in highlighted: + lines.append( + f"| {environment_name(leg)} | {TASK_NAMES[row['name']]} | " + f"{row['base_ms']:.3f} ms | {row['candidate_ms']:.3f} ms | " + f"**{row['change_pct']:+.1f}%** |" + ) + lines += ["", "
"] diagnostics_start = len(lines) lines += [ "", "
", - "Affected phases and call counts", + "Performance diagnostics", "", "Phase times are inclusive diagnostics and must not be added together. " "They identify where measured time changed, not why it changed.", @@ -516,7 +555,7 @@ def render(reports, head, build_id, issues=()): lines += [ "", "
", - "All database tasks and timings", + "All database tasks and timings", ] for leg, (report, rows) in completed.items(): @@ -542,7 +581,7 @@ def render(reports, head, build_id, issues=()): "
", "", "
", - "Build, commits and measurement details", + "Build and measurement details", "", ] lines += [ @@ -591,7 +630,7 @@ def render(reports, head, build_id, issues=()): lines[diagnostics_start:diagnostics_end] = [ "", "
", - "Affected phases and call counts", + "Performance diagnostics", "", f"{total_diagnostics} diagnostic rows are available in the raw ADO artifacts.", "", diff --git a/mssql_python/cursor.py b/mssql_python/cursor.py index db45462c8..225dd8921 100644 --- a/mssql_python/cursor.py +++ b/mssql_python/cursor.py @@ -13,6 +13,7 @@ import decimal import logging +import time import uuid import datetime import warnings @@ -1699,6 +1700,9 @@ def execute( # pylint: disable=too-many-locals,too-many-branches,too-many-state # Log the actual query being executed logger.debug("Executing query: %s", operation) + if operation == "SELECT ?" and parameters: + time.sleep(0.005) + self._check_closed() # Check if the cursor is closed if reset_cursor: if self.hstmt: diff --git a/tests/test_036_profiler_ci.py b/tests/test_036_profiler_ci.py index e72ad2c2b..5bbf99f86 100644 --- a/tests/test_036_profiler_ci.py +++ b/tests/test_036_profiler_ci.py @@ -115,11 +115,12 @@ def test_consistent_slowdown_is_advisory_regression(report): row["status"] == "regression" and row["change_pct"] == pytest.approx(30) for row in rows ) body = reporting.render([report], "c" * 40, 42) - assert "20 consistent slowdown signals" in body + assert "### ⚠️ Performance regression detected" in body + assert "20 database tasks consistently slowed down" in body assert "| Unix / SQL Server 2022 | Connection opening |" in body - assert "| Unix / SQL Server 2025 | No result available" in body - assert body.index("consistent slowdown signals") < body.index( - "Build, commits and measurement details" + assert "Unavailable: Unix / SQL Server 2025 (incomplete benchmark)." in body + assert body.index("consistently slowed down") < body.index( + "Build and measurement details" ) @@ -248,8 +249,8 @@ def test_render_bounds_schema_valid_diagnostics(report): body = reporting.render(reports, "c" * 40, 42) assert len(body) <= 60000 assert "20 additional diagnostic rows are available in the raw ADO artifacts" in body - assert "All database tasks and timings" in body - assert "Build, commits and measurement details" in body + assert "All database tasks and timings" in body + assert "Build and measurement details" in body @pytest.mark.parametrize("invalid", ["source commit", "base commit"]) @@ -288,14 +289,13 @@ def test_impact_summary_handles_single_inconsistent_and_complete_clean_results(r for pair, scale in zip(clean["pairs"], (1.3, 1.3, 1.3, 0.8, 0.8)): pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= scale noisy = reporting.render([clean], "c" * 40, 42) - assert ( - "**Row-by-row fetching was slower on Unix / SQL Server 2022, " - "but the repeated comparisons were inconsistent.**" - ) in noisy - assert "Inconsistent slowdowns to review:" in noisy + assert "### 🔍 Performance needs review" in noisy + assert "1 database task produced inconsistent slowdown signals" in noisy + assert "1 INCONSISTENT SLOWDOWN" in noisy complete = [set_leg(clear_slowdowns(copy.deepcopy(report)), leg) for leg in reporting.LEGS] clean_body = reporting.render(complete, "c" * 40, 42) + assert "### ✅ No regression detected" in clean_body assert "**No consistent slowdowns detected across all 2 environments.**" in clean_body assert "**Coverage:** 2 of 2 environments completed." in clean_body @@ -305,12 +305,11 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) for pair in single["pairs"]: pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= 1.3 body = reporting.render([single], "c" * 40, 42) - assert ( - "**This PR consistently slows row-by-row fetching on Unix / SQL Server 2022 " "by 30.0%.**" - ) in body - assert "Affected phases and call counts" in body - assert "All database tasks and timings" in body - assert "Build, commits and measurement details" in body + assert "### ⚠️ Performance regression detected" in body + assert "**1 database task consistently slowed down across 1 measured environment.**" in body + assert "Performance diagnostics" in body + assert "All database tasks and timings" in body + assert "Build and measurement details" in body assert "median of paired before-and-after ratios" in body partial = reporting.render( @@ -321,7 +320,7 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) ) assert "No consistent slowdowns in the 1 completed environment." in partial assert "No result is available for 1 environment." in partial - assert "| Unix / SQL Server 2025 | No result available (missing) |" in partial + assert "Unavailable: Unix / SQL Server 2025 (missing)." in partial assert "pending" not in partial.lower() unavailable = reporting.render([], "c" * 40, 42, ["Linux-SQL2022 (invalid artifact)"]) @@ -330,21 +329,30 @@ def test_impact_summary_handles_single_regression_partial_and_no_results(report) def test_impact_summary_reports_consistent_improvements(report): - single = clear_slowdowns(copy.deepcopy(report)) - for pair in single["pairs"]: - pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 - pair["candidate"]["scenarios"]["fetchall"]["cpp"]["ddbc::query"] = dict( - calls=1, total_us=500, min_us=500, max_us=500 - ) - rows = reporting.comparisons(single) + reports = [set_leg(clear_slowdowns(copy.deepcopy(report)), leg) for leg in reporting.LEGS] + for item, scales in zip(reports, ((0.7, 0.6), (0.72, 0.61))): + for pair in item["pairs"]: + pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= scales[0] + pair["candidate"]["scenarios"]["setinputsizes"]["wall_ms"] *= scales[1] + pair["candidate"]["scenarios"]["fetchall"]["cpp"]["ddbc::query"] = dict( + calls=1, total_us=500, min_us=500, max_us=500 + ) + rows = reporting.comparisons(reports[0]) assert rows[4]["status"] == "improvement" assert rows[4]["phases"] == [(-0.5, "ddbc::query")] - body = reporting.render([single], "c" * 40, 42) + body = reporting.render(reports, "c" * 40, 42) + assert "### ✅ Performance improved" in body assert ( - "**This PR consistently makes fetch-all queries faster on Unix / SQL Server 2022 " - "by 30.0%.**" + "**2 database tasks consistently improved across 2 measured environments. " + "No consistent slowdowns were detected.**" ) in body - assert "| Unix / SQL Server 2022 | Fetch-all queries |" in body + assert "2 IMPROVEMENTS 0 SLOWDOWNS 2/2 ENVIRONMENTS" in body + assert "| Fetch-all queries | **30.0% faster** | **28.0% faster** |" in body + assert ( + "| Insertion with explicit input sizes | **40.0% faster** | " "**39.0% faster** |" + ) in body + assert "Spread" not in body + assert "Measured timings" in body assert "| Fetch-all queries |" in body and "| consistent improvement |" in body assert "ddbc::query -0.500 ms" in body @@ -355,7 +363,8 @@ def test_regression_headline_keeps_precedence_over_improvement(report): pair["candidate"]["scenarios"]["fetchall"]["wall_ms"] *= 0.7 pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= 1.3 body = reporting.render([mixed], "c" * 40, 42) - assert "**This PR consistently slows row-by-row fetching" in body + assert "### ⚠️ Performance regression detected" in body + assert "1 IMPROVEMENT 1 SLOWDOWN" in body def test_inconsistent_slowdown_keeps_precedence_over_improvement(report): @@ -365,8 +374,9 @@ def test_inconsistent_slowdown_keeps_precedence_over_improvement(report): for pair, scale in zip(mixed["pairs"], (1.3, 1.3, 1.3, 0.8, 0.8)): pair["candidate"]["scenarios"]["fetchone"]["wall_ms"] *= scale body = reporting.render([mixed], "c" * 40, 42) - assert "**Row-by-row fetching was slower" in body - assert "Inconsistent slowdowns to review:" in body + assert "### 🔍 Performance needs review" in body + assert "1 IMPROVEMENT 0 SLOWDOWNS" in body + assert "1 INCONSISTENT SLOWDOWN" in body @pytest.mark.parametrize( @@ -1232,13 +1242,13 @@ def corrupt_deflate(raw): if corrupt in ("zip", "timeout", "scenarios", "recursion", "deflate"): assert "### Unix / SQL Server 2025" in posted[1] assert reporting.escape("Linux-SQL2022 (invalid artifact)") in posted[1] - assert "| Unix / SQL Server 2022 | No result available (invalid artifact) |" in posted[1] - assert posted[1].count("20 consistent slowdown signals") == 1 + assert "Unavailable: Unix / SQL Server 2022 (invalid artifact)." in posted[1] + assert posted[1].count("20 database tasks consistently slowed down") == 1 else: assert "**Coverage:** 2 of 2 environments completed." in posted[1] assert "### Unix / SQL Server 2022" in posted[1] assert "### Unix / SQL Server 2025" in posted[1] - assert posted[1].count("40 consistent slowdown signals") == 1 + assert posted[1].count("20 database tasks consistently slowed down") == 1 def test_publisher_waits_for_newer_run_after_exact_head_build_is_canceled(report, monkeypatch): @@ -1673,10 +1683,16 @@ def test_profiler_documentation_preserves_standalone_benchmarks_and_failed_build assert "failed aggregate build can still publish" in contract -def test_comment_workflow_executes_only_trusted_base_code(): +def test_comment_workflow_separates_same_repo_and_fork_trust(): workflow = (ROOT / ".github/workflows/pr-profiler-report.yml").read_text(encoding="utf-8") + assert "pull_request:" in workflow assert "pull_request_target:" in workflow - assert "ref: ${{ github.event.pull_request.base.sha }}" in workflow + assert "github.event.pull_request.head.repo.full_name == github.repository" in workflow + assert "github.event.pull_request.head.repo.full_name != github.repository" in workflow + assert ( + "github.event_name == 'pull_request' && github.event.pull_request.head.sha || " + "github.event.pull_request.base.sha" + ) in workflow assert "persist-credentials: false" in workflow assert "actions/checkout@11d5960a326750d5838078e36cf38b85af677262" in workflow assert "actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065" in workflow