views:

3917

answers:

5

Hello Stack Overflow!

I am having to use triggers in MSSQL for the first time, well triggers in general. Having read around and tested this myself I realise now that a trigger fires per command and not per row inserted, deleted or updated.

The entire thing is some statistics for an advertising system. Our main stat table is rather large and doesn't contain the data in a way that makes sense in most cases. It contains one row per advert clicked, viewed and etc. As a user one is more inclined to want to view this as day X has Y amount of clicks and Z amount of views and so forth. We have done this purely based on a SQL query so far, getting this sort of report from the main table, but as the table has grown so does the time for that query to execute. Because of this we have opted for using triggers to keep another table updated and hence making this a bit easier on the SQL server.

My issue is now to get this working with multiple records. What I have done is to create 2 stored procedures, one for handling the operation of an insert, and one for a delete. My insert trigger (written to work with a single record) then graps the data off the Inserted table, and sends it off to the stored procedure. The delete trigger works in the same way, and (obviously?) the update trigger does the same as a delete + an insert.

My issue is now how to best do this with multiple records. I have tried using a cursor, but as far as I have been able to read and see myself, this performs really badly. I have considered writing some "checks" as well - as in checking to see IF there are multiple records in the commands and then go with the cursor, and otherwise simply just avoid this. Anyhow, here's my solution with a cursor, and im wondering if there's a way of doing this better?

CREATE TRIGGER [dbo].[TR_STAT_INSERT]
   ON  [iqdev].[dbo].[Stat]
   AFTER INSERT
AS 
BEGIN
    SET NOCOUNT ON;

    DECLARE @Date DATE 
    DECLARE @CampaignId BIGINT
    DECLARE @CampaignName varchar(500)
    DECLARE @AdvertiserId BIGINT
    DECLARE @PublisherId BIGINT
    DECLARE @Unique BIT
    DECLARE @Approved BIT
    DECLARE @PublisherEarning money
    DECLARE @AdvertiserCost money
    DECLARE @Type smallint

    DECLARE InsertCursor CURSOR FOR SELECT Id FROM Inserted
    DECLARE @curId bigint

    OPEN InsertCursor

    FETCH NEXT FROM InsertCursor INTO @curId

    WHILE @@FETCH_STATUS = 0
    BEGIN

     SELECT @Date = [Date], @PublisherId = [PublisherCustomerId], @Approved = [Approved], @Unique = [Unique], @Type = [Type], @AdvertiserCost = AdvertiserCost, @PublisherEarning = PublisherEarning
     FROM Inserted
     WHERE Id = @curId

     SELECT @CampaignId = T1.CampaignId, @CampaignName = T2.Name, @AdvertiserId = T2.CustomerId
     FROM Advert AS T1
     INNER JOIN Campaign AS T2 on T1.CampaignId = T2.Id
     WHERE T1.Id = (SELECT AdvertId FROM Inserted WHERE Id = @curId)

     EXEC ProcStatInsertTrigger @Date, @CampaignId, @CampaignName, @AdvertiserId, @PublisherId, @Unique, @Approved, @PublisherEarning, @AdvertiserCost, @Type

     FETCH NEXT FROM InsertCursor INTO @curId
    END

    CLOSE InsertCursor
    DEALLOCATE InsertCursor
END

The stored procedure is rather big and intense and I do not think there's a way of having to avoid looping through the records of the Inserted table in one way or another (ok, maybe there is, but I'd like to be able to read the code too :p), so I'm not gonna bore you with that one (unless you like to think otherwise). So pretty much, is there a better way of doing this, and if so, how?

EDIT: Well after request, here's the sproc

CREATE PROCEDURE ProcStatInsertTrigger 
    @Date DATE,
    @CampaignId BIGINT,
    @CampaignName varchar(500),
    @AdvertiserId BIGINT,
    @PublisherId BIGINT,
    @Unique BIT,
    @Approved BIT,
    @PublisherEarning money,
    @AdvertiserCost money,
    @Type smallint
