Hacker News Reader

Hacker News

Comment by hinkley | Hacker News Reader

Parent Full thread

hinkley

dantillberg has the right of it.

We were doing some complicated fitness tests on our data to return appropriate rows to the user.

Actual:

SELECT * FROM things WHERE ... AND columnA>? AND columnB<5;

Expected:

SELECT * FROM things WHERE ... AND columnA>? AND columnB<?;

These sorts of mistakes tend to sneak back into projects that previously had this handled, so you do have to watch for regressions. (But that's not what happened in this case, if anyone is wondering.)

I don't know if there are any good ways to audit for these sorts of problems these days. At one point I joked that I was going to write my own database library that only accepted Prepared Statements and nothing else, but barring a full SQL query parser, I'm not entirely sure how I thought I was going to prevent someone from doing that.

View on HN

You really want this to be an SQL mode that the server enforces. Give me a query where anything that could be a parameter wasn't and I'll fail the query, write a nasty log, and maybe disconnect your client.

I think as an opinionated library, you could build a minimal parser, and fail queries that had values or didn't meet your parsing. I'm not super familiar with SQL these days, but I'd guess you could make a reasonable subset and have confidence that you would have false positives, but no false negatives.

Oracle, at least, has/had the problem that it can't use the values of parameters to affect the execution plan. So sometimes it's good to write in the values in the SQL statement.

If you write "select .. where account_id=?" then the query plan will probably be the same for any account_id, e.g. use the index, so that's fine, working as intended.

However, if you write "... where active=?" and active can only be true or false and there are a few thousand active entries but tens of millions of inactive ones, the query plan might be quite different for active=true and active=false. (Active=true would use an index, but active=false would make more sense to just sequentially read the whole table, as most rows will be used anyway.)

So if you have very few distinct values, the selection of the value influences the execution plan, and the value doesn't come from an untrusted source, it can be better to not use query parameters.

That is to say, 99% of the time you'll want to use query parameters, but rejecting queries which don't use them isn't a solution either, as you do need to avoid them sometimes, unfortunately.

> 99% of the time you'll want to use query parameters, but rejecting queries which don't use them isn't a solution either, as you do need to avoid them sometimes, unfortunately

This is one of the problems that I have with most SQL databases: they try to do the magic for you and make it hard to see what's going on under the hood. Databases aren't magic, they are just indexes, and there are only so many ways of organizing them. Providing a bit more transparency into how the data is stored and retrieved allow you to write efficient queries without the guesswork.

In that case, would you not use a partial index, and kill two birds with one stone?

On that same project we got into putting hints into the queries. I’m sure if push came to shove you could do that for situations like this one and solve the problem. Less portable that way, of course.

That's not entirely correct. A hard parse causes the optimizer to do bind variable peeking, i.e. it chooses the execution plan based on the bind variable value.

https://docs.oracle.com/cd/E11882_01/server.112/e41573/optim...

From the same source:

When choosing a plan, the optimizer only peeks at the bind value during the hard parse. This plan may not be optimal for all possible values.

The other way to fight things like this that however you want your users to behave should be the most straightforward way to use the code. If you library provides shortcuts to bad behavior, people will take them.

I was just rereading you comments and started searching for SQL linters and thinking how hard that would be in practice due to all the ways the queries get muddled in imperative code and a thought came to mind:

One of the ways we reduce people’s reliance on string concatenation is by providing interpolation. And I think that at least in some languages you can substitute your own logic for interpolation. You might be able to encourage bind variables that way.

    SELECT * FROM users 
        WHERE last_name=${surname} AND age>${legalDrinkingAge(state)} 
        ORDER BY birthdate ASC

This is mostly how entity framework behaves. Last I checked they had some sort of nonsense checking whether a variable was in _any_ closure before deciding whether to bind it or substitute it, but give or take the rough edges it will bind variables provided in parameterized strings.

Reply on news.ycombinator.com