Little Bobby Tables’ mother says you should always sanitise your data input. Except that I think she’s wrong. The SQL Injection aspect is for another post, where I’ll show you why I think SQL Injection is the same kind of attack as many other attacks, such as the old buffer overflow, but here I want to have a bit of a whinge about the way that some people sanitise data input, and even have a whinge about people who insist on using stored procedures for SSRS reports.

Let me say that again, in case you missed it the first time:

I want to have a whinge about people who insist on using stored procedures for SSRS reports.

Let’s look at the data input sanitisation aspect – except that I’m going to call it ‘parameter validation’. I’m talking about code that looks like this:

create procedure dbo.GetMonthSummaryPerSalesPerson(@eomdate datetime) as
begin
    /* First check that @eomdate is a valid date */
    if isdate(@eomdate) != 1
    begin
        select 'Please enter a valid date' as ErrorMessage;
        return;
    end

    /* Then check that time has passed since @eomdate */
    if datediff(day,@eomdate,sysdatetime()) < 5
    begin
        select 'Sorry - EOM is not complete yet' as ErrorMessage;
        return;
    end
   
    /* If those checks have succeeded, return the data */
    select SalesPersonID, count(*) as NumSales, sum(TotalDue) as TotalSales
    from Sales.SalesOrderHeader
    where OrderDate >= dateadd(month,-1,@eomdate)
        and OrderDate < @eomdate
    group by SalesPersonID
    order by SalesPersonID;
end

Notice that the code checks that a date has been entered. Seriously??!! This must only be to check for NULL values being passed in, because anything else would have to be a valid datetime to avoid an error.

The other check is maybe fair enough, but I still don’t like it.

The two problems I have with this stored procedure are the result sets and the small fact that the stored procedure even exists in the first place. But let’s consider the first one of these problems for starters. I’ll get to the second one in a moment.

If you read Jes Borland (@grrl_geek)’s recent post about returning multiple result sets in Reporting Services, you’ll be aware that Reporting Services doesn’t support multiple results sets from a single query. And when it says ‘single query’, it includes ‘stored procedure call’. It’ll only handle the first result set that comes back. But that’s okay – we have RETURN statements, so our stored procedure will only ever return a single result set.  Sometimes that result set might contain a single field called ErrorMessage, but it’s still only one result set.

Except that it’s not okay, because Reporting Services needs to know what fields to expect. Your report needs to hook into your fields, so SSRS needs to have a way to get that information. For stored procs, it uses an option called FMTONLY.

When Reporting Services tries to figure out what fields are going to be returned by a query (or stored procedure call), it doesn’t want to have to run the whole thing. That could take ages. (Maybe it’s seen some of the stored procedures I’ve had to deal with over the years!)

So it turns on FMTONLY before it makes the call (and turns it off again afterwards). FMTONLY is designed to be able to figure out the shape of the output, without actually running the contents. It’s very useful, you might think.

set fmtonly on
exec dbo.GetMonthSummaryPerSalesPerson '20030401';
set fmtonly off

Without the FMTONLY lines, this stored procedure returns a result set that has three columns and fourteen rows. But with FMTONLY turned on, those rows don’t come back.

But what I do get back hurts Reporting Services.

It doesn’t run the stored procedure at all. It just looks for anything that could be returned and pushes out a result set in that shape. Despite the fact that I’ve made sure that the logic will only ever return a single result set, the FMTONLY option kills me by returning three of them.

It would have been much better to push these checks down into the query itself.

alter procedure dbo.GetMonthSummaryPerSalesPerson(@eomdate datetime) as
begin
    select SalesPersonID, count(*) as NumSales, sum(TotalDue) as TotalSales
    from Sales.SalesOrderHeader
    where
    /* Make sure that @eomdate is valid */
        isdate(@eomdate) = 1
    /* And that it's sufficiently past */
    and datediff(day,@eomdate,sysdatetime()) >= 5
    /* And now use it in the filter as appropriate */
    and OrderDate >= dateadd(month,-1,@eomdate)
    and OrderDate < @eomdate
    group by SalesPersonID
    order by SalesPersonID;
end

Now if we run it with FMTONLY turned on, we get the single result set back. But let’s consider the execution plan when we pass in an invalid date.

First let’s look at one that returns data. I’ve got a semi-useful index in place on OrderDate, which includes the SalesPersonID and TotalDue fields. It does the job, despite a hefty Sort operation.

…compared to one that uses a future date:

You might notice that the estimated costs are similar – the Index Seek is still 28%, the Sort is still 71%. But the size of that arrow coming out of the Index Seek is a whole bunch smaller.

