felixge / sqlbench

sqlbench measures and compares the execution time of one or more SQL queries.
MIT License
368 stars 11 forks source link

How to approach supporting alternative drivers #2

Open aidansteele opened 4 years ago

aidansteele commented 4 years ago

In the README (and I think I remember on Twitter) you mentioned being open to PRs for alternative SQL implementations. At $dayjob we use MS SQL Server and so I thought I might try my hand at a PR to add support for that. Before getting started, I thought it would be useful to see if you had any thoughts / opinions on how this should be done.

Some current code to think about

There are at least a few things worth thinking about. The first is how we would specify the driver name, e.g. currently "pgx" here: https://github.com/felixge/sqlbench/blob/a7422d928ac6fd10ee4e91e52619da3be738ca0e/main.go#L77

And how it relates to the connection string here: https://github.com/felixge/sqlbench/blob/a7422d928ac6fd10ee4e91e52619da3be738ca0e/main.go#L35

There's also the EXPLAIN .. FORMAT JSON duration thing which I assume has equivalents in SQL Server and others, but to be honest right now I don't care so much about that, I think it would be fine to just have client-measurement only support at least to begin with!

https://github.com/felixge/sqlbench/blob/a7422d928ac6fd10ee4e91e52619da3be738ca0e/query_duration.go#L76

Some options

I guess the "easy" option is to add another command-line flag where the user can specify a driver name if they don't want pgx. This is probably good enough for MVP - anyone using this tool at this stage is probably happy to get their hands dirty!

Another option could be to deduce the driver name based on the schema in the connection string URL. I'm used to this from my days of Rails, but come to think of it, I've never seen it at $dayjob for MS SQL Server.. so maybe it wouldn't be obvious, and we'd have to document what schemas are available πŸ€”

Any thoughts? I'm inclined to just go with the easy option and not over-engineer this thing and lock this project into unhelpful complexity on day 2 πŸ˜‚

felixge commented 4 years ago

@aidansteele thanks for this thoughtful analysis!

There's also the EXPLAIN .. FORMAT JSON duration thing which I assume has equivalents in SQL Server and others, but to be honest right now I don't care so much about that, I think it would be fine to just have client-measurement only support at least to begin with!

Supporting only client measurements in the beginning is fine. But it'd be nice to give a good error message when somebody is trying to use the wrong method with the wrong database.

Another option could be to deduce the driver name based on the schema in the connection string URL. I'm used to this from my days of Rails, but come to think of it, I've never seen it at $dayjob for MS SQL Server.. so maybe it wouldn't be obvious, and we'd have to document what schemas are available πŸ€”

I think picking the driver based on the schema in the connection URL would be my preference. I think most users of sqlbench shouldn't care about the driver that is being used, so I'd prefer to not expose an option like this for now. But I don't feel super strongly about it.

Anyway, as far as I'm concerned the best way forward would be to "hack together" a proof of concept on your end and open a PR with it. No need to worry about it being super nice (unless you want to), we can refactor and improve things in follow-up iterations or PRs. I mostly care about not breaking PostgreSQL for now : p.

One Q: Will we be able to test SQL server in the CI like PostgreSQL? I know there is a docker image these days, but in my mind SQL Server was always $propietary : ).