6

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 :)

Improve this question
8
  • Can you tell us what "ProcStatInsertTrigger" does? (BTW: You shouldn't name an sproc "Trigger", for rather obvious reasons). If it does not much more than inserting data into some table, then there is a way to greatly simplify you approach. Commented Mar 19, 2009 at 12:34
  • Thanks for posting your final version. :) However I'm not sure it is optimal - you do many seemingly redundant sub-selects that can be drawn and calculated from the join result, IMHO. Commented Mar 19, 2009 at 17:57
  • I would LOVE to get rid of those as well. However as I'm no SQL Guru I have no clue how to. If you could show me the way on that I'd love to optimize it further. Also - would it be better performance wise to make a check to see if there's multiple rows and if not just do it like before? Commented Mar 19, 2009 at 18:57
  • Oh and also - going from the FAST_FORWARD cursor method to this - the cursor method took about 17 minutes to do the INSERT part (as mentioned i have a DELETE part too, which pretty much is just - instead of + in above query) - this one: ~30 seconds. Commented Mar 19, 2009 at 18:58
  • @kastermester: If I find time tomorrow I look into it, maybe I can wrap my head around it. A preliminary look at your code suggests it won't be easy. Anyhow, the running time improvement already achieved sounds pretty impressive. Commented Mar 19, 2009 at 20:09

5 Answers 5

Reset to default
8

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).

Improve this answer
Sign up to request clarification or add additional context in comments.

12 Comments

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 :).
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.
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
I'm glad this helped you. Especially since it took quite some time to understand your code and write that post up. :-)
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 ;)
|
1

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.

Improve this answer

1 Comment

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.
0

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

Improve this answer

1 Comment

Well that is a great start, thank you, I am trying out that approach now to see how much gain there is :)
0

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.

Improve this answer

Comments

0

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.

Improve this answer

1 Comment

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.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.