Skip to content

executor: fix INSERT regression for tables without generated columns - #68187

Merged
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
bb7133:fix/insert-fillrow-no-gc-allocation
May 11, 2026
Merged

executor: fix INSERT regression for tables without generated columns#68187
ti-chi-bot[bot] merged 2 commits into
pingcap:masterfrom
bb7133:fix/insert-fillrow-no-gc-allocation

Conversation

@bb7133

@bb7133 bb7133 commented May 6, 2026

Copy link
Copy Markdown
Member

What problem does this PR solve?

Issue Number: close #68129

Problem Summary:

PR #67917 introduced a performance regression in INSERT statements. In fillRow() inside insert_common.go, the code unconditionally calls MutRowFromDatums(row) — which allocates a Chunk plus N *Column objects — even when the table has zero virtual generated columns (gCols is empty). This causes unnecessary heap allocations on every single inserted row, increasing GC pressure and reducing throughput.

For YCSB Workload A (mixed read/write) against an 11-column table with no generated columns (the typical usertable schema), this regression measured at −23.8% OPS compared to the pre-PR baseline.

What changed and how does it work?

Two minimal changes in pkg/executor/insert_common.go:

  1. Lazy slice initialization: Changed gCols := make([]*table.Column, 0) to var gCols []*table.Column, avoiding an unnecessary heap allocation of an empty slice header on every call.

  2. Early return when no generated columns: Added a fast path immediately before the MutRowFromDatums call:

    if len(gCols) == 0 {
        return row, nil
    }

    When gCols is empty (the common case for tables without virtual generated columns), we skip MutRowFromDatums entirely and return the row directly.

These changes restore pre-regression performance for the common case while leaving the generated-column path fully intact.

Check List

Tests

  • Unit test (added BenchmarkInsertYCSBLike in pkg/executor/bench_gencol_test.go)
  • Integration test
  • Manual test (go-ycsb Workload A benchmark, 3-way comparison)
  • No need to test
    • I checked and no code files have been changed.

Benchmark results (go-ycsb Workload A, 8 threads, 30 s, tidb-server --store=unistore):

