Best Practice: Coding SQL Server triggers for multi-row operations

>> This is my first experience with triggers. So I used "INSTEAD OF" to check if the line sic to be inserted in the column is greater than 1000 or it adds error line sic in my error table, but I get an error this error: <<

And it seems to be your first experience with RDBMS and declarative programming , too. We insert sets in SQL, not lines (whatever that is). Because a column  is not a field or line, you can add a constraint to prevent prevent this, instead of trying to catch it later. Are you the carpenter who mops the floor rather than patching the roof? Think about this :)  

Msg 8101, Level 16, State 1, Procedure TR_CHECKMONEY, Line 9
An explicit value of the table's identity column 'CFCO_TRANSACTION' can be specified if the column list is used and IDENTITY_INSERT is ON.

>> Here is my table structure <<

Thank you! So many posters have no manners and expect us to read their minds whiel doing their homework. But you have serious, fundamental design errors.

We never use IDENTITY. It is a non-RDBMS left over from Sybase/UNIX that makes no sense. It is how the old file systems guys fake a physical record number instead of using a key and DRI. Mag tape lives! 

We do not use numeric types for identifiers and nominal scales. What math do you do on a phone number? In the US a phone number is 10 digits (NOT a ten place INTEGER). And what doe sit have to do with a transaction? Maybe the data comes over a phone line and that is why you use the word line in this posting?? 

We never, never use MONEY. It roudns too soon and gives the wrong answers! REALLY! Google it. 

We never, never use FLOAT for money. Floating point math has rounding problems that requrie special hardware and software to mitigate. And then it is illegal; read the EU and GAAP rules on financial reporting.

You never read the ISO-11179 rules for naming data elements or took a course on data modeling. The format is <attribute name>_<attribute property>, which means that you should have written transaction_id, etc. 

We do not store totals in a table; that was punch cards and tape files where we had to materialize them rather than keep them virtual like we do in SQL. 

I would have used an abbreviation for the transaction_status encoding, but you must have researched this and have some math you do on this SMALLINT, right??  That was sarcasm; we both know you have put no thought into the data types.  

Without any specs, my guess is that you needed something like this skeleton: 

CREATE TABLE CFCO_Transactions
(cost_amt DECIMAL(10,2) NOT NULL
  CHECK (cost_amt <= 1000.00),
 phone_nbr CHAR(10) NOT NULL, -- why is this here? 
 transaction_amt DECIMAL(10,2) NOT NULL,
  CHECK (<< relation between cost and transaction amt??>> ),
 transaction_status TINYINT DEFAULT 0 NOT NULL
  CHECK(transaction_status IN (.. )),
 transaction_date DATE DEFAULT CURRENT_TIMESTAMP NOT NULL ); 

See how constraints do most of the work in SQL? 

I know this is your first year with SQL, but your boss failed to get you some basic education. A TRIGGER is procedural code so non-SQL programmers can keep the mindset they had from mag tape files and VB, FORTRAN, et al.  in 35+ years I have written five of them and I know I could avoid at least 3 of them today. 

