Speeding up a stored procedure

Question

Is there any way to optimize this stored procedure? Maybe something instead of so many joins? It takes some time to execute. Maybe there are other options that I could look into?

    SET ANSI_NULLS ON
    GO
    SET QUOTED_IDENTIFIER ON
    GO

    ALTER PROCEDURE [stimulator].[GetLastMessages2]
        @serviceId bigint,
        @stimulatorId int,
        @from datetime,
        @to datetime,
        @atLeast int = null,
        @lessThan int = null,
        @lastHourLessThan int = null,
        @hourFactor int = 24,
        @lastXHourLessThan int = null,
        @totalLessThan int = null
    AS
    BEGIN

        SET NOCOUNT ON;

        DECLARE @H1Ago datetime
        DECLARE @HXAgo datetime
        DECLARE @Now datetime
        DECLARE @Today datetime

        SET @H1Ago = DATEADD(hh, -1, getdate())
        SET @HXAgo = DATEADD(hh, -@hourFactor, getdate())
        SET @Now = getdate()
        SET @Today = DATEADD(dd, 0, DATEDIFF(dd, 0, @Now))

        SELECT tel_telewidz,
                acn_id,
                guid,
                [First],
                [Last],
                [inCount],
                isNULL(lastHourCount, 0) lastHourCount,
                isNULL(lastXCount, 0) lastXCount,
                isNULL(allCount, 0) allCount,
                lastStimulation,
                body,
                STUFF((SELECT '|' + body
                            FROM SP_BILLING_DW.hurt.MESSAGE_In inmsg
                            WHERE inmsg.tel_telewidz = d.tel_telewidz
                              AND service_id = @serviceId
                              AND creation_time > @from
                              AND creation_time <= @to
                            FOR XML PATH('')), 1, 1, '') [allbody]
        FROM (
        SELECT tel_telewidz, acn_id, guid, creation_time, service_id
            ,ROW_NUMBER() OVER(PARTITION BY tel_telewidz ORDER BY creation_time) [No]
            ,COUNT(*) OVER(PARTITION BY tel_telewidz) [inCount]
            ,MIN(creation_time) OVER(PARTITION BY tel_telewidz) [First]
            ,MAX(creation_time) OVER(PARTITION BY tel_telewidz) [Last]
            ,body
        FROM SP_BILLING_DW.hurt.MESSAGE_In
        WHERE 
            creation_time > @from
        AND
            creation_time <= @to
        AND
            service_id = @serviceId     
        ) d
        LEFT JOIN 
        (
            SELECT msisdn, count(*) lastHourCount
            FROM stimulator.SentMessages
            WHERE serviceId = @serviceId
              AND stimulatorId = @stimulatorId
              AND sendDate > @H1Ago
            GROUP BY msisdn
        ) lh ON d.tel_telewidz = lh.msisdn
        LEFT JOIN 
        (
            SELECT msisdn, count(*) lastXCount
            FROM stimulator.SentMessages
            WHERE serviceId = @serviceId
              AND stimulatorId = @stimulatorId
              AND sendDate > @HXAgo
            GROUP BY msisdn
        ) ld ON d.tel_telewidz = ld.msisdn
        LEFT JOIN 
        (
            SELECT msisdn, count(*) allCount
            FROM stimulator.SentMessages
            WHERE serviceId = @serviceId
              AND stimulatorId = @stimulatorId
              AND sendDate > @from
            GROUP BY msisdn
        ) nDays  ON d.tel_telewidz = nDays.msisdn
        LEFT JOIN 
        (
            SELECT msisdn, max(sendDate) lastStimulation
            FROM stimulator.SentMessages
            WHERE serviceId = @serviceId
              AND stimulatorId = @stimulatorId
              AND sendDate > @from
            GROUP BY msisdn
        ) ls  ON d.tel_telewidz = ls.msisdn

        WHERE 
            d.[No] = d.[inCount]
        AND 
        ((@atLeast IS NULL) OR (d.[inCount] >= @atLeast))

    AND
        ((@lessThan IS NULL) OR (d.[inCount] < @lessThan))
    AND
        ((@lastHourLessThan IS NULL) OR (lastHourCount IS NULL) OR (lastHourCount < @lastHourLessThan))
    AND
        ((@lastXHourLessThan IS NULL) OR (lastXCount IS NULL) OR (lastXCount < @lastXHourLessThan))
    AND
        ((@totalLessThan IS NULL) OR (allCount IS NULL) OR (allCount < @totalLessThan))

    END

