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: add optional message to CANCEL commands #130776

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

XiaochenCui
Copy link
Contributor

@XiaochenCui XiaochenCui commented Sep 16, 2024

overview

Previously, asynchronous CANCEL commands ("CANCEL QUERY", "CANCEL SESSION") didn't provide much information back to the query/session being canceled.

This commit addresses this issue by:

  • Update the SQL syntax of "CANCEL" commands ("CANCEL QUERY", "CANCEL SESSION") to add a new "WITH MESSAGE " clause.
  • The canceled query/session will throw an error with the from the canceler.

Fixes: #127490

Release note (sql change): Add optional "WITH MESSAGE " clause to CANCEL commands, providing more detailed information when a query is canceled.

sql syntax changes

origin:
// CANCEL QUERIES [IF EXISTS] <selectclause>
// CANCEL QUERY [IF EXISTS] <expr>

new:
// CANCEL QUERIES [IF EXISTS] <selectclause> [WITH MESSAGE <cancel_message>]
// CANCEL QUERY [IF EXISTS] <expr> [WITH MESSAGE <cancel_message>]

how to review

  1. I updated the sql syntax: pkg/sql/parser/sql.y
  2. A new logic test is added to test this feature: pkg/sql/logictest/testdata/logic_test/cancel
  3. The "cancel message" is sent from the canceler: pkg/sql/conn_executor.go
  4. The "target query" receives the "cancel message" and throw it to client: pkg/sql/conn_executor_exec.go

Other changes are trivial.

concerns

  • This pr involves sql syntax changes, should we open a new pr and work on a RFC at first? (see: 20171011_adding_sql_syntax.md ), that's also the reason I post this unfinished commit for review

    one that focuses on changes to the SQL grammar; in short, new syntactic features should be discussed via the RFC process and merged with the same PR that actually implements the feature.

  • The current implementation uses a synchronized map to send the "cancel message" to the query being canceled. This might be too heavy for such a small feature, and further experimentation is needed to assess the performance impact. (or we can just get the work done, depend on the development style)

todo

  • Design new syntax for "CANCEL QUERY" to support adding custom messages.
  • Design new syntax for "CANCEL SESSION" to support adding custom messages.

Copy link

blathers-crl bot commented Sep 16, 2024

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

I have added a few people who may be able to assist in reviewing:

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@blathers-crl blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels Sep 16, 2024
@blathers-crl blathers-crl bot requested a review from michae2 September 16, 2024 01:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@XiaochenCui

This comment was marked as outdated.

@jeffswenson jeffswenson added the T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) label Sep 16, 2024
Copy link

blathers-crl bot commented Sep 20, 2024

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Copy link

blathers-crl bot commented Sep 24, 2024

It looks like your PR touches SQL parser code but doesn't add or edit parser tests. Please make sure you add or edit parser tests if you edit the parser.

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

Previously, asynchronous CANCEL commands ("CANCEL QUERY", "CANCEL
SESSION") didn't provide much information back to the query/session
being canceled.

This commit addresses this issue by:

- Update the SQL syntax of "CANCEL" commands ("CANCEL QUERY", "CANCEL
SESSION") to add a new "WITH MESSAGE <message>" clause.
- The canceled query/session will throw an error with the <message>
from the canceler.

Fixes: cockroachdb#127490

Release note (sql change): Add optional "WITH MESSAGE <message>" clause
to CANCEL commands, providing more detailed information when a query
is canceled.
@XiaochenCui
Copy link
Contributor Author

concerns

  • This pr involves sql syntax changes, should we open a new pr and work on a RFC at first? (see: 20171011_adding_sql_syntax.md ), that's also the reason I post this unfinished commit for review

    one that focuses on changes to the SQL grammar; in short, new syntactic features should be discussed via the RFC process and merged with the same PR that actually implements the feature.

  • The current implementation uses a synchronized map to send the "cancel message" to the query being canceled. This might be too heavy for such a small feature, and further experimentation is needed to assess the performance impact. (or we can just get the work done, depend on the development style)

hi, @michae2
I've updated the sql syntax and now we can pass message to the query being canceled. All lints and test have also be passed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-community Originated from the community T-sql-foundations SQL Foundations Team (formerly SQL Schema + SQL Sessions) X-blathers-triaged blathers was able to find an owner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

sql: add optional message to CANCEL commands
3 participants