Skip to content

Commit

Permalink
sql: pass statement request id to backend
Browse files Browse the repository at this point in the history
Previously, the frontend passed a statement fingerprint to the API to
update the corresponding statement diagnostic request to an expired
state. Now we pass the statement diagnostic ID directly. This resolves a
SQL parsing error that occurs when a statement fingerprint contains
single-quotes (i.e. when a statement fingerprint contains placeholders).

Release note: None
  • Loading branch information
Thomas Hardy committed Jan 31, 2022
1 parent 7adf8b7 commit 128eb4a
Show file tree
Hide file tree
Showing 6 changed files with 15 additions and 13 deletions.
2 changes: 1 addition & 1 deletion docs/generated/http/full.md
Original file line number Diff line number Diff line change
Expand Up @@ -3687,7 +3687,7 @@ Support status: [reserved](#support-status)

| Field | Type | Label | Description | Support status |
| ----- | ---- | ----- | ----------- | -------------- |
| statement_fingerprint | [string](#cockroach.server.serverpb.CancelStatementDiagnosticsReportRequest-string) | | | [reserved](#support-status) |
| request_id | [int64](#cockroach.server.serverpb.CancelStatementDiagnosticsReportRequest-int64) | | | [reserved](#support-status) |



Expand Down
2 changes: 1 addition & 1 deletion pkg/server/serverpb/status.proto
Original file line number Diff line number Diff line change
Expand Up @@ -1286,7 +1286,7 @@ message CreateStatementDiagnosticsReportResponse {
}

message CancelStatementDiagnosticsReportRequest {
string statement_fingerprint = 1;
int64 request_id = 1 [(gogoproto.customname) = "RequestID"];
}

message CancelStatementDiagnosticsReportResponse {
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/statement_diagnostics_requests.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ func (s *statusServer) CancelStatementDiagnosticsReport(
}

var response serverpb.CancelStatementDiagnosticsReportResponse
err := s.stmtDiagnosticsRequester.CancelRequest(ctx, req.StatementFingerprint)
err := s.stmtDiagnosticsRequester.CancelRequest(ctx, req.RequestID)
if err != nil {
response.Canceled = false
response.Error = err.Error()
Expand Down
2 changes: 1 addition & 1 deletion pkg/server/status.go
Original file line number Diff line number Diff line change
Expand Up @@ -396,7 +396,7 @@ type StmtDiagnosticsRequester interface {
// CancelRequest updates an entry in system.statement_diagnostics_requests
// for tracing a query with the given fingerprint to be expired (thus,
// canceling any new tracing for it).
CancelRequest(ctx context.Context, stmtFingerprint string) error
CancelRequest(ctx context.Context, requestID int64) error
}

// newStatusServer allocates and returns a statusServer.
Expand Down
14 changes: 8 additions & 6 deletions pkg/sql/stmtdiagnostics/statement_diagnostics.go
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ func (r *Registry) insertRequestInternal(
}

// CancelRequest is part of the server.StmtDiagnosticsRequester interface.
func (r *Registry) CancelRequest(ctx context.Context, stmtFingerprint string) error {
func (r *Registry) CancelRequest(ctx context.Context, requestID int64) error {
g, err := r.gossip.OptionalErr(48274)
if err != nil {
return err
Expand All @@ -402,19 +402,21 @@ func (r *Registry) CancelRequest(ctx context.Context, stmtFingerprint string) er
// request as "expired" by setting `expires_at` into the past. This will
// allow any queries that are currently being traced for this request to
// write their collected bundles.
fmt.Sprintf("UPDATE system.statement_diagnostics_requests SET expires_at = '1970-01-01' "+
"WHERE completed = false AND statement_fingerprint = '%s' "+
"AND (expires_at IS NULL OR expires_at > now()) RETURNING id;", stmtFingerprint),
"UPDATE system.statement_diagnostics_requests SET expires_at = '1970-01-01' "+
"WHERE completed = false AND id = $1 "+
"AND (expires_at IS NULL OR expires_at > now()) RETURNING id;",
requestID,
)
if err != nil {
return err
}

if row == nil {
// There is no pending diagnostics request with the given fingerprint.
return errors.Newf("no pending request found for the fingerprint: %s", stmtFingerprint)
return errors.Newf("no pending request found for the fingerprint: %s", requestID)
}
reqID := RequestID(tree.MustBeDInt(row[0]))

reqID := RequestID(requestID)
r.cancelRequest(reqID)

// Notify all the other nodes that this request has been canceled.
Expand Down
6 changes: 3 additions & 3 deletions pkg/sql/stmtdiagnostics/statement_diagnostics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ func TestDiagnosticsRequest(t *testing.T) {
// Verify that an error is returned when attempting to cancel non-existent
// request.
t.Run("cancel non-existent request", func(t *testing.T) {
require.NotNil(t, registry.CancelRequest(ctx, "foo"))
require.NotNil(t, registry.CancelRequest(ctx, 123456789))
})

// Verify that if a request (either conditional or unconditional, w/ or w/o
Expand All @@ -227,7 +227,7 @@ func TestDiagnosticsRequest(t *testing.T) {
require.NoError(t, err)
checkNotCompleted(reqID)

err = registry.CancelRequest(ctx, fprint)
err = registry.CancelRequest(ctx, reqID)
require.NoError(t, err)
checkNotCompleted(reqID)

Expand Down Expand Up @@ -271,7 +271,7 @@ func TestDiagnosticsRequest(t *testing.T) {
go func() {
defer wg.Done()
<-waitCh
err := registry.CancelRequest(ctx, fprint)
err := registry.CancelRequest(ctx, reqID)
require.NoError(t, err)
}()

Expand Down

0 comments on commit 128eb4a

Please sign in to comment.