AS
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;
IF @Approved = 1
     BEGIN
      DECLARE @test bit

      SELECT @test = 1 FROM CachedStats WHERE [Date] = @Date AND CampaignId = @CampaignId AND CustomerId = @PublisherId

      IF @test IS NULL
       BEGIN
        INSERT INTO CachedStats ([Date], CustomerId, CampaignId, CampaignName) VALUES (@Date, @PublisherId, @CampaignId, @CampaignName)
       END

      SELECT @test = NULL

        DECLARE @Clicks int
        DECLARE @TotalAdvertiserCost money
        DECLARE @TotalPublisherEarning money
        DECLARE @PublisherCPC money
        DECLARE @AdvertiserCPC money

        SELECT @Clicks = Clicks, @TotalAdvertiserCost = AdvertiserCost + @AdvertiserCost, @TotalPublisherEarning = PublisherEarning + @PublisherEarning FROM CachedStats
        WHERE [Date] = @Date AND CustomerId = @PublisherId AND CampaignId = @CampaignId

        IF @Type = 0 -- If click add one to the calculation
         BEGIN
          SELECT @Clicks = @Clicks + 1
         END

        IF @Clicks > 0
         BEGIN
          SELECT @PublisherCPC = @TotalPublisherEarning / @Clicks, @AdvertiserCPC = @TotalAdvertiserCost / @Clicks
         END
        ELSE
         BEGIN
          SELECT @PublisherCPC = 0, @AdvertiserCPC = 0
         END
      IF @Type = 0
       BEGIN

        UPDATE CachedStats SET
         Clicks = @Clicks,
         UniqueClicks = UniqueClicks + @Unique,
         PublisherEarning = @TotalPublisherEarning,
         AdvertiserCost = @TotalAdvertiserCost,
         PublisherCPC = @PublisherCPC,
         AdvertiserCPC = @AdvertiserCPC
        WHERE [Date] = @Date AND CustomerId = @PublisherId AND CampaignId = @CampaignId
       END
      ELSE IF @Type = 1 OR  @Type = 4 -- lead or coreg
       BEGIN
        UPDATE CachedStats SET
         Leads = Leads + 1,
         PublisherEarning = @TotalPublisherEarning,
         AdvertiserCost = @TotalAdvertiserCost,
         AdvertiserCPC = @AdvertiserCPC,
         PublisherCPC = @AdvertiserCPC
        WHERE [Date] = @Date AND CustomerId = @PublisherId AND CampaignId = @CampaignId
       END
      ELSE IF @Type = 3 -- Isale
       BEGIN
        UPDATE CachedStats SET
         Leads = Leads + 1,
         PublisherEarning = @TotalPublisherEarning,
         AdvertiserCost = @TotalAdvertiserCost,
         AdvertiserCPC = @AdvertiserCPC,
         PublisherCPC = @AdvertiserCPC,
         AdvertiserOrderValue = @AdvertiserCost,
         PublisherOrderValue = @PublisherEarning 
        WHERE [Date] = @Date AND CustomerId = @PublisherId AND CampaignId = @CampaignId     
       END
        ELSE IF @Type = 2 -- View
       BEGIN
        UPDATE CachedStats SET
         [Views] = [Views] + 1,
         UniqueViews = UniqueViews + @Unique,
         PublisherEarning = @TotalPublisherEarning,
         AdvertiserCost = @TotalAdvertiserCost,
         PublisherCPC = @PublisherCPC,
         AdvertiserCPC = @AdvertiserCPC
        WHERE [Date] = @Date AND CustomerId = @PublisherId AND CampaignId = @CampaignId   
       END
     END
END

After help, here's my final result, posted in case others have a similiar issue

CREATE TRIGGER [dbo].[TR_STAT_INSERT]
   ON  [iqdev].[dbo].[Stat]
   AFTER INSERT