The coolest thing here is what’s going on with that Index Seek. Let’s look at some of the properties of it.

Glance down it with me… Estimated CPU cost of 0.0005728, 387 estimated rows, estimated subtree cost of 0.0044385, ForceSeek false, Number of Executions 0.

That’s right – it doesn’t run. So much for reading plans right-to-left...

The key is the Filter on the left of it. It has a Startup Expression Predicate in it, which means that it doesn’t call anything further down the plan (to the right) if the predicate evaluates to false.

Using this method, we can make sure that our stored procedure contains a single query, and therefore avoid any problems with multiple result sets. If we wanted, we could always use UNION ALL to make sure that we can return an appropriate error message.

alter procedure dbo.GetMonthSummaryPerSalesPerson(@eomdate datetime) as
begin
    select SalesPersonID, count(*) as NumSales, sum(TotalDue) as TotalSales, /*Placeholder: */ '' as ErrorMessage
    from Sales.SalesOrderHeader
    where
    /* Make sure that @eomdate is valid */
        isdate(@eomdate) = 1
    /* And that it's sufficiently past */
    and datediff(day,@eomdate,sysdatetime()) >= 5
    /* And now use it in the filter as appropriate */
    and OrderDate >= dateadd(month,-1,@eomdate)
    and OrderDate < @eomdate
    group by SalesPersonID

    /* Now include the error messages */
    union all
    select 0, 0, 0, 'Please enter a valid date' as ErrorMessage
    where isdate(@eomdate) != 1
    union all
    select 0, 0, 0, 'Sorry - EOM is not complete yet' as ErrorMessage
    where datediff(day,@eomdate,sysdatetime()) < 5

    order by SalesPersonID;
end

But still I don’t like it, because it’s now a stored procedure with a single query. And I don’t like stored procedures that should be functions.

That’s right – I think this should be a function, and SSRS should call the function. And I apologise to those of you who are now planning a bonfire for me. Guy Fawkes’ night has already passed this year, so I think you miss out. (And I’m not going to remind you about when the PASS Summit is in 2012.)

create function dbo.GetMonthSummaryPerSalesPerson(@eomdate datetime)
returns table as
return (
    select SalesPersonID, count(*) as NumSales, sum(TotalDue) as TotalSales, '' as ErrorMessage
    from Sales.SalesOrderHeader
    where
    /* Make sure that @eomdate is valid */
        isdate(@eomdate) = 1
    /* And that it's sufficiently past */
    and datediff(day,@eomdate,sysdatetime()) >= 5
    /* And now use it in the filter as appropriate */
    and OrderDate >= dateadd(month,-1,@eomdate)
    and OrderDate < @eomdate
    group by SalesPersonID
    union all
    select 0, 0, 0, 'Please enter a valid date' as ErrorMessage
    where isdate(@eomdate) != 1
    union all
    select 0, 0, 0, 'Sorry - EOM is not complete yet' as ErrorMessage
    where datediff(day,@eomdate,sysdatetime()) < 5
);

We’ve had to lose the ORDER BY – but that’s fine, as that’s a client thing anyway. We can have our reports leverage this stored query still, but we’re recognising that it’s a query, not a procedure. A procedure is designed to DO stuff, not just return data. We even get entries in sys.columns that confirm what the shape of the result set actually is, which makes sense, because a table-valued function is the right mechanism to return data.

And we get so much more flexibility with this.

If you haven’t seen the simplification stuff that I’ve preached on before, jump over to http://bit.ly/SimpleRob and watch the video of when I broke a microphone and nearly fell off the stage in Wales. You’ll see the impact of being able to have a simplifiable query. You can also read the procedural functions post I wrote recently, if you didn’t follow the link from a few paragraphs ago.

So if we want the list of SalesPeople that made any kind of sales in a given month, we can do something like:

select SalesPersonID
from dbo.GetMonthSummaryPerSalesPerson(@eomonth)
order by SalesPersonID;

This doesn’t need to look up the TotalDue field, which makes a simpler plan.

select *
from dbo.GetMonthSummaryPerSalesPerson(@eomonth)
where SalesPersonID is not null
order by SalesPersonID;

This one can avoid having to do the work on the rows that don’t have a SalesPersonID value, pushing the predicate into the Index Seek rather than filtering the results that come back to the report.

If we had joins involved, we might see some of those being simplified out. We also get the ability to include query hints in individual reports. We shift from having a single-use stored procedure to having a reusable stored query – and isn’t that one of the main points of modularisation?

Stored procedures in Reporting Services are just a bit limited for my liking. They’re useful in plenty of ways, but if you insist on using stored procedures all the time rather that queries that use functions – that’s rubbish.

@rob_farley