Binary OPS vs Baseline
Baseline (f2ebab0, before #67917) 19,143
Regressed (35e8e04, after #67917) 14,591 −23.8%
Fixed (35e8e04 + this PR) 18,990 −0.8% (within noise)

Side effects:

  • Performance regression: Consumes more CPU
  • Performance regression: Consumes more Memory
  • Breaking backward compatibility

Documentation:

  • Affects user behaviors
  • Contains syntax changes
  • Contains variable changes
  • Contains experimental features
  • Changes MySQL compatibility

Release note

Fix a performance regression in INSERT statements introduced by #67917: skip MutRowFromDatums allocation when a table has no virtual generated columns, restoring ~24% OPS throughput for workloads on plain tables.

Summary by CodeRabbit

  • Refactor
    • Reduced work and allocations during INSERT when no generated columns exist by adding a fast-path that skips generated-column evaluation, improving insert performance.
  • Tests
    • Added a YCSB-like benchmark to measure INSERT performance on wide tables and track regressions.

Review Change Stack

…ingcap#68129)

When PR pingcap#67917 hoisted MutRowFromDatums(row) outside the generated-column
evaluation loop to avoid O(G×C) allocations, the call became unconditional.
For tables with no generated columns (the common case — e.g. YCSB usertable),
MutRowFromDatums is now called on every INSERT row, allocating a full Chunk
with N Column objects and copying all datums, all for zero benefit since the
loop body never executes.

This is the root cause of the ~20% YCSB Workload A regression reported in
issue pingcap#68129.

Fix: guard the MutRowFromDatums call with an early return when gCols is empty,
restoring zero overhead for the common case while preserving the O(G×C)→O(C)
optimization for tables that actually have generated columns.

Also change the gCols slice initializer from make([]*table.Column, 0) to
var gCols []*table.Column to avoid the small slice header allocation when
no generated columns are present.

Fixes pingcap#68129
@ti-chi-bot ti-chi-bot Bot added do-not-merge/needs-linked-issue do-not-merge/needs-tests-checked do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 6, 2026
@pantheon-ai

pantheon-ai Bot commented May 6, 2026

Copy link
Copy Markdown

@bb7133 I've received your pull request and will start the review. I'll conduct a thorough review covering code quality, potential issues, and implementation details.

⏳ This process typically takes 10-30 minutes depending on the complexity of the changes.

ℹ️ Learn more details on Pantheon AI.

@ti-chi-bot ti-chi-bot Bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 6, 2026
@pingcap-cla-assistant

pingcap-cla-assistant Bot commented May 6, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented May 6, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 1bf76185-b484-4b13-b6f6-53b340c594c8

📥 Commits

Reviewing files that changed from the base of the PR and between 494845e and 09434ba.

📒 Files selected for processing (2)
  • pkg/executor/bench_gencol_test.go
  • pkg/executor/insert_common.go
🚧 Files skipped from review as they are similar to previous changes (2)
  • pkg/executor/bench_gencol_test.go
  • pkg/executor/insert_common.go

📝 Walkthrough

Walkthrough

fillRow now builds a generated-column slice lazily and returns immediately when no generated columns exist; a new YCSB-like INSERT benchmark was added to exercise the insert path.

Changes

Generated Column Precomputation

Layer / File(s) Summary
Data Shape / Declarations
pkg/executor/insert_common.go
gCols declared as a nil slice (var) instead of a pre-initialized slice.
Core Implementation / Fast-path
pkg/executor/insert_common.go
fillRow appends generated columns during the initial column iteration and returns early when len(gCols) == 0, skipping generated-column evaluation.
Benchmark
pkg/executor/bench_gencol_test.go
Added BenchmarkInsertYCSBLike which creates an 11-column VARCHAR usertable and repeatedly runs INSERT IGNORE of a constant row.

Sequence Diagram(s)

(Skipped — conditions for diagram generation not met.)

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • henrybw
  • terry1purcell
  • hawkingrei

Poem

🐰 I hopped through columns, one tidy scan,
Collected the generated, then filled what I can.
If none need magic, I bound past the gate—
Quick little thump, no extra work to wait.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main fix: addressing an INSERT performance regression for tables without generated columns.
Description check ✅ Passed The description comprehensively covers the problem, the solution with code details, benchmark results, test additions (BenchmarkInsertYCSBLike), and includes a detailed release note.
Linked Issues check ✅ Passed The PR directly addresses issue #68129 by fixing the INSERT performance regression (−23.8% OPS) caused by PR #67917, restoring throughput to near-baseline levels through optimized gCols handling.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the performance regression: optimization in insert_common.go and a targeted benchmark test in bench_gencol_test.go, with no extraneous modifications.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 golangci-lint (2.12.1)

Command failed

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ti-chi-bot ti-chi-bot Bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/needs-triage-completed and removed do-not-merge/needs-linked-issue do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels May 6, 2026
@codecov

codecov Bot commented May 6, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.8981%. Comparing base (0e5e814) to head (09434ba).
⚠️ Report is 32 commits behind head on master.

Additional details and impacted files
@@               Coverage Diff                @@
##             master     #68187        +/-   ##
================================================
- Coverage   77.7559%   76.8981%   -0.8578%     
================================================
  Files          1990       1973        -17     
  Lines        551769     556868      +5099     
================================================
- Hits         429033     428221       -812     
- Misses       121816     128402      +6586     
+ Partials        920        245       -675     
Flag Coverage Δ
integration 41.5131% <100.0000%> (+1.7112%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
dumpling 60.4888% <ø> (ø)
parser ∅ <ø> (∅)
br 50.0597% <ø> (-13.0338%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…SERT path

Adds a benchmark reproducing the regression from pingcap#68129: an 11-column
table (1 VARCHAR PK + 10 VARCHAR fields, no generated columns) modelling
the YCSB usertable schema. This exercises the MutRowFromDatums fast-path
guard added in the fix and will catch any future regression on plain tables.
@ti-chi-bot ti-chi-bot Bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. do-not-merge/needs-triage-completed do-not-merge/needs-tests-checked labels May 6, 2026
@ti-chi-bot ti-chi-bot Bot added approved needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 7, 2026
@ti-chi-bot

ti-chi-bot Bot commented May 7, 2026

Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cfzjywxk, tiancaiamao

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cfzjywxk,tiancaiamao]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot Bot added lgtm and removed needs-1-more-lgtm Indicates a PR needs 1 more LGTM. labels May 7, 2026
@ti-chi-bot

ti-chi-bot Bot commented May 7, 2026

Copy link
Copy Markdown

[LGTM Timeline notifier]

Timeline:

  • 2026-05-07 03:53:23.173499545 +0000 UTC m=+326276.046849507: ☑️ agreed by cfzjywxk.
  • 2026-05-07 15:14:21.141815358 +0000 UTC m=+367134.015165340: ☑️ agreed by tiancaiamao.

@hawkingrei

Copy link
Copy Markdown
Contributor

/retest

@bb7133
bb7133 force-pushed the fix/insert-fillrow-no-gc-allocation branch from 494845e to 09434ba Compare May 11, 2026 01:59
@ti-chi-bot
ti-chi-bot Bot merged commit a58b358 into pingcap:master May 11, 2026
35 checks passed
@ti-chi-bot ti-chi-bot Bot added the needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. label Jun 15, 2026
@ti-chi-bot

Copy link
Copy Markdown
Member

In response to a cherrypick label: new pull request created to branch release-8.5: #69208.

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

Labels

approved lgtm needs-cherry-pick-release-8.5 Should cherry pick this PR to release-8.5 branch. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

There is a 19.6% performance regression in ycsb after PR#67917

5 participants