AS 
BEGIN

    SET NOCOUNT ON

    -- insert all missing "CachedStats" rows
    INSERT INTO
        CachedStats ([Date], AdvertId, CustomerId, CampaignId, CampaignName) 
    SELECT DISTINCT
        CONVERT(Date, i.[Date]), i.AdvertId, i.[PublisherCustomerId], c.Id, c.Name
    FROM
        Inserted i
        INNER JOIN Advert AS   a ON a.Id = i.AdvertId
        INNER JOIN Campaign AS c ON c.Id = a.CampaignId
    WHERE
        i.[Approved] = 1
        AND NOT EXISTS (
                SELECT 1 
                FROM CachedStats as t
                WHERE 
                        [Date] = CONVERT(Date, i.[Date])
                        AND CampaignId = c.Id 
                        AND CustomerId = i.[PublisherCustomerId]
                        AND t.AdvertId = i.AdvertId
        )

  -- update all affected records at once
    UPDATE 
        CachedStats
    SET
        Clicks = 
      Clicks + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 0
      ),
     UniqueClicks = 
      UniqueClicks + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.[Unique] = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 0
      ),
        [Views] = 
      [Views] + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 2
      ),
     UniqueViews = 
      UniqueViews + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.[Unique] = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 2
      ),
     Leads = 
      Leads + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.[Unique] = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] IN (1,3,4)
      ),
     PublisherEarning =
      CachedStats.PublisherEarning + ISNULL((
       SELECT SUM(PublisherEarning) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId   

      ), 0),
     AdvertiserCost =
      CachedStats.AdvertiserCost + ISNULL((
       SELECT SUM(AdvertiserCost) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
      ), 0),
     PublisherOrderValue =
      PublisherOrderValue + ISNULL((
       SELECT SUM(PublisherEarning) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 3    
      ), 0),
     AdvertiserOrderValue =
      AdvertiserOrderValue + ISNULL((
       SELECT SUM(AdvertiserCost) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId   
       AND   s.[Type] = 3
      ), 0),
     PublisherCPC = 
      CASE WHEN (Clicks + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 0
      )) > 0 THEN
       (CachedStats.PublisherEarning + ISNULL((
       SELECT SUM(PublisherEarning) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId   
      ), 0)) -- COST ^
       / (
        Clicks + (
         SELECT COUNT(*) FROM Inserted s
         WHERE s.Approved = 1
         AND   s.PublisherCustomerId = i.PublisherCustomerId
         AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
         AND   s.AdvertId = i.AdvertId
         AND   s.[Type] = 0
        )    
       ) --- Clicks ^
      ELSE
       0
      END, 
     AdvertiserCPC = 
      CASE WHEN (Clicks + (
       SELECT COUNT(*) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId
       AND   s.[Type] = 0
      )) > 0 THEN
       (CachedStats.AdvertiserCost + ISNULL((
       SELECT SUM(AdvertiserCost) FROM Inserted s
       WHERE s.Approved = 1
       AND   s.PublisherCustomerId = i.PublisherCustomerId
       AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
       AND   s.AdvertId = i.AdvertId   
      ), 0)) -- COST ^
       / (
        Clicks + (
         SELECT COUNT(*) FROM Inserted s
         WHERE s.Approved = 1
         AND   s.PublisherCustomerId = i.PublisherCustomerId
         AND   CONVERT(Date, s.[Date]) = CONVERT(Date, i.[Date])
         AND   s.AdvertId = i.AdvertId
         AND   s.[Type] = 0
        )    
       ) --- Clicks ^
      ELSE
       0
      END  
   FROM
     Inserted i
    WHERE
     i.Approved = 1 AND
     CachedStats.Advertid = i.AdvertId AND
     CachedStats.[Date] = Convert(Date, i.[Date]) AND
     CachedStats.CustomerId = i.PublisherCustomerId
  SET NOCOUNT OFF
END

It looks slightly different now because I had to index it per advertisement too - but thanks alot for the help - sped everything up from 30hour+ to 30 sec to generate the CachedStats from my own development Stat table :)

A: 

First thing I would do is use a FAST_FORWARD cursor instead. As you are only going from one record to the next and not doing any updates this will be much better for performance.

DECLARE CURSOR syntax

Garry Shutler
Well that is a great start, thank you, I am trying out that approach now to see how much gain there is :)
kastermester
+1  A: 

Depending on what version of MSSQL you are running, you should also consider using Indexed Views for this as well. That could very well be a simpler approach than your triggers, depending on what the report query looks like. See here for more info.

Also, in your trigger, you should try to write your updates to the materialized results table as a set based operation, not a cursor. Writing a cursor based trigger could potentially just be moving your problem from the report query to your table inserts instead.

Scott Ivey
I would second the indexed views suggestion, and the one to use set-based operations in the cursor. It may be more complex SQL but it will be loads more efficient.
mwigdahl
A: 

You can slightly optimize your cursor variation by doing FAST_FORWARD, READ_ONLY and LOCAL options on the cursor. Also, you're pulling the Id into your cursor, and then looping back to get the values. Either use CURRENT_OF or throw them all into variables. But, I wouldn't expect these changes to buy you much.