Here are the tables involved:

stimulator.SentMessages

        CREATE TABLE [stimulator].[SentMessages](
            [Id] [int] IDENTITY(1,1) NOT NULL,
            [serviceId] [bigint] NOT NULL,
            [stimulatorId] [int] NOT NULL,
            [guid] [uniqueidentifier] NOT NULL,
            [sendDate] [datetime] NOT NULL,
            [msisdn] [varchar](16) NOT NULL,
            [body] [varchar](1024) NOT NULL,
         CONSTRAINT [PK_st_sentmess] PRIMARY KEY CLUSTERED 
        (
            [Id] ASC
        )WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON) ON [PRIMARY]
        ) ON [PRIMARY]

        GO

        SET ANSI_PADDING OFF
        GO

        ALTER TABLE [stimulator].[SentMessages] ADD  CONSTRAINT [DF_SentMessages_sendDate]  DEFAULT (getdate()) FOR [sendDate]
        GO

hurt.MESSAGE_In

    CREATE TABLE [hurt].[MESSAGE_In](
        [ID] [bigint] NOT NULL,
        [guid] [uniqueidentifier] NOT NULL,
        [tel_telewidz] [varchar](50) NULL,
        [tel_operator] [varchar](50) NULL,
        [msg_type] [char](1) NULL,
        [direction] [char](1) NULL,
        [udh] [varchar](900) NULL,
        [body] [varchar](900) NULL,
        [mms_subject] [varchar](900) NULL,
        [acn_id] [smallint] NULL,
        [require_status_report] [tinyint] NULL,
        [creation_time] [datetime] NOT NULL,
        [is_spam] [tinyint] NULL,
        [is_binary_sms] [tinyint] NULL,
        [partner_guid] [varchar](50) NULL,
        [reply_to_guid] [uniqueidentifier] NULL,
        [expected_no_of_parts] [tinyint] NULL,
        [transmitted_parts_no] [tinyint] NULL,
        [data_insertu] [datetime] NOT NULL,
        [data_update] [datetime] NOT NULL,
        [tel_operator_nadpis] [varchar](50) NULL,
        [machineName] [varchar](50) NULL,
        [service_id] [int] NULL,
        [client_id] [int] NULL,
        [partner_id] [int] NULL,
        [operatorMnemonic] [char](1) NULL,
        [status_wysylki] [tinyint] NULL,
        [data_wysylki] [datetime] NULL,
        [opis_wysylki] [varchar](500) NULL,
        [status_dostarczenia] [tinyint] NULL,
        [data_dostarczenia] [datetime] NULL,
        [opis_dostarczenia] [varchar](500) NULL,
        [status_obslugi] [tinyint] NULL,
        [has_ack] [tinyint] NULL,
        [in_sent_time] [datetime] NULL,
        [ev_ServiceUpdate] [datetime] NULL,
     CONSTRAINT [PK_MESSAGE_In_new] PRIMARY KEY CLUSTERED 
    (
        [creation_time] ASC,
        [ID] ASC
    )WITH (PAD_INDEX = OFF, STATISTICS_NORECOMPUTE = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS = ON, ALLOW_PAGE_LOCKS = ON)
    )

    GO

    SET ANSI_PADDING OFF
    GO

Answer 1

A couple of things to think about while writing your query....

