Click here to monitor SSC
SQLServerCentral is supported by Red Gate Software Ltd.
 
Log in  ::  Register  ::  Not logged in
 
 
 
        
Home       Members    Calendar    Who's On


Add to briefcase

Cursor within triggers... Expand / Collapse
Author
Message
Posted Tuesday, September 18, 2012 5:58 AM


Mr or Mrs. 500

Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500

Group: General Forum Members
Last Login: Today @ 3:43 AM
Points: 515, Visits: 1,140
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...
Post #1360719
Posted Tuesday, September 18, 2012 6:14 AM


SSChampion

SSChampionSSChampionSSChampionSSChampionSSChampionSSChampionSSChampionSSChampionSSChampionSSChampion

Group: General Forum Members
Last Login: Monday, November 17, 2014 12:50 PM
Points: 13,872, Visits: 9,598
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
Post #1360734
Posted Tuesday, September 18, 2012 8:46 AM
SSCrazy

SSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazy

Group: General Forum Members
Last Login: Today @ 8:24 AM
Points: 2,268, Visits: 3,426
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)

Carl Sagan said: "There is no such thing as a dumb question." Sagan obviously never watched a congressional hearing!
Post #1360841
Posted Tuesday, September 18, 2012 5:38 PM


SSCommitted

SSCommittedSSCommittedSSCommittedSSCommittedSSCommittedSSCommittedSSCommittedSSCommitted

Group: General Forum Members
Last Login: Tuesday, November 18, 2014 12:29 PM
Points: 1,945, Visits: 3,121
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;


Books in Celko Series for Morgan-Kaufmann Publishing
Analytics and OLAP in SQL
Data and Databases: Concepts in Practice
Data, Measurements and Standards in SQL
SQL for Smarties
SQL Programming Style
SQL Puzzles and Answers
Thinking in Sets
Trees and Hierarchies in SQL
Post #1361063
Posted Tuesday, September 18, 2012 5:49 PM


SSC-Insane

SSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-InsaneSSC-Insane

Group: General Forum Members
Last Login: Today @ 9:36 AM
Points: 20,804, Visits: 32,736
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?



Lynn Pettis

For better assistance in answering your questions, click here
For tips to get better help with Performance Problems, click here
For Running Totals and its variations, click here or when working with partitioned tables
For more about Tally Tables, click here
For more about Cross Tabs and Pivots, click here and here
Managing Transaction Logs

SQL Musings from the Desert Fountain Valley SQL (My Mirror Blog)
Post #1361065
Posted Tuesday, September 18, 2012 7:26 PM
SSCrazy

SSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazySSCrazy

Group: General Forum Members
Last Login: 2 days ago @ 8:23 AM
Points: 2,873, Visits: 5,189
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!"
(So many miracle inventions provided by MS to us...)

How to post your question to get the best and quick help
Post #1361079
Posted Wednesday, September 19, 2012 4:36 AM


Mr or Mrs. 500

Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500

Group: General Forum Members
Last Login: Today @ 3:43 AM
Points: 515, Visits: 1,140
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...
Post #1361249
Posted Wednesday, September 19, 2012 5:44 AM


Mr or Mrs. 500

Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500Mr or Mrs. 500

Group: General Forum Members
Last Login: Today @ 3:43 AM
Points: 515, Visits: 1,140
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...
Post #1361272
« Prev Topic | Next Topic »

Add to briefcase

Permissions Expand / Collapse