Cursor within triggers...

  • Hi,

    I have a products table and a storage products table.

    CREATE TABLE Products (Product INT NOT NULL, Description NVARCHAR(50), TotalStock INT)

    CREATE TABLE ProdutStorages (Storage INT NOT NULL, Product INT NOT NULL, Stock INT)

    Also I have 2 stored procedures to update the stocks but only the one on ProdutStorages is used directly, since the TotalStock is based on the storages stock.

    CREATE PROCEDURE GCP_STK_UpdateProductStorage

    @storage INT,

    @product INT,

    @Stock INT

    AS

    BEGIN

    SET NOCOUNT ON

    IF EXISTS (SELECT TOP 1 1 FROM ProdutStorages WHERE Storage = @storage AND Product = @product)

    UPDATE ProdutStorages SET Stock = Stock + @Stock

    ELSE

    INSERT INTO ProdutStorages VALUES (@Storage, @product, @Stock)

    END

    GO

    CREATE PROCEDURE GCP_STK_UpdateProduct

    @Operation AS VARCHAR(1),

    @product AS INT,

    @CurrentStock AS INT

    AS

    BEGIN

    SET NOCOUNT ON

    IF @Operation = 'I'

    UPDATE Products SET TotalStock = TotalStock + @CurrentStock WHERE Product = @product

    IF @Operation = 'D'

    UPDATE Products SET TotalStock = TotalStock - @CurrentStock WHERE Product = @product

    END

    GO

    To update the product stock there's a trigger on the ProdutStorages.

    CREATE TRIGGER GCP_STK_ProdutStorage_INS ON ProdutStorages FOR INSERT

    AS

    SET NOCOUNT ON

    DECLARE @product AS INT

    DECLARE @CurrentStock AS INT

    DECLARE curRegistos CURSOR LOCAL FORWARD_ONLY STATIC READ_ONLY FOR SELECT Product, Stock FROM Inserted WHERE Stock <> 0

    OPEN curRegistos

    FETCH NEXT FROM curRegistos INTO @product, @CurrentStock

    WHILE @@FETCH_STATUS = 0

    BEGIN

    EXEC GCP_STK_UpdateProduct 'I', @product, @CurrentStock

    FETCH NEXT FROM curRegistos INTO @product, @CurrentStock

    END

    CLOSE curRegistos

    DEALLOCATE curRegistos

    GO

    As you can see there's a cursor so the stored procedure over Products table is called...

    This could be simply replaced by

    UPDATE Products SET TotalStock = TotalStock + Stock FROM Inserted WHERE Products.Product = Inserted.Products AND Stock <> 0

    but the GCP_STK_UpdateProduct is used on other situations and we decided to do some code reuse... And if the stocks business rules changes we only have to change this SP....

    but using a cursor inside a trigger to call and SP is it worth the code reuse?

    Isn't this a case where code "denormalization" would be very well applied?!

    Or can we call the SP as many times as there are inserted lines without the cursor? Say building a query dynamically and executing it?!

    Thanks,

    Pedro



    If you need to work better, try working less...

  • I'd dump the cursor, put set-based code in the trigger, and call it a day.

    Or, better yet, use a proc for all updates/inserts into the table, and have the proc handle all the logic internally, and get rid of the trigger completely.

    - Gus "GSquared", RSVP, OODA, MAP, NMVP, FAQ, SAT, SQL, DNA, RNA, UOI, IOU, AM, PM, AD, BC, BCE, USA, UN, CF, ROFL, LOL, ETC
    Property of The Thread

    "Nobody knows the age of the human race, but everyone agrees it's old enough to know better." - Anon

  • Definitely don't use a cursor.

    But you could pass all the values at once as a table-valued parameter; you just need to modify the code of the sp to handle a table-valued parameter.

    SQL DBA,SQL Server MVP(07, 08, 09) A socialist is someone who will give you the shirt off *someone else's* back.

  • CELKO (9/18/2012)


    I have a product_sku CHAR(10) s table and a storage product_sku CHAR(10) s table.

    No, you do not. A table has a key and you have none. What you have are two decks of punch cards written in SQL. You have no DRI or other constraints. And there is a serious design flaw called non-normal form redundancy (Google it).First let's clean that mess:

    CREATE TABLE Products

    (product_sku CHAR(10) NOT NULL PRIMARY KEY,

    product_description VARCHAR(50) NOT NULL);

    We would never store a total we can compute in the products table. You would have to fix this design flaw with triggers! We hate triggers and procedural code in SQL.

    CREATE TABLE Warehouses

    (product_sku CHAR(10) NOT NULL

    REFERENCES Products(product_sku)

    ON DELETE CASCADE

    ON DELETE UPDATE,

    warehouse_nbr INTEGER NOT NULL,

    PRIMARY KEY (product_sku, warehouse_nbr),

    stock_cnt INTEGER NOT NULL

    CHECK (stock_cnt >= 0));

    Now let's look at your procedures. You have not heard of the MERGE statement? It is also known as an UPSERT in some SQL products.

    CREATE PROCEDURE Upsert_Product

    (@in_warehouse_nbr INTEGER,

    @in_product_sku CHAR(10),

    @in_stock_cnt INTEGER)

    AS

    BEGIN

    SET NOCOUNT ON;

    MERGE INTO Warehouse AS Target

    USING (SELECT X.*

    FROM (VALUES (@in_warehouse_nbr, @in_product_sku, @in_stock_cnt))

    AS Source (warehouse_nbr, product_sku, stock_cnt))

    ON Target.warehouse_nbr = @in_warehouse_nbr

    AND Target.product_sku = @in_product_sku

    WHEN MATCHED

    THEN UPDATE

    SET stock_cnt = stock_cnt + @in_stock_cnt

    WHEN NOT MATCHED

    THEN INSERT

    VALUES (@in_warehouse_nbr, @in_product_sku, @in_stock_cnt);

    END;

    Your second procedure is not needed at all. The trigger is also a waste of code and time. SQL programmers put this kind of computation in a VIEW where it is always current and invoked only when needed.

    CREATE VIEW Stock_Levels (product_sku, product_description, stock_cnt_tot)

    AS

    SELECT P.product_sku, P.product_description, SUM(stock_cnt)

    FROM Products AS P

    LEFT OUTER JOIN

    Warehouses AS W

    ON P.product_sku = W.product_sku

    GROUP BY P.product_sku, P.product_description;

    Only problem Mr. Celko is that we can't even get past your CREATE TABLE statement, it has an error:

    CREATE TABLE Warehouses

    (product_sku CHAR(10) NOT NULL

    REFERENCES Products(product_sku)

    ON DELETE CASCADE

    ON DELETE UPDATE,

    warehouse_nbr INTEGER NOT NULL,

    PRIMARY KEY (product_sku, warehouse_nbr),

    stock_cnt INTEGER NOT NULL

    CHECK (stock_cnt >= 0));

    Msg 156, Level 15, State 1, Line 5

    Incorrect syntax near the keyword 'DELETE'.

    Maybe you should take the time to be sure the code you post is correct before you post it. Not very professional of you to post code that doesn't even work, and you call yourself a SQL Expert?

  • CELKO (9/18/2012)


    I have a product_sku CHAR(10) s table and a storage product_sku CHAR(10) s table.

    No, you do not. A table has a key and you have none. What you have are two decks of punch cards written in SQL. You have no DRI or other constraints...

    He has. At least he has what is called keyless table. Have you ever heard of it? Google it! Yes it is normally undesirable for a number of reasons, but it is still a table. You can create one and it will be there, just try! It doesn't look like two decks of punch cards to me.

    Did you ever consider the possibility that OP very often do post very simple representation of what they really have?

    ...

    We would never store a total we can compute in the products table. You would have to fix this design flaw with triggers!

    ...

    You are wrong here. We do store "totals" in the tables (and not only in Data warehouse and Reporting solutions, where it is very common). It all depends on requirement. Do you know them? How can you judge?

    ...

    We hate triggers and procedural code in SQL.

    ...

    May I ask who is "We"? Do your call yourself in a plural form? Actually, looks like you constantly do this...

    I don't hate triggers and procedural code. I do use triggers and procedural code in SQL where appropriate. Other SQL Experts I know, usually do the same - use any SQL Server "tool" where it is appropriate - they don't usually hate them.

    ...

    Your second procedure is not needed at all. The trigger is also a waste of code and time. SQL programmers put this kind of computation in a VIEW where it is always current and invoked only when needed.

    ...

    It all depends!

    Some time yes, SQL Developers put this sort of things into VIEW, sometimes we use COMPUTED column and some times we do persist it in a table.

    It all depends!

    Have you tried to execute your posted code, or "We" don't care? 😀

    _____________________________________________
    "The only true wisdom is in knowing you know nothing"
    "O skol'ko nam otkrytiy chudnyh prevnosit microsofta duh!":-D
    (So many miracle inventions provided by MS to us...)

    How to post your question to get the best and quick help[/url]

  • Eugene Elutin (9/18/2012)


    CELKO (9/18/2012)


    I have a product_sku CHAR(10) s table and a storage product_sku CHAR(10) s table.

    No, you do not. A table has a key and you have none. What you have are two decks of punch cards written in SQL. You have no DRI or other constraints...

    He has. At least he has what is called keyless table. Have you ever heard of it? Google it! Yes it is normally undesirable for a number of reasons, but it is still a table. You can create one and it will be there, just try! It doesn't look like two decks of punch cards to me.

    Did you ever consider the possibility that OP very often do post very simple representation of what they really have?

    That's exactly what I did... my products' table has over 100 fields and so does the storage one... It would be complete waste of time to post it all here for the question's purpose.

    Eugene Elutin (9/18/2012)


    CELKO (9/18/2012)


    ...

    We would never store a total we can compute in the products table. You would have to fix this design flaw with triggers!

    ...

    You are wrong here. We do store "totals" in the tables (and not only in Data warehouse and Reporting solutions, where it is very common). It all depends on requirement. Do you know them? How can you judge?

    If the product's storage table has over 1.000.000 rows, cause there are over 100 storage and 10.000 products (it's a kind of Walmart company, with many products and stores) I believe it's better do "denormalize" the table and put a total column on the product's table. There are few insert and updates but lot's of list so it's best this way...

    This is some code I just "inherited" and have to analyse and improve... that's why I posted these questions, I'd never build a trigger with cursors to call a another SP... unless it's the only way to do it.

    But in this case since the the 2nd SP is called from other SPs, not only by the trigger, it was design like this to do some code reuse, but I believe it's best not to do it...

    Thanks,

    Pedro



    If you need to work better, try working less...

  • PiMané (9/19/2012)


    If the product's storage table has over 1.000.000 rows, cause there are over 100 storage and 10.000 products (it's a kind of Walmart company, with many products and stores) I believe it's better do "denormalize" the table and put a total column on the product's table. There are few insert and updates but lot's of list so it's best this way...

    Just to give an example from the data numbers above a SELECT Product, Description, TotalStock FROM Products WHERE Product = 10 is twice as fast as the SELECT from the view and has less reads since it only uses one table...

    The difference is even greater when applying paging (BETWEEN 10 AND 30) where the table select is 5 times faster and even less reads..

    Thanks,

    Pedro



    If you need to work better, try working less...

Viewing 7 posts - 1 through 6 (of 6 total)

You must be logged in to reply to this topic. Login to reply