Variable Naming

ALTER PROCEDURE [stimulator].[GetLastMessages2]
    @serviceId bigint,
    @stimulatorId int,
    @from datetime,
    @to datetime,
    @atLeast int = null,
    @lessThan int = null,
    @lastHourLessThan int = null,
    @hourFactor int = 24,
    @lastXHourLessThan int = null,
    @totalLessThan int = null
AS
BEGIN

    SET NOCOUNT ON;

    DECLARE @H1Ago datetime
    DECLARE @HXAgo datetime
    DECLARE @Now datetime
    DECLARE @Today datetime

    SET @H1Ago = DATEADD(hh, -1, getdate())
    SET @HXAgo = DATEADD(hh, -@hourFactor, getdate())
    SET @Now = getdate()
    SET @Today = DATEADD(dd, 0, DATEDIFF(dd, 0, @Now))

What are these variables?

• @from
• @to
• @atLeast
• @lessThan
• @lastHourLessThan
• ...

What is their purpose? you should see if you can find more descriptive names for these, don't try to make them as short as possible because that makes the code obscure.

---

Extraneous Variables

Also, get rid of @Now you are not really using it for anything, instead write @Today like this

DECLARE @Today DATETIME
SET @Today = DATEADD(dd, 0, DATEDIFF(dd, 0, GETDATE())

---

Formatting

I noticed that not all the reserved keywords were capitalized, Be Consistent with formatting otherwise you will get lost in your code.

I am talking about...

• The type on your variable declarations

  DECLARE @Today datetime
  

  should be

  DECLARE @Today DATETIME
  

• Database functions

  SET @Now = getdate()
  

  should be

  SET @Now = GETDATE()
  

I only bring this up because of your inconsistent use of CAPS

List of keywords from your code that should be in all caps:

1. BIGINT
2. INT
3. DATETIME
4. NULL
5. GETDATE()
6. ISNULL()
7. COUNT()
8. MAX()

You also have a spot where the code is not indented, I am not sure if that is copy paste to CodeReview or not though. On the same piece of code you change your style on the next nested block.

    FROM (
    SELECT tel_telewidz, acn_id, guid, creation_time, service_id
        ,ROW_NUMBER() OVER(PARTITION BY tel_telewidz ORDER BY creation_time) [No]
        ,COUNT(*) OVER(PARTITION BY tel_telewidz) [inCount]
        ,MIN(creation_time) OVER(PARTITION BY tel_telewidz) [First]
        ,MAX(creation_time) OVER(PARTITION BY tel_telewidz) [Last]
        ,body
    FROM SP_BILLING_DW.hurt.MESSAGE_In
    WHERE 
        creation_time > @from
    AND
        creation_time <= @to
    AND
        service_id = @serviceId     
    ) d
    LEFT JOIN 
    (
        SELECT msisdn, count(*) lastHourCount
        FROM stimulator.SentMessages
        WHERE serviceId = @serviceId
          AND stimulatorId = @stimulatorId
          AND sendDate > @H1Ago
        GROUP BY msisdn
    ) lh ON d.tel_telewidz = lh.msisdn
    LEFT JOIN 
    ....

this should look more like this:

    FROM (
        SELECT tel_telewidz, acn_id, guid, creation_time, service_id
            ,ROW_NUMBER() OVER(PARTITION BY tel_telewidz ORDER BY creation_time) [No]
            ,COUNT(*) OVER(PARTITION BY tel_telewidz) [inCount]
            ,MIN(creation_time) OVER(PARTITION BY tel_telewidz) [First]
            ,MAX(creation_time) OVER(PARTITION BY tel_telewidz) [Last]
            ,body
        FROM SP_BILLING_DW.hurt.MESSAGE_In
        WHERE creation_time > @from
            AND creation_time <= @to
            AND service_id = @serviceId) d
    LEFT JOIN (
        SELECT msisdn, count(*) lastHourCount
        FROM stimulator.SentMessages
        WHERE serviceId = @serviceId
          AND stimulatorId = @stimulatorId
          AND sendDate > @H1Ago
        GROUP BY msisdn
    ) lh ON d.tel_telewidz = lh.msisdn
    LEFT JOIN 
    ...

that first nested select statement was very different from the rest.

---

Things I might change in the code

I would probably turn the queries inside of your LEFT JOINs into Temporary Tables and select from them inside the joins. It might speed up the query a little bit, but don't quote me on that (unless I am right).

I know that it will make your query a lot cleaner and easier to read, which should always translate into efficiency somewhere in the whole process.

Answer 2

Great answer by @Malachi. Let's talk about performance.

Variables

You have lots of variables and this can slow it down. I see at least two that could be completely eliminated:

SET @Now = getdate()
SET @Today = DATEADD(dd, 0, DATEDIFF(dd, 0, @Now))

There is not really a point in setting a variable as a place holder for a built-in SQL function. You could replace every instance of @Now and @Today with GETDATE() and your procedure should behave identically.

You have this:

    @hourFactor int = 24,

And this:

    SET @HXAgo = DATEADD(hh, -@hourFactor, getdate())

I would say you can eliminate @hourFactor entirely, since it's always 24, and instead write:

    SET @HXAgo = DATEADD(hh, -24, GETDATE())

Nested subqueries

Nested subqueries can get really expensive when you have multiple levels of nesting as you do here. I would suggest that you try to add the following at the beginning and do a performance analysis to see where the SQL engine is spending the most resources.

SET SHOWPLAN_XML ON
GO

Please note you will have to remove that to get the actual query result set again so I recommend to make a copy of the procedure and modify that one for testing. Temporary tables would not only make your code easier to read but may improve performance by nesting a SELECT on a filtered result set instead of the whole set.

Joins

This is likely what is causing the most performance problems. JOIN is one of the most expensive operations for the SQL Server engine. Of course they are absolutely crucial to using SQL, but in your case your have 4 LEFT JOIN on the same table (stimulator.SentMessages) with almost identical WHERE conditions and that seems redundant. I would combine them all in one TEMP TABLE with something like this:

DELETE TABLE #tt_stimulatorSentMessages IF EXISTS;
CREATE TABLE #tt_stimulatorSentMessages(
  [Id] [int] IDENTITY(1,1) NOT NULL,
  [serviceId] [bigint] NOT NULL,
  [stimulatorId] [int] NOT NULL,
  [guid] [uniqueidentifier] NOT NULL,
  [sendDate] [datetime] NOT NULL,
  [msisdn] [varchar](16) NOT NULL,
  [body] [varchar](1024) NOT NULL,
  CONSTRAINT [PK_st_sentmess] PRIMARY KEY CLUSTERED
);
INSERT INTO #tt_SentMessages 
SELECT * FROM 
FROM stimulator.SentMessages
WHERE serviceId = @serviceId
AND stimulatorId = @stimulatorId;

