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

cli: use ExecerContext and companions in sql_util.go #25435

Closed
tbg opened this issue May 11, 2018 · 2 comments
Closed

cli: use ExecerContext and companions in sql_util.go #25435

tbg opened this issue May 11, 2018 · 2 comments
Labels
A-cli-client CLI commands that pertain to using SQL features C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. no-issue-activity X-stale

Comments

@tbg
Copy link
Member

tbg commented May 11, 2018

When porting the node status command over to use SQL, we were unable to use context-based cancellation since there is an unaddressed TODO to switch to ExecerContext (etc) in sqlConnI. We worked around that using SET statement_timeout=x but that's just a crutch.

See #25094 (comment).

@knz for triage.

Jira issue: CRDB-6358

@tbg tbg added C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. A-cli labels May 11, 2018
@tbg tbg assigned knz May 11, 2018
@tbg
Copy link
Member Author

tbg commented May 23, 2018

I just ran into a problem with QueryRowContext in the context of #25837 where I cancelled a context and it had no effect, which led to this illuminating comment from @knz:

two things
first I think the API that enables context cancellation is actually not used here (something to do with which variant of go's sql stuff and lib/pq are in use)
Also I am not sure our SQL server-side is able to recognize when a client conn drops while a query is running (I think it only picks up the cancellation when the current stmt finishes - not sure)

Especially the former seems troubling. If something takes a context, we expect context cancellation to work. Had we had the contexted API available in #25094 (see link above) we likely would've messed it up (well, hopefully a test would've caught the problem).

@knz knz removed their assignment Apr 8, 2020
@knz knz added A-cli-client CLI commands that pertain to using SQL features and removed A-cli labels Mar 20, 2021
@github-actions
Copy link

We have marked this issue as stale because it has been inactive for
18 months. If this issue is still relevant, removing the stale label
or adding a comment will keep it active. Otherwise, we'll close it in
10 days to keep the issue queue tidy. Thank you for your contribution
to CockroachDB!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli-client CLI commands that pertain to using SQL features C-cleanup Tech debt, refactors, loose ends, etc. Solution not expected to significantly change behavior. no-issue-activity X-stale
Projects
No open projects
Development

No branches or pull requests

2 participants