Skip to content

sql: fix the session tracing in some cases#64766

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:fix-tracing
May 6, 2021
Merged

sql: fix the session tracing in some cases#64766
craig[bot] merged 1 commit intocockroachdb:masterfrom
yuzefovich:fix-tracing

Conversation

@yuzefovich
Copy link
Copy Markdown
Member

@yuzefovich yuzefovich commented May 6, 2021

When the session tracing is started, we might need to start two tracing
spans: we always start a span for the connection, but also if we're
inside of a txn, we start a separate span for the txn. Previously, the
span of the previous txn wasn't properly cleaned up when the session
tracing is started again outside of a txn, which resulted in old
(irrelevant) entries being added to the newer trace. Now this is fixed.

Additionally, this commit makes sure to finish the tracing spans (which
wasn't done previously).

Fixes: #59203.
Fixes: #60672.

Release note (bug fix): Previously, the session trace (i.e.
SHOW TRACE FOR SESSION) could contain entries that corresponded to the previous
trace (i.e. SET TRACING=ON didn't properly reset the trace). Now this
is fixed.

@yuzefovich yuzefovich requested review from a team, andreimatei and irfansharif May 6, 2021 05:52
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

When the session tracing is started, we might need to start two tracing
spans: we always start a span for the connection, but also if we're
inside of a txn, we start a separate span for the txn. Previously, the
span of the previous txn wasn't properly cleaned up when the session
tracing is started again outside of a txn, which resulted in old
(irrelevant) entries being added to the newer trace. Now this is fixed.

Additionally, this commit makes sure to finish the tracing spans (which
wasn't done previously).

Release note (bug fix): Previously, the session trace (i.e. `SHOW TRACE
FOR SESSION`) could contain entries that corresponded to the previous
trace (i.e. `SET TRACING=ON` didn't properly reset the trace). Now this
is fixed.
Copy link
Copy Markdown
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @andreimatei)

@yuzefovich
Copy link
Copy Markdown
Member Author

TFTR!

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented May 6, 2021

Build succeeded:

@craig craig bot merged commit 84c450e into cockroachdb:master May 6, 2021
@yuzefovich yuzefovich deleted the fix-tracing branch June 4, 2021 04:12
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.

execbuilder: unexpectedly changed KV usage due to tracing commit tracing: wrongly emitting trace events for session's first txn

3 participants