You really need to move to a set based approach. That stored proc is definitely doable in a set based model - though it may take 3 or 4 different update statements. But even 3 or 4 different triggers (1 for views, 1 for clicks, etc.) would be better than the cursor approach.

Mark Brackett
A: 

Your best bet is to move to a set based operation in the trigger. I'm not going write this for you 100% but let me get you started, and we can see where we go from there. Keep in mind I am writting this without tables / schemas and so I'm not going validate. Expect Typos:-)

Let's look at your update statements first, From what I can tell you are updating the same table with the same where clause the only difference is the columns. You can consolidate this to look like:

UPDATE CachedStats SET
        /* Basically we are going to set the counts based on the type inline in the update clause*/

    Leads= CASE WHEN (@Type = 1 OR  @Type = 4 OR @Type=3 ) THEN Leads + 1 ELSE LEADS END,
        Clicks=CASE WHEN (@Type=0) THEN Clicks+1 ELSE Clicks END,
    Views=CASE WHEN (@Type=4) THEN Views+1 ELSE Views END,
        PublisherEarning = @PublisherEarning + PublisherEarning,
        AdvertiserCost = @AdvertiserCost +AdvertiserCost,
FROM CachedStats CS
INNER JOIN Inserted INS
    ON CS.Date=Inserted.Date AND CS.CustomerId=Ins.PublisherId AND CS.CampaignId=Ins.CampaignId

I do aggree with you that this could get ugly but that's a decision you'll have to make.

As for your insert clause, I would handle that the same way you already are just insert into the table from the Inserted table whatever doesn't already exist.

JoshBerke
There's no need to try and shoehorn it into a single UPDATE statement. Just run 3 or 4 UPDATE statements in the trigger. Performance certainly can't be any worse than cursor + single UPDATE statements - and is likely to be not much worse than a single CASEified UPDATE.
Mark Brackett
+7  A: 

The trick with these kinds of situations is to turn the sequential operation (for each record do xyz) into a set-based operation (an UPDATE statement).

I have analyzed your stored procedure and merged your separate UPDATE statements into a single one. This single statement can then be transformed into a version that can be applied to all inserted records at once, eliminating the need for a stored procedure and thereby the need for a cursor.

EDIT: Below is the code that we finally got working. Execution time for the whole operation went down from "virtually forever" (for the original solution) to something under one second, according to the OP's feedback. Overall code size also decreased quite noticeably.

CREATE TRIGGER [dbo].[TR_STAT_INSERT]
   ON  [iqdev].[dbo].[Stat]
   AFTER INSERT