Switching your mindset from procedural to declarative programming is like trying to speak English with Japanese grammar :(

Sorry but you are not yet ready to work with triggers.  And I would argue that you have much to learn with respect to effective tsql coding. 

So, assuming that your trigger worked, what is the user experience in a situation where the logic jumps into your "error" situation.  When the ELSE logic in your trigger executes, it simply ends after the insert statement.  There is no effort made to notify the user (and more appropriately, to the application used to insert the rows) that the new rows have been ignored.  The application will simply assume that all the rows were successfully inserted.  Since most applications will attempt to refresh their content after a successful database update, the application will expect to find the rows there were inserted - which don't actually exists because of your trigger.  Then what? 

Next, lets look at your existing code.  You have 2 statements that set @COST_PAID - one right after the other.  Why?  Given that you are writing an instead-of trigger, you cannot refer to the actual table to "look up" anything about the inserted rows.  This information can only be found in the inserted virtual table.  This is an important distinction between instead-of triggers and after triggers.  Writing instead-of triggers is more difficult than after triggers - so the immediate question that comes to mind is why you chose an instead-of trigger rather than an after trigger.  You are not ready for either, but you chose the more difficult one.

Next, I can't say I understand your logic but I'll assume the IF logic is correct.  As I just said, your rows only exist in the virtual inserted table.  Therefore, your insert statement does not do what you want but it does generate an complete set of duplicate rows (as already pointed out). So you should be inserting by selecting from the inserted table.  And of course, we don't use "select *" for production-level coding except in an exists clause. You have discovered why this is so.

Lastly, you need to think much more about what information to record in your error table.  You need to make sure that the message you record is understandable to anyone who might need to read it.  Here, you start your message with "ERROR".  Well, this is going in an error table so it should be a standard assumption that everything in this table is an error.  "ERROR" is both redundant and wasteful.  OTOH, perhaps your table is misnamed and you use it for multiple purposes.  If so, you have a schema issue.  Message logging should be formally categorized if that is important - one should not rely on convention. And if your error table has multiple purposes, how is anyone supposed to make sense of the schema and what your "model" actually represents?  Next, your actual message is incorrectly worded.  You are ignoring rows if cost <= 1000.  Your text implies that cost has an upper limit of 1000.  Why did you not choose something like "Cost must be greater than 1000"?  Your error message doesn't include ANY useful information about the transaction that was just ignored.  Some of that information might be useful to include in your error message.  Without it, all anyone knows is that at least one row was ignored and completely lost.

And lastly, why did you choose to use a trigger when the simplest approach by far is to add a constraint to the table?  You should take another hard, long, perhaps uncomfortable look at why you have chosen this path.  As the others have suggestion, you should also reconsider your schema decisions.  Why?  Your table only supports whole numbers as monetary amounts, though cost is defined as float. That is highly unusual.

• Proposed as answer by Naomi NModerator 13 hours 42 minutes ago

Sorry but you are not yet ready to work with triggers.  And I would argue that you have much to learn with respect to effective tsql coding. 

So, assuming that your trigger worked, what is the user experience in a situation where the logic jumps into your "error" situation.  When the ELSE logic in your trigger executes, it simply ends after the insert statement.  There is no effort made to notify the user (and more appropriately, to the application used to insert the rows) that the new rows have been ignored.  The application will simply assume that all the rows were successfully inserted.  Since most applications will attempt to refresh their content after a successful database update, the application will expect to find the rows there were inserted - which don't actually exists because of your trigger.  Then what? 

Next, lets look at your existing code.  You have 2 statements that set @COST_PAID - one right after the other.  Why?  Given that you are writing an instead-of trigger, you cannot refer to the actual table to "look up" anything about the inserted rows.  This information can only be found in the inserted virtual table.  This is an important distinction between instead-of triggers and after triggers.  Writing instead-of triggers is more difficult than after triggers - so the immediate question that comes to mind is why you chose an instead-of trigger rather than an after trigger.  You are not ready for either, but you chose the more difficult one.

Next, I can't say I understand your logic but I'll assume the IF logic is correct.  As I just said, your rows only exist in the virtual inserted table.  Therefore, your insert statement does not do what you want but it does generate an complete set of duplicate rows (as already pointed out). So you should be inserting by selecting from the inserted table.  And of course, we don't use "select *" for production-level coding except in an exists clause. You have discovered why this is so.

Lastly, you need to think much more about what information to record in your error table.  You need to make sure that the message you record is understandable to anyone who might need to read it.  Here, you start your message with "ERROR".  Well, this is going in an error table so it should be a standard assumption that everything in this table is an error.  "ERROR" is both redundant and wasteful.  OTOH, perhaps your table is misnamed and you use it for multiple purposes.  If so, you have a schema issue.  Message logging should be formally categorized if that is important - one should not rely on convention. And if your error table has multiple purposes, how is anyone supposed to make sense of the schema and what your "model" actually represents?  Next, your actual message is incorrectly worded.  You are ignoring rows if cost <= 1000.  Your text implies that cost has an upper limit of 1000.  Why did you not choose something like "Cost must be greater than 1000"?  Your error message doesn't include ANY useful information about the transaction that was just ignored.  Some of that information might be useful to include in your error message.  Without it, all anyone knows is that at least one row was ignored and completely lost.

And lastly, why did you choose to use a trigger when the simplest approach by far is to add a constraint to the table?  You should take another hard, long, perhaps uncomfortable look at why you have chosen this path.  As the others have suggestion, you should also reconsider your schema decisions.  Why?  Your table only supports whole numbers as monetary amounts, though cost is defined as float. That is highly unusual.

• Proposed as answer by Naomi NModerator Friday, July 24, 2015 5:26 PM

Sorry but you are not yet ready to work with triggers.  And I would argue that you have much to learn with respect to effective tsql coding. 

So, assuming that your trigger worked, what is the user experience in a situation where the logic jumps into your "error" situation.  When the ELSE logic in your trigger executes, it simply ends after the insert statement.  There is no effort made to notify the user (and more appropriately, to the application used to insert the rows) that the new rows have been ignored.  The application will simply assume that all the rows were successfully inserted.  Since most applications will attempt to refresh their content after a successful database update, the application will expect to find the rows there were inserted - which don't actually exists because of your trigger.  Then what? 

Next, lets look at your existing code.  You have 2 statements that set @COST_PAID - one right after the other.  Why?  Given that you are writing an instead-of trigger, you cannot refer to the actual table to "look up" anything about the inserted rows.  This information can only be found in the inserted virtual table.  This is an important distinction between instead-of triggers and after triggers.  Writing instead-of triggers is more difficult than after triggers - so the immediate question that comes to mind is why you chose an instead-of trigger rather than an after trigger.  You are not ready for either, but you chose the more difficult one.

Next, I can't say I understand your logic but I'll assume the IF logic is correct.  As I just said, your rows only exist in the virtual inserted table.  Therefore, your insert statement does not do what you want but it does generate an complete set of duplicate rows (as already pointed out). So you should be inserting by selecting from the inserted table.  And of course, we don't use "select *" for production-level coding except in an exists clause. You have discovered why this is so.

Lastly, you need to think much more about what information to record in your error table.  You need to make sure that the message you record is understandable to anyone who might need to read it.  Here, you start your message with "ERROR".  Well, this is going in an error table so it should be a standard assumption that everything in this table is an error.  "ERROR" is both redundant and wasteful.  OTOH, perhaps your table is misnamed and you use it for multiple purposes.  If so, you have a schema issue.  Message logging should be formally categorized if that is important - one should not rely on convention. And if your error table has multiple purposes, how is anyone supposed to make sense of the schema and what your "model" actually represents?  Next, your actual message is incorrectly worded.  You are ignoring rows if cost <= 1000.  Your text implies that cost has an upper limit of 1000.  Why did you not choose something like "Cost must be greater than 1000"?  Your error message doesn't include ANY useful information about the transaction that was just ignored.  Some of that information might be useful to include in your error message.  Without it, all anyone knows is that at least one row was ignored and completely lost.

And lastly, why did you choose to use a trigger when the simplest approach by far is to add a constraint to the table?  You should take another hard, long, perhaps uncomfortable look at why you have chosen this path.  As the others have suggestion, you should also reconsider your schema decisions.  Why?  Your table only supports whole numbers as monetary amounts, though cost is defined as float. That is highly unusual.

• Proposed as answer by Naomi NModerator Friday, July 24, 2015 5:26 PM

I am still in doubt if the inclusion is accepted when cost is greater than 1000

     IF @COST_PAID>1000
     BEGIN
            INSERT INTO CFCO_TRANSACTION

or when cost is less than or equal to 1000

      ... VALUES ('ERROR THE COST SHOULD BE UP TO 1000')

Hello everybody,

Sorry for the delay, and thank you all for your help but I didn't used this trigger because my table tructure was changed, now i'm using Ozeki SMS gateway server to receive and send SMS through SQL server right now everything is good, but only when someone makes a payment of $ 10 while the product normally cost $ 25 I do not know the SMS send and tell the user that sent the amount is less.

This is the message model

ID. TRANSACT. 102830498374 you received $ 1 number 24381018799 your balance is $ 11

And this is the my table structure 

/****** Object:  Table [dbo].[ozekimessagein]    Script Date: 07/27/2015 22:54:15 ******/
SET ANSI_NULLS ON
GO
 
SET QUOTED_IDENTIFIER ON
GO
 
CREATE TABLE [dbo].[ozekimessagein](
	[ID] [int] IDENTITY(1,1) NOT NULL,
	[sender] [decimal](18, 0) NULL,
	[receiver] [decimal](18, 0) NULL,
	[msg] [nchar](300) NULL,
	[senttime] [time](7) NULL,
	[receivedtime] [time](7) NULL,
	[operator] [nchar](100) NULL,
	[msgtype] [nchar](100) NULL,
 CONSTRAINT [PK_ozekimessagein] PRIMARY KEY CLUSTERED 
(
	[ID] ASC
)WITH (PAD_INDEX  = OFF, STATISTICS_NORECOMPUTE  = OFF, IGNORE_DUP_KEY = OFF, ALLOW_ROW_LOCKS  = ON, ALLOW_PAGE_LOCKS  = ON) ON [PRIMARY]
) ON [PRIMARY]
 
GO

I would like if possible to create I trigger which will check the [msg] if the amount is less 10$ then insert this query on this table

INSERT INTO [ozekimessageout]
           ([receiver]
           ,[msg],[status])
     VALUES('receiver', 'Not received, amount is to less', 'send' )
GO

But is the amount is >10$

Do this :

INSERT INTO [ozekimessageout]           ([receiver]           ,[msg],[status])     VALUES('sender', 'Payment received', 'send' )GO

Thank you

In which table the "ID. TRANSACT..." message is stored:  ozekimessagein or ozekimessageout?

I couldn't fully understand what you are requesting.

But try

-- code #1 v2
CREATE TRIGGER TR_CHECKMONEY on [dbo].[ozekimessagein]
after Insert as

begin
declare @rc int;
set @rc= (SELECT Count(*) from (SELECT top (2) * from Inserted) as I);
IF @rc = 0 
  return;

with Inserted_2 as (
SELECT *, Balance= dbo.GetBalance (msg)
  from Inserted
  where Substring(msg, 1, 13) = 'ID. TRANSACT.'
)
INSERT into [ozekimessageout] ([receiver], [msg], [status]) 
  SELECT case when Balance < 10 then 'receiver' else 'sender' end,
         case when Balance < 10 then 'Not received, amount is to less' else 'Payment received' end,
         'send'
    from Inserted_2
    where Balance is not NULL;
end;
go

and

-- code #2
CREATE FUNCTION dbo.GetBalance (@Msg nvarchar(300))
returns int as
begin
IF @Msg is NULL or Len (@Msg) = 0
  return NULL;

declare @MsgR nvarchar(300), @I int;
set @MsgR= Ltrim (Reverse (@Msg));
set @I= PatIndex ('%[^0-9]%', @MsgR);
IF @I = 1
  return NULL;

set @MsgR= Reverse (Left (@MSgR, @I -1));

return Cast (@MsgR as int);
end;
go

You probably want to say: 

'Not received as amount is too small'

-----------------------

'Not received, amount is to less' is not a proper English.

Unless you want to use another language for messages and that message was just a rough translation.