CODEWITHSUNDEEP
sqlsql-server
ryansin
ryansin

Reputation: 1817

Trigger doesn't fire for every update

I created a trigger to fire AFTER UPDATE of a table -- SOP_Head. This is supposed to monitor whenever a sales order has its status changed to "picked" so that a request for an advanced shipping notice to be created is generated at this stage.

CREATE TRIGGER OrderPickedCreateASN ON SOP_Head AFTER UPDATE
AS
BEGIN
    -- Check the new status of the order
    DECLARE @sopstatus  SMALLINT
    SELECT @sopstatus=SOPSTATUS FROM inserted

    -- Get the SOPNUMBE
    DECLARE @sopnumbe   CHAR(21)
    SELECT @sopnumbe=SOPNUMBE FROM inserted

    -- A SOPSTATUS of 4 means the order has been set to picked
    -- At this stage, create an ASN export request
    IF @sopstatus=4
    BEGIN
        -- Create the ASN export record
        EXEC AddOrderASNExportRequest @in_sopnumbe=@sopnumbe
    END
    -- If any other status, remove any existing requests
    ELSE
    BEGIN
        EXEC RemoveOrderASNExportRequest @in_sopnumbe=@sopnumbe
    END
END
GO

When I attempt to fire this trigger using the following update, it only fires for the first and last order numbers (ORD117196 AND ORD117199).

UPDATE SOP_Head SET SOPSTATUS=4 
WHERE SOPNUMBE IN ('ORD117196','ORD117197','ORD117198','ORD117199')

If I adjust the above statement to only include the two remaining orders for which an ASN export request is not generated (ORD117197 and ORD117198), they are created without issue.

Why is it that the trigger doesn't appear to be fired for all four updates of SOP_Head?

Upvotes: 0

Views: 60

Answers (2)

Vladislav
Vladislav

Reputation: 2992

I can think of a two general approaches to your problem.

One of them is actually the one that you have implemented - using a stored procedure. However, I would recommend firing off one execution of a stored procedure for bulk of records from SOP_Head. The power of SQL is when working on a set of records, not looping them one by one. This, of course, requires some redesigning of your SPs. A possible implementation could be the following:

--this UDTT could be defined in a more generic way for a good reuse
CREATE TYPE dbo.SopNrArray AS TABLE 
(
    SopNumber CHAR(21) PRIMARY KEY
)
GO

CREATE TRIGGER OrderPickedCreateASN ON SOP_Head AFTER UPDATE
AS
BEGIN
    DECLARE @sopstatusADD dbo.SopNrArray;
    INSERT @sopstatusADD
    SELECT DISTINCT SOPNUMBE FROM inserted WHERE SOPSTATUS=4;
    --consider removing DISTINCT if all SOPNUMBE would be unique

    DECLARE @sopstatusREM dbo.SopNrArray;
    INSERT @sopstatusREM
    SELECT DISTINCT SOPNUMBE FROM inserted WHERE SOPSTATUS!=4;
    --consider removing DISTINCT if all SOPNUMBE would be unique


    -- This check could eventually be removed if considered to be more optimal
    IF EXISTS (SELECT NULL FROM @sopstatusADD)
    BEGIN
        -- Create the ASN export record
        EXEC AddOrderASNExportRequest @sopstatusADD
    END

    -- This check could eventually be removed if considered to be more optimal
    IF EXISTS (SELECT NULL FROM @sopstatusREM)
    BEGIN
        EXEC RemoveOrderASNExportRequest @sopstatusREM
    END
END
GO

The other approach would be to implement some kind of FIFO queue using another dedicated table and then a frequently-enough scheduled SQL Agent Job that would process the records. This has the following main pros:

  • it is a lot extensible for future implementations

  • does not affect the performance of the main flow.

Upvotes: 1

Gordon Linoff
Gordon Linoff

Reputation: 1271241

Your code assumes that inserted has only one row. inserted is a view that can contain multiple rows.

This is simply wrong. You are going to need to loop through inserted to do one exec per update.

Probably the safest method is to use a cursor -- because you are using stored procedures in the trigger. That makes the code much more complicated than it would otherwise need to be.

If you have a unique id, I would probably do:

declare @i int;
declare @n int;

select @n = count(*), @i = 1 from inserted;

while @i <= n
begin
    with i as (
          select i.*, row_number() over (order by id) as seqnum
          from inserted
         )
    select @sopstatus = SOPSTATUS, @sopnumbe = SOPNUMBE
    from inserted
    where seqnum = @i;

    set @i := @i + 1;

    IF @sopstatus=4
    BEGIN
        -- Create the ASN export record
        EXEC AddOrderASNExportRequest @in_sopnumbe=@sopnumbe
    END
    -- If any other status, remove any existing requests
    ELSE
    BEGIN
        EXEC RemoveOrderASNExportRequest @in_sopnumbe=@sopnumbe
    END

end;  -- while

This is actually a bit more expensive than a cursor if may rows are inserted at the same time, but I find it easier to code.

Upvotes: 2

Related Questions