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: Exclude statements that spawn jobs from showing up in query tables. #17786

Merged
merged 1 commit into from
Aug 24, 2017

Conversation

itsbilal
Copy link
Member

@itsbilal itsbilal commented Aug 21, 2017

Add a type switch for the AST, and manually exclude BACKUP/RESTORE/ALTER TABLEss from being added to session.mu.ActiveQueries, making them uncancellable and "invisible" to SHOW QUERIES.

Fixes #17730 .

@itsbilal itsbilal self-assigned this Aug 21, 2017
@itsbilal itsbilal requested review from knz and a team August 21, 2017 17:50
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz
Copy link
Contributor

knz commented Aug 22, 2017

The intention is laudable but the approach is limited. Please instead use the same approach as HiddenFromStats and IndependentFromParallelizedPriors in parser/stmt.go.


Reviewed 2 of 2 files at r1.
Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@itsbilal itsbilal force-pushed the ignore-backup-in-show-queries branch from 5c211a4 to 99d6020 Compare August 23, 2017 18:23
@itsbilal itsbilal requested a review from a team August 23, 2017 18:23
@itsbilal
Copy link
Member Author

Done. Implemented a new pseudo-interface. PTAL!


Review status: 0 of 3 files reviewed at latest revision, all discussions resolved, some commit checks pending.


Comments from Reviewable

@itsbilal itsbilal force-pushed the ignore-backup-in-show-queries branch 2 times, most recently from 290e166 to 7a776ad Compare August 24, 2017 15:59
@nvanbenschoten
Copy link
Member

Nice improvement!


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 3 unresolved discussions, all commit checks successful.


pkg/sql/executor.go, line 1097 at r2 (raw file):

		queryMeta.ctxCancel = txnState.cancel

		// Ignore statements that spawn jobs from SHOW QUERIES and from being cancellable using CANCEL QUERY.

A few long lines here. Try to keep under 100 chars, per our style guide.


pkg/sql/session.go, line 661 at r2 (raw file):

	queryMeta, ok := s.mu.ActiveQueries[queryID]
	if !ok {
		// Could be an ignored statement type.

Give this a bit more detail (eg. mention HiddenFromShowQueries). "ignored" could mean a lot of things.


pkg/sql/session.go, line 662 at r2 (raw file):

	if !ok {
		// Could be an ignored statement type.
		s.mu.Unlock()

I would just defer the Unlock at the top. We don't do anything expensive after unlocking anyway.


Comments from Reviewable

@itsbilal itsbilal force-pushed the ignore-backup-in-show-queries branch from 7a776ad to a98c469 Compare August 24, 2017 18:44
@itsbilal
Copy link
Member Author

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.


pkg/sql/executor.go, line 1097 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

A few long lines here. Try to keep under 100 chars, per our style guide.

Done.


pkg/sql/session.go, line 661 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Give this a bit more detail (eg. mention HiddenFromShowQueries). "ignored" could mean a lot of things.

Done.


pkg/sql/session.go, line 662 at r2 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

I would just defer the Unlock at the top. We don't do anything expensive after unlocking anyway.

Done.


Comments from Reviewable

@itsbilal itsbilal force-pushed the ignore-backup-in-show-queries branch from a98c469 to 666102a Compare August 24, 2017 18:46
@nvanbenschoten
Copy link
Member

:lgtm:


Review status: 1 of 3 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@itsbilal itsbilal merged commit f173622 into cockroachdb:master Aug 24, 2017
@itsbilal itsbilal deleted the ignore-backup-in-show-queries branch August 24, 2017 19:09
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