Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sql: session tracing inside a txn is painful #23558

Closed
andreimatei opened this issue Mar 7, 2018 · 1 comment
Closed

sql: session tracing inside a txn is painful #23558

andreimatei opened this issue Mar 7, 2018 · 1 comment
Assignees
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@andreimatei
Copy link
Contributor

If one uses, for example, crdb.ExecuteTx() and has code like the following:

                if _, err := tx.Exec(`set tracing=on`); err != nil {
                    return err
                }
                defer func() {
                    _, _ = tx.Exec(`set tracing=off`)
                }()

that's no good cause the final set tracing=off is rejected if the txn is in an error state. Moreover, starting tracing on that conn again fails. We should allow set tracing=off in all states. Or something.

Hacks like the following help, although tracing leaks on conns where it isn't stopped properly.

            var tracing string
            if err := tx.QueryRow(`show tracing`).Scan(&tracing); err != nil {
                return err
            }
            if tracing != "off" {
                if _, err := tx.Exec(`set tracing=off; set tracing=on;`); err != nil {
                    return err
                }
            } else {
                if _, err := tx.Exec(`set tracing=on`); err != nil {
                    return err
                }
            }

cc @petermattis

@andreimatei andreimatei added the C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) label Mar 7, 2018
@andreimatei andreimatei self-assigned this Mar 7, 2018
andreimatei added a commit to andreimatei/cockroach that referenced this issue May 2, 2018
Before this patch, starting tracing while tracing was ongoing, as well
as stopping tracing while it wasn't resulted in errors. Also, like all
statements, running set tracing in an aborted transaction was an error.
All these facts colluded to make tracing from a program around some code
that might result in errors hilariously frustrating - you start tracing,
then you may or may not get an error, then you want to stop tracing -
but how do you stop?

This patch makes it so that starting twice, stopping twice, or running
set tracing after an error, is a-OK.

Fixes cockroachdb#23558

Release note(sql): `set tracing=<mode>` became less finicky about the
current session state, so that a client can more easily trace around
statement that produce errors.
@benesch
Copy link
Contributor

benesch commented May 3, 2018

This is awesome. I ran straight into the issue you described while attempting to debug some TPCC-C performance issues and it was not fun.

andreimatei added a commit to andreimatei/cockroach that referenced this issue May 3, 2018
Before this patch, starting tracing while tracing was ongoing, as well
as stopping tracing while it wasn't resulted in errors. Also, like all
statements, running set tracing in an aborted transaction was an error.
All these facts colluded to make tracing from a program around some code
that might result in errors hilariously frustrating - you start tracing,
then you may or may not get an error, then you want to stop tracing -
but how do you stop?

This patch makes it so that starting twice, stopping twice, or running
set tracing after an error, is a-OK.
Note that `set tracing=cluster;set tracing=cluster;set tracing=off`
results in tracing being off (there's no counter for the nesting).

Fixes cockroachdb#23558

Release note(sql): `set tracing=<mode>` became less finicky about the
current session state, so that a client can more easily trace around
statement that produce errors.
andreimatei added a commit to andreimatei/cockroach that referenced this issue May 3, 2018
Before this patch, starting tracing while tracing was ongoing, as well
as stopping tracing while it wasn't resulted in errors. Also, like all
statements, running set tracing in an aborted transaction was an error.
All these facts colluded to make tracing from a program around some code
that might result in errors hilariously frustrating - you start tracing,
then you may or may not get an error, then you want to stop tracing -
but how do you stop?

This patch makes it so that starting twice, stopping twice, or running
set tracing after an error, is a-OK.
Note that `set tracing=cluster;set tracing=cluster;set tracing=off`
results in tracing being off (there's no counter for the nesting).

Fixes cockroachdb#23558

Release note(sql): `set tracing=<mode>` became less finicky about the
current session state, so that a client can more easily trace around
statement that produce errors.
andreimatei added a commit to andreimatei/cockroach that referenced this issue May 3, 2018
Before this patch, starting tracing while tracing was ongoing, as well
as stopping tracing while it wasn't resulted in errors. Also, like all
statements, running set tracing in an aborted transaction was an error.
All these facts colluded to make tracing from a program around some code
that might result in errors hilariously frustrating - you start tracing,
then you may or may not get an error, then you want to stop tracing -
but how do you stop?

This patch makes it so that starting twice, stopping twice, or running
set tracing after an error, is a-OK.
Note that `set tracing=cluster;set tracing=cluster;set tracing=off`
results in tracing being off (there's no counter for the nesting).

Fixes cockroachdb#23558

Release note(sql): `set tracing=<mode>` became less finicky about the
current session state, so that a client can more easily trace around
statement that produce errors.
craig bot pushed a commit that referenced this issue May 3, 2018
25262: sql: make 'set tracing' just work r=andreimatei a=andreimatei

Before this patch, starting tracing while tracing was ongoing, as well
as stopping tracing while it wasn't resulted in errors. Also, like all
statements, running set tracing in an aborted transaction was an error.
All these facts colluded to make tracing from a program around some code
that might result in errors hilariously frustrating - you start tracing,
then you may or may not get an error, then you want to stop tracing -
but how do you stop?

This patch makes it so that starting twice, stopping twice, or running
set tracing after an error, is a-OK.

Fixes #23558

Co-authored-by: Andrei Matei <andrei@cockroachlabs.com>
@craig craig bot closed this as completed in #25262 May 3, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Development

No branches or pull requests

2 participants