That way you can simplify your JOIN operations like this. That way you operate on a potentially much smaller result set, and it looks cleaner.

    LEFT JOIN 
    (
        SELECT msisdn, count(*) lastHourCount
        FROM #tt_SentMessages 
        WHERE sendDate > @H1Ago
        GROUP BY msisdn
    ) lh ON d.tel_telewidz = lh.msisdn
-- etc.

Other things

These things would not really improve performance much, if any, but they would make the query easier for you or the next programmer to maintain.

1. Use CTEs (or temp tables) for your nested subqueries. That way the logic of the procedure is easier to follow.

2. As @Malachi said, use better names for variables, as well as aliases. A table alias like dh or dl is pretty cryptic and says nothing about the content or purpose.

3. Add markdowns that explain the context and logic of what the code does, or the meaning of a variable, etc. It takes almost no time and makes the code a lot simpler to go through.

I will compliment you on being very explicit and on not using that devil BETWEEN for your date filtering.

Answer 3

General Remarks

First off, you want to make this a dynamic SQL statement, or use OPTION(RECOMPILE): SQL Server has issues with the null-or-provided-parameter pattern (that is, stuff like ((@atLeast IS NULL) OR (d.[inCount] >= @atLeast))).

I also feel a disconnect about your parameters - you're provided the @from and @to, but you're also using the current time (GETDATE()) for some things. In essence, it's making your procedure do two (somewhat) unrelated things - the last message received over a given range, and the count of the last messages sent. Conceptually, this should be broken up into two separate procedures. Something like GetMessagesReceived(...) and GetCountOfMessagesSentSince(...). That, or the @from and @to should also control the other counts. Note that the use of GETDATE() makes this procedure hard to test (you need to modify the data each time), and may be another reason to recompile.