AS 
BEGIN
  SET NOCOUNT ON

  -- insert all missing "CachedStats" rows
  INSERT INTO
    CachedStats ([Date], AdvertId, CustomerId, CampaignId, CampaignName) 
  SELECT DISTINCT
    CONVERT(Date, i.[Date]), i.AdvertId, i.PublisherCustomerId, c.Id, c.Name
  FROM
    Inserted i
    INNER JOIN Advert   a ON a.Id = i.AdvertId
    INNER JOIN Campaign c ON c.Id = a.CampaignId
  WHERE
    i.Approved = 1
    AND NOT EXISTS ( 
      SELECT 1 
      FROM   CachedStats
      WHERE  Advertid   = i.AdvertId AND
             CustomerId = i.PublisherCustomerId AND
             [Date]     = CONVERT(DATE, i.[Date])
    )

  -- update all affected records at once
  UPDATE 
    CachedStats
  SET
    Clicks               = Clicks               + i.AddedClicks,
    UniqueClicks         = UniqueClicks         + i.AddedUniqueClicks,
    [Views]              = [Views]              + i.AddedViews,
    UniqueViews          = UniqueViews          + i.AddedUniqueViews,
    Leads                = Leads                + i.AddedLeads,
    PublisherEarning     = PublisherEarning     + ISNULL(i.AddedPublisherEarning, 0),
    AdvertiserCost       = AdvertiserCost       + ISNULL(i.AddedAdvertiserCost, 0),
    PublisherOrderValue  = PublisherOrderValue  + ISNULL(i.AddedPublisherOrderValue, 0),
    AdvertiserOrderValue = AdvertiserOrderValue + ISNULL(i.AddedAdvertiserOrderValue, 0)
  FROM
    (
    SELECT
      AdvertId,
      CONVERT(DATE, [Date]) [Date],
      PublisherCustomerId,
      COUNT(*) NumRows,
      SUM(CASE WHEN Type IN (0)                      THEN 1 ELSE 0 END) AddedClicks,
      SUM(CASE WHEN Type IN (0)     AND [Unique] = 1 THEN 1 ELSE 0 END) AddedUniqueClicks,
      SUM(CASE WHEN Type IN (2)                      THEN 1 ELSE 0 END) AddedViews,
      SUM(CASE WHEN Type IN (2)     AND [Unique] = 1 THEN 1 ELSE 0 END) AddedUniqueViews,
      SUM(CASE WHEN Type IN (1,3,4) AND [Unique] = 1 THEN 1 ELSE 0 END) AddedLeads,
      SUM(PublisherEarning)                                      AddedPublisherEarning,
      SUM(AdvertiserCost)                                        AddedAdvertiserCost,
      SUM(CASE WHEN Type IN (3) THEN PublisherOrderValue  ELSE 0 END) AddedPublisherOrderValue,
      SUM(CASE WHEN Type IN (3) THEN AdvertiserOrderValue ELSE 0 END) AddedAdvertiserOrderValue
    FROM
      Inserted
    WHERE
      Approved = 1
    GROUP BY
      AdvertId,
      CONVERT(DATE, [Date]),
      PublisherCustomerId
    ) i 
    INNER JOIN CachedStats cs ON 
      cs.Advertid   = i.AdvertId AND
      cs.CustomerId = i.PublisherCustomerId AND
      cs.[Date]     = i.[Date]

  SET NOCOUNT OFF
END

The operations involving the CachedStats table will greatly benefit from one multiple-column index over (Advertid, CustomerId, [Date]) (as confirmed by the OP).

Tomalak
Ok seems like you managed to take what I thought would be a complete mess and turn it into something beautiful - I am testing this at the moment and will return with what i expect to be an accepted answer very soon :).
kastermester
I have tried this now, and I have made some moderations to it (like your code obviously couldn't know some column names would conflict and etc). A problem though is in your update command where it is assumed only one new thing of each will get updated. I am trying to sort this out atm though.
kastermester
I've got part of this working now and am more than expecting to get the rest done. I will need to make slight alterations to my table layout, but nothing that'll break your solution. I plan to post the final insert trigger once done with it so others can see what I did. Thank you for your help! :D
kastermester
I'm glad this helped you. Especially since it took quite some time to understand your code and write that post up. :-)
Tomalak
You are a genious! Down to 5 seconds without the index, and SSMS never manages to count to 1 with the index. Only thing that I wonder of is the NumRows you count up, it seems unused? Either way, thank you ALOT from 30 min to 0 seconds... that's whack ;)
kastermester
I thought I include NumRows in case you want to use it for something, statistically. It's easy to calculate, and it may carry some value for you. If it doesn't, then take it out and never mind. Please do double check all calculations for any errors. I also hope you learned a thing or two. ;-)
Tomalak
Oh, and please post the code you have working (if it deviates from mine). I doubt anyone else on the site has spent enough time on this question to understand it, but I would like to see it nevertheless.
Tomalak
Ok, I can remove it then. I did check the code for errors, but it DID actually work right out of the box (kudos for that, pretty impressive). The only change I have made is in the way you insert the missing rows. Which still looks the way I had it before only difference is the distinct, I believe.
kastermester
And yes, I have learned alot from this and have already had some similiar problems which have been solved by similiar approaches and it is really great to have something like this to refer back to when in doubt about stuff like this - as the performance is outstanding and amazing me!
kastermester
I have made a final edit that fixes the missing DISTINCT in the INSERT query. I also removed the check for CampaignId in the NOT EXISTS sub-query. It is redundant and may even prevent the use of the index (not sure).
Tomalak
looks right now, i did however forget, where you sum up the AddedPublisherOrderValue and AddedAdvertiserOrderValue, you sum on the wrong columns (needs to be PublisherEarning and AdvertiserCost respectively). I am sorry I forgot about this, but the change was so small i forgot all about it.
kastermester
Ah, well. Let's just say that I wanted to leave something to you. ;-)
Tomalak