Skip to content

sql: fix "rows written" metrics#167360

Merged
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:fix-written-metric
Apr 2, 2026
Merged

sql: fix "rows written" metrics#167360
trunk-io[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:fix-written-metric

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

@yuzefovich yuzefovich commented Apr 1, 2026

This commit fixes a bug that was introduced long time ago in 03c09a3. In particular, in that change we began tracking "rows written" for the purposes of the guardrails, and later this information was also added into statement statistics as well as extended to some metrics.

The bug was that we only check the first wrapped planNode for being a mutationPlanNode, but it's actually possible for a single planNodeToRowSource wrapper to wrap multiple planNodes at once. For example, if we have a mutation in the CTE, then we could have bufferNode -> insertNode -> valuesNode, all handled by a single wrapper. Previously, we would only check bufferNode, and since it's not a mutation planNode, we'd emit no "written" metrics.

This commit fixes the issue by traversing the planNode tree until we either reach the first not-wrapped planNode (i.e. it's handled by DistSQL separately) or reach the leaf nodes.

Note that this bug was further hidden by the fact that if we collect execution stats (which we do for stmt bundles and when the txn is sampled), then we wrap each planNode separately, so in those scenarios the behavior was correct.

Epic: None

Release note (bug fix): Previously, if a mutation was executed in a subquery (e.g. as a CTE), the "rows written" metrics like sql.statements.index_rows_written.count and
sql.statements.index_bytes_written.count might not be incremented correctly. This is now fixed.

This commit fixes a bug that was introduced long time ago in
03c09a3. In particular, in that change
we began tracking "rows written" for the purposes of the guardrails, and
later this information was also added into statement statistics as well
as extended to some metrics.

The bug was that we only check the first wrapped `planNode` for being
a `mutationPlanNode`, but it's actually possible for a single
`planNodeToRowSource` wrapper to wrap multiple `planNode`s at once. For
example, if we have a mutation in the CTE, then we could have
`bufferNode -> insertNode -> valuesNode`, all handled by a single
wrapper. Previously, we would only check `bufferNode`, and since it's
not a mutation planNode, we'd emit no "written" metrics.

This commit fixes the issue by traversing the planNode tree until we
either reach the first not-wrapped planNode (i.e. it's handled by
DistSQL separately) or reach the leaf nodes.

Note that this bug was further hidden by the fact that if we collect
execution stats (which we do for stmt bundles and when the txn is
sampled), then we wrap each planNode separately, so in those scenarios
the behavior was correct.

Release note (bug fix): Previously, if a mutation was executed in
a subquery (e.g. as a CTE), the "rows written" metrics like
`sql.statements.index_rows_written.count` and
`sql.statements.index_bytes_written.count` might not be incremented
correctly. This is now fixed.
@yuzefovich yuzefovich requested review from a team and DrewKimball April 1, 2026 23:11
@yuzefovich yuzefovich added backport-25.4.x Flags PRs that need to be backported to 25.4 backport-26.1.x Flags PRs that need to be backported to 26.1 backport-26.2.x Flags PRs that need to be backported to 26.2 labels Apr 1, 2026
@trunk-io
Copy link
Copy Markdown
Contributor

trunk-io Bot commented Apr 1, 2026

😎 Merged successfully - details.

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm:

@DrewKimball reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on yuzefovich).

@yuzefovich
Copy link
Copy Markdown
Member Author

TFTR!

/trunk merge

@trunk-io trunk-io Bot merged commit fc60810 into cockroachdb:master Apr 2, 2026
50 of 52 checks passed
@blathers-crl
Copy link
Copy Markdown

blathers-crl Bot commented Apr 2, 2026

Encountered an error creating backports. Some common things that can go wrong:

  1. The backport branch might have already existed.
  2. There was a merge conflict.
  3. The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.


merge conflict cherry-picking 684c04f to blathers/backport-release-25.4-167360

Backport to branch 25.4.x failed. See errors above.


🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@yuzefovich yuzefovich deleted the fix-written-metric branch April 2, 2026 20:25
@yuzefovich yuzefovich removed the backport-25.4.x Flags PRs that need to be backported to 25.4 label Apr 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-26.1.x Flags PRs that need to be backported to 26.1 backport-26.2.x Flags PRs that need to be backported to 26.2 backport-failed target-release-26.3.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants