MySQL: Support multiple statements

[exul]

What is this feature?

It allows enabling multiple statement execution for the MySQL data source.

Why do we need this feature?
This allows executing SQL queries that consists of multiple statements like SET @tags='deploy'; SELECT time_sec, description as text, tags FROM event WHERE tags=@tags.

There are a couple of things that need to be considered when deciding if this should be merged:

• Multiple statements can result in multiple result sets, currently Grafana would only support processing the first result set
• To support multiple result sets a change in the SDK would be needed (putting https://github.com/grafana/grafana-plugin-sdk-go/blob/main/data/sqlutil/sql.go#L46 into another loop and calling rows.NextResultSet)
• Multiple result sets could return different colums, which would make the data processing way more complex

Summary: While this will enable cases described in #24721 where multiple statements are used to set variables, it doesn't enable support for multiple result sets and I'm unsure if this could cause issues further down the line.

Who is this feature for?

Users who use the MySQL data source.

Which issue(s) does this PR fix?:

Fixes #24721

I'm unsure if the tests are actually helpful since the test DB uses a different connection string that is hard coded (https://github.com/grafana/grafana/blob/main/pkg/services/sqlstore/sqlutil/sqlutil.go#L31 and https://github.com/grafana/grafana/blob/main/pkg/tsdb/mysql/mysql_test.go#L1281-L1282) whereas when the actual data source is used, the connection string is based on the options passed via DataSourceInfo (https://github.com/grafana/grafana/blob/main/pkg/tsdb/mysql/mysql.go#L100-L110). I didn't find a way to use the same for both and also didn't find other cases where changes like this are tested.

There's a minimal example to reproduce the issue in isolation: https://github.com/exul/xorm-multi-statements and a more detailed analysis in the issue: #24721 (comment).

Special notes for your reviewer:

Please check that:

• It works as expected from a user's perspective.
• If this is a pre-GA feature, it is behind a feature toggle.
• The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

[CLAassistant]

All committers have signed the CLA.

[github-actions]

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[exul]

Is this something that would be useful/will be reviewed? Otherwise I'll close the PR since it will have more and more merge conflicts over time.

[zoltanbedi]

Hey @exul,
Thank you for creating this PR and sorry it slipped through the cracks. I think the changes makes sense. I'm going to take a look at it and will let you know soon.

[lwandz13]

The doc update looks great! Should this be backported to 10.0 and 10.1? There is a check for backporting that will fail unless backports are specified or if no backport is required add the no-backport label.

[exul]

Hey @zoltanbedi, no worries, it's totally understandable given the amount of PRs this repo has 🙂.

[gabor]

hi @exul thanks for the PR, one question.. why did you decide to make this a separate setting in the datasource config? do you think just enabling it by-default would cause problems?

(i do understand that with this mode, you may receive multiple result sets. but, i don't think that's backward-incompatible, considering such a queries right now return an error-message.)

but i may be missing something here. thanks!

[exul]

Hi @gabor, I decided to make it configurable because setting multiStatements is not the default in MySQL and the docs specifically mention that the behavior changes. I'm also not sure how forks/other versions of MySQL like MariaDB or AWS Aurora handle this.

[gabor]

an update... i think we'll have to make it so that when multiple queries return data, all the returned data is returned. i know you also mentioned this problem.

the reason is, i'm worried about backward compatibility. if we go live with a version where only results from the first query are returned, it will be very hard to release an improvement where all the results are returned, without breaking backward compatiblity.

of course, this just makes the problem larger 😿

[exul]

Yes, I totally understand that concern. I'll close this PR, since the support of multiple statements would probably also allow to get rid of this as a setting, which basically only leaves the extension of the connection string from this PR.

[gabor]

(EDIT: actually multi-result-set-support was added 3 months ago, so the question is not actual anymore, see grafana/grafana-plugin-sdk-go#735 )

@exul i did a little exploration around this area, and.. if i just add the conncnnstrstr += "&multiStatements=true" part, and then run this query:
SELECT 42; SELECT 43,
then i get back both 42 and 43

it seems to return "both results"

you mentioned rows.NextResultSet() being necessary for these scenarios to work correctly.. could you give an example of a scenario that is not working? (one that produces multiple result sets?)

[gabor]

(NOTE: it seems queries where the number-of-columns differs show an error-message currently, things like select 41 as a, 42 as b; select 43; .. but this is not the expected problem (of only showing the first result))

[gabor]

actually, it seems we are already calling NextResultSet ( https://github.com/grafana/grafana-plugin-sdk-go/blob/392629d2d7cdee638cf6df3c0b3423583685750f/data/sqlutil/sql.go#L68 ), though the whole experience is not the best for cases where the number of columsn differs.

EDIT: support added 3 months ago: grafana/grafana-plugin-sdk-go#735