---

Indices

For MESSAGE_In, you need starting with [service_id, tel_telewidz, creation_time].

For SentMessages, you need one or more starting with some combination of serviceId, stimulatorId, msisdn, with the 4th column being sendDate (which one is best depends on your data distribution - the optimizer should be recommending one of them anyways).

---

Query

The most recent message:
It's not clear why you're getting information for the most recent message, along with the aggregate information. This feels like it's also a victim of the query doing too many things. Still, we can tighten that subquery up - the row-index can be removed, assuming that message time is unique (if not, your sort ordering is unstable, which can be just as bad).

SELECT tel_telewidz, acn_id, guid, body, creation_time, 
       COUNT(*) OVER(PARTITION BY tel_telewidz) inCount
       MIN(creation_time) OVER(PARTITION BY tel_telewidz) earliest
       MAX(creation_time) OVER(PARTITION BY tel_telewidz) latest

FROM SP_BILLING_DW.hurt.MESSAGE_In
WHERE creation_time >= @from
      AND creation_time < @to
      AND service_id = @serviceId    

Try not to use delimited or escaped names if at all possible. Notice that I flipped which end of the range query over creation_time was inclusive - when querying dates/times/timestamps, always use lower-bound inclusive, upper-bound exclusive (this actually applies to positive ranges for all contiguous types - that is, everything except integers). I'd have ditched the "extra" creation_time column, but we need that to get the last row.

Subqueries over SentMessages

We can actually combine all of these. The last two are trivial (given they operator over the same set of rows), adding in the first to is a little more interesting. For that, we need to utilize the fact that COUNT(<expression>) will ignore rows when the given expression returns null. That, and a way to make inline tables...

SELECT msisdn, COUNT(oneHourAgo) lastHourCount,
               COUNT(someHoursAgo) lastXCount,
               COUNT(sinceRangeStart) allCount,
               MAX(sendDate) lastStimulation
FROM stimulator.SentMessages
LEFT JOIN (VALUES(@H1Ago)) a(oneHourAgo)
       ON sendDate >= oneHourAgo
LEFT JOIN (VALUES(@HXAgo)) b(someHoursAgo)
       ON sendDate >= someHoursAgo
LEFT JOIN (VALUES(@from)) c(sinceRangeStart)
       ON sendDate >= sinceRangeStart
WHERE serviceId = @serviceId
      AND stimulatorId = @stimulatorId
      AND sendDate >= (SELECT MIN(starts)
                       FROM (VALUES(@H1Ago), (@HXAgo), (@from)) n(starts))
GROUP BY msisdn

(Note that the lower ranges are now inclusive!)

Concatenated message string
For dealing with the concatenated messages, you also need to make some changes. Remember that in creating the XML data, it's going to want to "escape" certain characters - if you have any of these in your data, they'd be malformed. Also, the ordering of your messages is undefined - might as well just make it most-recent first:

