Avoid a panic when using show diagnostics with text/csv #9682
Conversation
ghost
assigned 
jsternberg
force-pushed
the
js-9504-csv-show-diagnostics-panic
branch
from
7242ac4
to
c5ac172
Compare
services/httpd/response_writer.go
Outdated
| @@ -147,7 +148,29 @@ func (w *csvFormatter) WriteResponse(resp Response) (n int, err error) { | |||
| } | |||
| } | |||
|
|
|||
| for _, row := range result.Series { | |||
| for i, row := range result.Series { | |||
| if i > 0 && !reflect.DeepEqual(result.Series[i-1].Columns, row.Columns) { | |||
There was a problem hiding this comment.
Since these are both string slices, can you make a helper method to compare them instead of using reflect?
| w.columns[1] = "tags" | ||
| copy(w.columns[2:], row.Columns) | ||
| if err := csv.Write(w.columns); err != nil { | ||
| return n, err |
There was a problem hiding this comment.
The n tracking is missing most of the csv calls - it's unfortunate that the csv.Writer doesn't expose the bytes written.
Assuming we do want to track n (for the QueryRequestBytesTransmitted stat), what do you think about using io.MultiWriter with the underlying w and some fake writer like
type countWriter struct { n int64 }
func (w *countWriter) Write(p []byte) (int, err) {
w.n += len(p)
return len(p), nil
}so that we can properly track n?
There was a problem hiding this comment.
That would be good. Can we open up another PR for that though? The behavior is already broken and I'm not breaking it anymore than it previously was.
There was a problem hiding this comment.
Sure thing - opened #9690 to track it.
jsternberg
force-pushed
the
js-9504-csv-show-diagnostics-panic
branch
from
c5ac172
to
27f1372
Compare
| w.columns[1] = "tags" | ||
| copy(w.columns[2:], row.Columns) | ||
| if err := csv.Write(w.columns); err != nil { | ||
| return n, err |
There was a problem hiding this comment.
Sure thing - opened #9690 to track it.
If the columns change between series, it will now act as if it was a new statement id and reprint the headers. This only happens with show diagnostics at the moment and we shouldn't add this functionality anywhere else anyway.
jsternberg
force-pushed
the
js-9504-csv-show-diagnostics-panic
branch
from
27f1372
to
243ed2e
Compare
If the columns change between series, it will now act as if it was a new
statement id and reprint the headers. This only happens with show
diagnostics at the moment and we shouldn't add this functionality
anywhere else anyway.
Fixes #9504.