Skip to content

Add auto_trace & remove SHOW TRACE #3508

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

Merged
merged 7 commits into from
Aug 8, 2018
Merged

Add auto_trace & remove SHOW TRACE #3508

merged 7 commits into from
Aug 8, 2018

Conversation

lnhsingh
Copy link
Contributor

@lnhsingh lnhsingh commented Aug 6, 2018

New SQL shell option called auto_trace, which shows tracing for every subsequent statement. Remove SHOW TRACE in v2.1 docs.

Closes #3475 and #3452.

@lnhsingh lnhsingh requested review from knz and rmloveland August 6, 2018 21:25
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@lnhsingh lnhsingh changed the title Add auto_trace to SQL shell options Add auto_trace & remove SHOW TRACE Aug 6, 2018
@lnhsingh
Copy link
Contributor Author

lnhsingh commented Aug 6, 2018

@knz, in the old docs for SHOW TRACE, we have an example that shows how to trace conflicting transactions. Can you do this with SHOW TRACE FOR SESSION? If you can, I can't figure out how to do it.

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

:lgtm: but I'd prefer if you would also document the results option.

Reviewed 1 of 1 files at r1, 3 of 4 files at r2, 3 of 3 files at r3.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


v2.1/set-vars.md, line 248 at r3 (raw file):

`on`  | Same as `cluster`.
`kv`  | Same as `cluster` except that "kv messages" are collected instead of regular trace messages. See [`SHOW TRACE FOR SESSION`](show-trace-for-session.html).
`local` | Trace recording is enabled; only trace messages issued by the local node are collected.

there is a new option results too.

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained


v2.1/show-trace-for-session.md, line 140 at r3 (raw file):

~~~

<!-- ### Trace conflicting transactions

@knz, in the old docs for SHOW TRACE, we have an example that shows how to trace conflicting transactions. Can you do this with SHOW TRACE FOR SESSION? If you can, I can't figure out how to do it.

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


v2.1/set-vars.md, line 248 at r3 (raw file):

Previously, knz (kena) wrote…

there is a new option results too.

Done.

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r4, 2 of 2 files at r5.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


v2.1/show-trace-for-session.md, line 140 at r3 (raw file):

Previously, lhirata wrote…

@knz, in the old docs for SHOW TRACE, we have an example that shows how to trace conflicting transactions. Can you do this with SHOW TRACE FOR SESSION? If you can, I can't figure out how to do it.

Yes I'm sure you can but I don't recall the steps exactly either. Andrei, Bram, Nathan would know.

Copy link
Contributor

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

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

LGTM overall! Found one sentence I think needs adjusting.

v2.1/set-vars.md Outdated
`local` | Trace recording is enabled; only trace messages issued by the local node are collected.
`results` | Result rows and row counts are copied to the session trace. This must be specified explicitly when printing the results produced by a query in the trace is needed.<br><br>Example: `SET tracing = kv, results;`
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this sentence. Do you mean something like "this must be specified in order for the output of the query to be printed to the session trace" (or?)

`client_min_messages` | Ignored; recognized for compatibility with PostgreSQL clients. Only posible value is "`on`". | N/A | No
`extra_float_digits` | Ignored; recognized for compatibility with PostgreSQL clients. | N/A | No
`standard_conforming_strings` | Ignored; recognized for compatibility with PostgreSQL clients. | N/A | No
`application_name` | The current application name for statistics collection. | Empty string | Yes
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this block all whitespace changes? (I think yes but wanted to confirm.)

Copy link
Contributor Author

@lnhsingh lnhsingh left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale)


v2.1/set-vars.md, line 39 at r5 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

Is this block all whitespace changes? (I think yes but wanted to confirm.)

Yep


v2.1/set-vars.md, line 249 at r5 (raw file):

Previously, rmloveland (Rich Loveland) wrote…

I'm not sure I understand this sentence. Do you mean something like "this must be specified in order for the output of the query to be printed to the session trace" (or?)

Updated. Thanks!

@cockroach-teamcity
Copy link
Member

@cockroach-teamcity
Copy link
Member

@lnhsingh lnhsingh merged commit 4da1403 into master Aug 8, 2018
@lnhsingh lnhsingh deleted the auto-trace branch August 8, 2018 14:46
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.

4 participants