STUFF((SELECT '|' + body
       FROM SP_BILLING_DW.hurt.MESSAGE_In inmsg
       WHERE inmsg.tel_telewidz = Incoming.tel_telewidz
             AND inmsg.service_id = @serviceId
             AND inmsg.creation_time >= @from
             AND inmsg.creation_time < @to
       ORDER BY inmsg.creation_time DESC
       FOR XML PATH(''), TYPE).VALUE('.', 'VARCHAR(MAX)'), 1, 1, '')

(and of course, I had to switch which side of the timestamp range was inclusive)

Combined query
And everything together, along with the remaining conditions:

SELECT tel_telewidz, acn_id, guid,
       earliest, latest, inCount,
       COALESCE(lastHourCount, 0) lastHourCount,
       COALESCE(lastXCount, 0) lastXCount,
       COALESCE(allCount, 0) allCount,
       lastStimulation,
       body,
       STUFF((SELECT '|' + body
              FROM SP_BILLING_DW.hurt.MESSAGE_In inmsg
              WHERE inmsg.tel_telewidz = Incoming.tel_telewidz
                    AND inmsg.service_id = @serviceId
                    AND inmsg.creation_time >= @from
                    AND inmsg.creation_time < @to
              ORDER BY inmsg.creation_time DESC
              FOR XML PATH(''), TYPE).VALUE('.', 'VARCHAR(MAX)'), 1, 1, '')

FROM (SELECT tel_telewidz, acn_id, guid, body, creation_time, 
             COUNT(*) OVER(PARTITION BY tel_telewidz) inCount
             MIN(creation_time) OVER(PARTITION BY tel_telewidz) earliest
             MAX(creation_time) OVER(PARTITION BY tel_telewidz) latest
      FROM SP_BILLING_DW.hurt.MESSAGE_In
      WHERE creation_time >= @from
            AND creation_time < @to
            AND service_id = @serviceId) Incoming 
LEFT JOIN (SELECT msisdn, NULLIF(COUNT(oneHourAgo), 0) lastHourCount,
                          NULLIF(COUNT(someHoursAgo), 0) lastXCount,
                          NULLIF(COUNT(sinceRangeStart), 0) allCount,
                          MAX(sendDate) lastStimulation
           FROM stimulator.SentMessages
           LEFT JOIN (VALUES(@H1Ago)) a(oneHourAgo)
                  ON sendDate >= oneHourAgo
           LEFT JOIN (VALUES(@HXAgo)) b(someHoursAgo)
                  ON sendDate >= someHoursAgo
           LEFT JOIN (VALUES(@from)) c(sinceRangeStart)
                  ON sendDate >= sinceRangeStart
           WHERE serviceId = @serviceId
                 AND stimulatorId = @stimulatorId
                  AND sendDate >= (SELECT MIN(starts)
                                   FROM (VALUES(@H1Ago), 
                                               (@HXAgo), 
                                               (@from)) n(starts))
           GROUP BY msisdn) Outgoing
       ON Outgoing.msisdn = Incoming.tel_telewidz
WHERE Incoming.creation_time = Incoming.latest
      AND (@atLeast IS NULL OR Incoming.inCount >= @atLeast)
      AND (@lessThan IS NULL OR Incoming.inCount < @lessThan)
      AND (@lastHourLessThan IS NULL OR lastHourCount IS NULL
           OR lastHourCount < @lastHourLessThan)
      AND (@lastXHourLessThan IS NULL OR lastXCount IS NULL 
           OR lastXCount < @lastXHourLessThan)
      AND (@totalLessThan IS NULL OR allCount IS NULL 
          OR allCount < @totalLessThan)

Note that I had to add NULLIF(...) to the subquery counts to maintain the semantics of the original query. I feel like I want to change that whole portion into an INNER JOIN (and use a HAVING clause to throw out results on out-of-range results), but there's no guarantee rows actually exist, so.... Note that the current form will include rows if there are no counts at all from the subquery; however, logically it's actually a zero count, so maybe it should still be excluded? I dunno.