sql server programming: Right way to do a insert -- sql server programming

Re: Right way to do a insert

David Portas
4/21/2006 2:19:45 PM

sql server programming: [quoted text, click to view]

True. Good catch.

[quoted text, click to view]

I would handle the error with TRY CATCH in 2005. In 2000 I might leave
out error handling if the application dealt with it but most of the
time I log errors to a table or the log.

--
David Portas, SQL Server MVP

Whenever possible please post enough code to reproduce your problem.
Including CREATE TABLE and INSERT statements usually helps.
State what version of SQL Server you are using and specify the content
of any error messages.

SQL Server Books Online:
http://msdn2.microsoft.com/library/ms130214(en-US,SQL.90).aspx
--

Re: Right way to do a insert

David Portas
4/21/2006 2:23:29 PM

[quoted text, click to view]

If INSERTs fail intermittently for some reason can you be sure the
users will call support every time? If you catch and log the error
you'll know about it sooner and you can capture enough information to
make it easier to debug.

--
David Portas, SQL Server MVP

Whenever possible please post enough code to reproduce your problem.
Including CREATE TABLE and INSERT statements usually helps.
State what version of SQL Server you are using and specify the content
of any error messages.

SQL Server Books Online:
http://msdn2.microsoft.com/library/ms130214(en-US,SQL.90).aspx
--

Re: Right way to do a insert

David Browne
4/21/2006 4:07:20 PM

[quoted text, click to view]

I would only put error handling here in SQL 2000, and only for the case
where this procedure is called from another procedure. In SQL 2005 a
calling procedure can CATCH the error, and doesn't need a return value to
know if the execution failed.

Also I would omit the dbo. in the table reference and the procedure name
because there's no performance reason to have it there, and as a general
practice of naming, objects in the same namespace (schema) should refer to
each other with relative names.

CREATE PROCEDURE MyTable_Insert
@name VARCHAR(50),
@ID INT OUTPUT
AS
BEGIN
SET NOCOUNT ON

INSERT MyTable (name)
VALUES (@name)
IF @@ERROR <> 0 RETURN @@ERROR

SET @ID = SCOPE_IDENTITY()
END

David

Re: Right way to do a insert

Adam Machanic
4/21/2006 4:42:32 PM

A few comments here:

First of all, I'd tend to SET NOCOUNT ON at the start of the procedure,
before beginning a transaction -- but that might be more of a style thing.
In addition, there's really no reason to ever SET NOCOUNT OFF in a stored
procedure.

Second, you probably don't need an explicit transaction in this case. The
INSERT has its own implicit transaction, and you really only need an
explicit one if you're doing multiple DML operations at the same time (or,
if you need to serialize reads or do other things that are not too common in
most apps).

Third, you should avoid @@IDENTITY, assuming you're using SQL Server 2000 or
later. @@IDENTITY can return seemingly-erroneous results if another insert
happens due to a trigger firing. This can cause issues that can be very
tough to debug. Instead, use the SCOPE_IDENTITY function, which doesn't
have that problem.

Finally, I personally avoid using RETURN in stored procedures; instead, I
much prefer OUTPUT parameters. The return value is set by SQL Server when
exceptions occur, and I feel that overriding it is a form of in-band
signalling.

So given all of this, I would write your procedure as:

CREATE PROCEDURE dbo.spInsert
@name VARCHAR(50),
@ID INT OUTPUT
AS
BEGIN
SET NOCOUNT ON

INSERT tbl (name)
VALUES (@name)

SET @ID = SCOPE_IDENTITY()
END
GO

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

[quoted text, click to view]

Re: Right way to do a insert

Adam Machanic
4/21/2006 4:51:29 PM

[quoted text, click to view]

Shouldn't this be:

INSERT INTO dbo.tbl (name) VALUES(@name);
SELECT
@ID = SCOPE_IDENTITY(),
@r = @@ERROR ;

? Otherwise, won't @@ERROR reset thanks to the SCOPE_IDENTITY() line?

By the way, I personally would not put error handling in here, even in a
production app, since there's only one DML statement. Might as well let the
app handle the problem, IMO.

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

Re: Right way to do a insert

David Portas
4/21/2006 9:45:57 PM

[quoted text, click to view]

Looks OK but there are a few things you could change. The transaction is
unnecessary because there is only one DML statement involved, i.e. the
INSERT is the only thing that modifies data. NOCOUNT is scoped to the proc
so your NOCOUNT OFF does nothing useful. In SQL Server 2000 and 2005
SCOPE_IDENTITY() is usually preferred to @@IDENTITY because it won't be
affected by any triggers on the table. Finally, the RETURN status is
normally used to return success or error. Use OUTPUT parameters for other
values.

In a production application you should also add error handling. I've not
done that here because you didn't say what version you are using and that
matters a lot.

CREATE PROCEDURE dbo.spInsert(
@name VARCHAR(50),
@id INT OUTPUT)
AS
SET NOCOUNT ON ;
DECLARE @r INT ;
INSERT INTO dbo.tbl (name) VALUES(@name);
SET @ID = SCOPE_IDENTITY();
SET @r = @@ERROR ;
RETURN @r ;

GO

DECLARE @i INT, @r INT ;
EXEC @r = dbo.spInsert 'foo', @i OUTPUT ;

--
David Portas, SQL Server MVP

Whenever possible please post enough code to reproduce your problem.
Including CREATE TABLE and INSERT statements usually helps.
State what version of SQL Server you are using and specify the content
of any error messages.

SQL Server Books Online:
http://msdn2.microsoft.com/library/ms130214(en-US,SQL.90).aspx
--

Right way to do a insert

Michael Fällgreen
4/21/2006 10:28:39 PM

Just to make sure - is this the right way to do a simple insert?

Create PROCEDURE dbo.spInsert(
@name varchar(50))
AS
begin tran
set nocount on
insert into tbl (name) values(@name)
set nocount off
declare @id int
set @id = @@IDENTITY
commit tran
return @id

Re: Right way to do a insert

Dean
4/21/2006 10:44:49 PM

Michael,

There's no need for an explicit transaction if there's no more than a single
INSERT or UPDATE in a batch. But it wont do any harm either :)
More important thing: don't use @@IDENTITY function, but use
SCOPE_IDENTITY() instead, it's much more reliable (many will argue that you
shouldn't use the identity property at all..). Also, use an OUTPUT param to
return the identity value to calling application, and save RETURN for
success/fail code.
And, you should really check the @@ERROR value after each statement (or,
with 2005, use the TRY..CATCH). Check these excellent articles for a
detailed explanation of error handling inside T-SQL:

http://www.sommarskog.se/error-handling-I.html
http://www.sommarskog.se/error-handling-II.html

Dean

[quoted text, click to view]

Re: Right way to do a insert

Michael Fällgreen
4/21/2006 11:00:27 PM

Thanks alot all. BTW - im on a SQL Server 2005

Don't get the errorhandling thoug - i mean - I'm not going to do anything
about the error anyway. If the insert fails it fails with a standard
err.message. Why would i whanna catch an error in a simpel insert ?

Thanks

"David Portas" <REMOVE_BEFORE_REPLYING_dportas@acm.org> skrev i en
meddelelse news:eblLZSYZGHA.3652@TK2MSFTNGP03.phx.gbl...
[quoted text, click to view]

Re: Right way to do a insert

Brian Selzer
4/22/2006 12:00:00 AM

[quoted text, click to view]

That depends on what you're doing. ADO.NET makes use of the rows affected
to determine whether or not to throw a System.Data.DBConcurrencyException.
For procedures called from ADO.NET, execute SET NOCOUNT ON at the start of
the procedure, and then immediately before executing the statement that
writes to the database, SET NOCOUNT OFF.

[quoted text, click to view]

I don't think that a stored procedure's return value is set by the database
engine when exceptions or errors occur. Another mechanism is used to inform
the client because more than one error can occur within a procedure since
some don't kill the entire batch--instead, just the offending statement is
terminated.

I prefer to use return values to pass back status, and output parameters to
pass back other information, such as identity and rowversion.

[quoted text, click to view]

Re: Right way to do a insert

Adam Machanic
4/22/2006 3:37:15 PM

[quoted text, click to view]

Can you show some code to duplicate the problem? I haven't seen that
before.

[quoted text, click to view]

It is. Here's an example:

---
create proc errorProc
as
begin
select 1/0
end
go

declare @retval int

exec @retval = errorProc

select @retval
go

drop proc errorProc
go
---

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

Re: Right way to do a insert

Dan Guzman
4/22/2006 3:46:53 PM

[quoted text, click to view]

If I'm not mistaken, SET NOCOUNT OFF is needed for ADO.NET
DataAdapter.Update and also when RowsAffected is desired in both ADO and
ADO.NET. Other than that, SET NOCOUNT ON ought to be the setting for all
other stored procedure statements since that will suppress DONE_IN_PROC
messages that are sent to the client after each statement. Those messages
can cause problems ADO classic apps and reduce performance of procedures
that execute many statements.

[quoted text, click to view]

The engine will set the value as long as the RETURN statement is executed.
However, the client app might have trouble getting the return value due to
DONE_IN_PROC messages, exception messages and multiple result sets. The
behavior may also vary by API and provider. IMHO, the return value as well
as output parameter values should usually be discarded when an exception
occurs.

[quoted text, click to view]

Our development standard is to use the procedure return value only to
indicate success/failure, with zero indicating success. This is the basic
pattern used by most of the Microsoft-supplied system stored procedures. We
usually just set the return value to @@ERROR.

I think Erland did a good job of descibing SQL error handling issues in this
articles at http://www.sommarskog.se/

--
Hope this helps.

Dan Guzman
SQL Server MVP

[quoted text, click to view]

Re: Right way to do a insert

Brian Selzer
4/23/2006 3:24:09 PM

[quoted text, click to view]
It's not a problem. It's the way ADO.NET works. The Update method on the
System.Data.SqlClient.SqlDataAdapter relies on the rows affected to verify
whether the InsertCommand, UpdateCommand or DeleteCommand was successful,
and throws a DBConcurrencyException if there were no affected rows.
[quoted text, click to view]

Is the above behavior documented anywhere? If it is, I haven't been able to
find it. My return value was -6. Is that the same as yours? The return
code should be NULL since it was not set prior to executing the procedure.
I think that this may be a bug.

[quoted text, click to view]

Re: Right way to do a insert

Adam Machanic
4/23/2006 4:57:29 PM

[quoted text, click to view]

The return code will never be NULL -- that's documented in BOL, in the
topic for RETURN.

I first noticed the negative return codes for exceptions when working
with 7.0, and the behavior has not changed since then, so I'm pretty sure
it's not a bug. Whether or not it's documented, I do not know.

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

Re: Right way to do a insert

David Browne
4/23/2006 6:47:47 PM

[quoted text, click to view]

And it is not. If you SET NOCOUNT ON, you can use DataAdapters just fine.
As usual you should check @@rowcount after DML that you expect to always
affect one or more row.

Here's an example:

--------------Program.cs------------------------

using System;
using System.Data;
using System.Collections.Generic;
using System.Diagnostics;
using System.Data.SqlClient;

namespace csTest
{
public static class Program
{

public static void Exec( string cmd, SqlConnection con)
{
Console.WriteLine(cmd);
new SqlCommand(cmd, con).ExecuteNonQuery();
}

public static void Main()
{
SqlConnectionStringBuilder cb1 = new SqlConnectionStringBuilder();
cb1.DataSource = "(local)";
cb1.IntegratedSecurity = true;
cb1.InitialCatalog = "test";

try
{

using (SqlConnection con = new SqlConnection(cb1.ConnectionString))
{
con.Open();
con.FireInfoMessageEventOnUserErrors = true;
con.InfoMessage += new
SqlInfoMessageEventHandler(con_InfoMessage);

Exec(@"create table #foo(id int primary key, txt varchar(50))",
con);
Exec(@"insert into #foo(id,txt) values (1,'hello')", con);
Exec(@"
create procedure #foo_update @id int, @txt varchar(50)
as
begin
set nocount on
update #foo set txt = @txt where id = @id;
if @@rowcount = 0 raiserror('No Data Found for ID %d',16,1,@id)
end",con);

SqlDataAdapter da = new SqlDataAdapter(new SqlCommand("select *
from #foo", con));
da.UpdateCommand = new SqlCommand("#foo_update", con);
da.UpdateCommand.CommandType = CommandType.StoredProcedure;
da.UpdateCommand.Parameters.Add(new
SqlParameter("@txt",SqlDbType.VarChar,50,"txt"));
da.UpdateCommand.Parameters.Add(new SqlParameter("@id",
SqlDbType.Int, 0, "id"));

DataSet ds = new DataSet();
da.Fill(ds);
DataTable dt = ds.Tables[0];

dt.Rows[0]["txt"] = "goodbye";
da.Update(dt);

//requery the table
ds = new DataSet();
da.Fill(ds);
dt = ds.Tables[0];
Console.WriteLine(dt.Rows[0]["txt"]);

}
}
catch (Exception ex)
{
Console.WriteLine(ex);
}

Console.ReadLine();

}

static void con_InfoMessage(object sender, SqlInfoMessageEventArgs e)
{
Console.WriteLine(e.Message);
}
}

}

--------------End Program.cs------------------------

David

Re: Right way to do a insert

Adam Machanic
4/23/2006 9:55:39 PM

[quoted text, click to view]

Ahhh, the execution plan error strikes again :)

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

Re: Right way to do a insert

Adam Machanic
4/23/2006 9:57:48 PM

"David Browne" <davidbaxterbrowne no potted meat@hotmail.com> wrote in
message news:u6p$2BzZGHA.3740@TK2MSFTNGP03.phx.gbl...
[quoted text, click to view]

Ahh, thanks for showing code to verify! I was waiting until tomorrow to
check my source code at work, but I'm pretty sure we have SET NOCOUNT ON in
every single stored procedure in the system, and make heavy use of pretty
much every ADO.NET feature. I was getting nervous that I'd missed something
:-)

--
Adam Machanic
Pro SQL Server 2005, available now
http://www.apress.com/book/bookDisplay.html?bID=457
--

Re: Right way to do a insert

Erland Sommarskog
4/23/2006 10:30:30 PM

Adam Machanic (amachanic@hotmail._removetoemail_.com) writes:
[quoted text, click to view]

However, there may be no return value at all:

CREATE PROCEDURE deathtodeferrednameresolution AS
SELECT * FROM doesnotexist
go
DECLARE @ret int
SELECT @ret = 4711
EXEC @ret = deathtodeferrednameresolution
SELECT @ret -- Still 4711

--
Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se

Books Online for SQL Server 2005 at
http://www.microsoft.com/technet/prodtechnol/sql/2005/downloads/books.mspx
Books Online for SQL Server 2000 at

Re: Right way to do a insert

Erland Sommarskog
4/23/2006 10:41:53 PM

Brian Selzer (brian@selzer-software.com) writes:
[quoted text, click to view]

But is that really for any .UpdateCommand? All our stored procedures have
SET NOCOUNT ON in them. (Added by our load tool.) We're just about to
make our first functions in .Net, but it would be a great miss if we
can't use the .Update method because of this.

In any case, if SET NOCOUNT ON, there is 0 coming back, there is nothing
at all coming back, so the concurrency violation should not be raised
in this case.

--
Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se

Books Online for SQL Server 2005 at
http://www.microsoft.com/technet/prodtechnol/sql/2005/downloads/books.mspx
Books Online for SQL Server 2000 at

Re: Right way to do a insert

Brian Selzer
4/23/2006 11:37:35 PM

Whether or not the DBConcurrencyException is thrown is configurable. If you
set the ContinueUpdateOnError property of the DataAdapter to True, the
generated DBConcurrencyException is not thrown: instead, the text from its
Message property is assigned to the RowError property of the DataRow that
failed to update.

In addition, in your handler for the RowUpdated event, you can set the
Status property to Continue to prevent the exception from being thrown.
This may work better for you, provided your procedures indicate success with
a return value or output parameter.

As an aside, I don't always use the Update method to commit changes. For a
small number of rows, it works ok; but when there are many rows to be
changed, I prefer to dump them first to a temp table and then commit them
en mass using set-based operations. I find that response time is
better--especially when there are a lot of indexes.

[quoted text, click to view]

Re: Right way to do a insert

Brian Selzer
4/24/2006 7:27:14 PM

After looking at your code, I've found that it bombs out at the first
failure. This is a disaster waiting to happen. If the Update affected 400
rows and an error occured on row 151, then updates to 150 rows succeeded, 1
failed, and 349 haven't even been tried, but the program doesn't know how
many succeeded, only that one row failed. If an error occurs or is raised
due to a concurrency violation, then a System.Data.SqlClient.SqlException is
thrown and control transfers to the catch block, so the fill code doesn't
execute (which is a good thing because it would overwrite the 349 pending
changes). Now, you could possibly start a transaction before calling the
update statement and roll back in the catch block, but if you do that, you
can expect deadlocks. The code below works because only one row is
affected. System.Data.DataAdapter.Update is designed to commit all of the
changes in the disconnected DataSet or DataTable; it is not meant to be
called repeatedly (that is, once for each affected row). If you want to
issue single-row updates, then just use a parameterized SqlCommand. Don't
bother with the SqlDataAdapter. You wouldn't use a nail gun to hang a
picture, would you?

"David Browne" <davidbaxterbrowne no potted meat@hotmail.com> wrote in
message news:u6p$2BzZGHA.3740@TK2MSFTNGP03.phx.gbl...
[quoted text, click to view]

Re: Right way to do a insert

David Browne
4/24/2006 9:51:37 PM

[quoted text, click to view]

I was with you until there. Not using transactions for multiple seperate
DML statements comprising a logical unit of work is a problem for both
performance and correctness. This approach just treats concurrency
violations like any other runtime error. You have to be ready for a
constraint violation, string truncation or data type conversion error to
happen when you marshal bunch of .NET types (the DataTable) to SQL Server
types. In any of these cases you need to make sure that the client's work
does not end up partially commited.

I'm not saying that checking @@rowcount to catch concurrency violations is
the perfect solution: sending back just the rowcount for the update
statement is good too. It's better in that it preserves the optimistic
concurrency features of the DataSet, but worse in that any client calling
the procedure needs to be checking the row count returned to see if the
update suceeded.

[quoted text, click to view]

The DataAdapter.Update method just iterates the DataTable and issues a bunch
of single row DML statements. I don't follow.

David

Re: Right way to do a insert

Erland Sommarskog
4/24/2006 10:06:57 PM

David Browne (davidbaxterbrowne no potted meat@hotmail.com) writes:
[quoted text, click to view]

Thanks, David! That gave some relief.

--
Erland Sommarskog, SQL Server MVP, esquel@sommarskog.se

Books Online for SQL Server 2005 at
http://www.microsoft.com/technet/prodtechnol/sql/2005/downloads/books.mspx
Books Online for SQL Server 2000 at

Re: Right way to do a insert

Brian Selzer
4/25/2006 12:00:00 AM

"David Browne" <davidbaxterbrowne no potted meat@hotmail.com> wrote in
message news:eoESRNBaGHA.4292@TK2MSFTNGP02.phx.gbl...
[quoted text, click to view]

With respect to correctness, I agree. A logical unit of work should be
wrapped in a transaction. I disagree with the premise that marshalling .NET
types to SQL Server types can in any way cause constraint violations, string
truncation, or data type conversion errors. These should only occur if the
values to be marshalled are incorrect to begin with. With respect to
performance, I disagree. A series of updates may execute slower within a
transaction context than without. My personal preference is to push a
multiple-row logical unit of work to a temp table so that it can be
committed with set-based operations where the transaction context begins and
ends within the stored procedure. This allows a number of concurrency
improvement and deadlock avoidance mechanisms to be implemented such as
applying update locks or update range-locks before issuing the data
modification statements as well as reducing the number of physical writes
both to indexes and the transaction log.

[quoted text, click to view]

But that happens automatically as part of the DbDataAdapter. If it is
acceptable to partially commit the DataSet, then ContinueUpdateOnError
should be set to True so that after the Update, all of the rows that could
commit, did, and those that failed can be identified and dealt with by the
application by looking at the RowError property.

[quoted text, click to view]

Your code appears to be designed to commit a single row instead of all of
the rows in a DataTable.

[quoted text, click to view]

Re: Right way to do a insert

David Browne
4/25/2006 6:40:49 PM

[quoted text, click to view]

Yes but it's just sometimes difficult to discover the incorrectness of user
input wrt domain constraints that don't automatically marshal to .NET types,
like check constraints or string length limits. But that was just an
example of the kinds of runtime errors that can occur when flushing DataSet
changes back to the server.

[quoted text, click to view]

Only very rarely. Without a transaction the user must wait for a
transaction log flush after each statement. This is always a physical IO,
and puts an strict limit on your DML throughput.

[quoted text, click to view]

I agree that pushing data to a temp table and merging it using SQL is a good
solution. Especially since you can use the SQLBulkCopy object to load the
temp table in bulk.

[quoted text, click to view]

No, the example happened to use a single row, but would work the same for
any number of rows in the DataTable.

David

Re: Right way to do a insert

Brian Selzer
4/25/2006 9:34:29 PM

"David Browne" <davidbaxterbrowne no potted meat@hotmail.com> wrote in
message news:OK4WSHMaGHA.440@TK2MSFTNGP05.phx.gbl...
[quoted text, click to view]

I will reiterate my original observation: the code fails in the same way for
any number of rows greater than one. It bombs out at the first failure,
leaving the database in an indeterminate state. It cannot be determined
which rows that can commit will, nor will all of the rows that can't commit
be identified.

[quoted text, click to view]

Re: Right way to do a insert

David Browne
4/26/2006 12:00:00 AM

[quoted text, click to view]

....
[quoted text, click to view]

True, but you must code to account for runtime errors in updating DataSets
anyway. Concurrency problems aren't the only thing that can go wrong.

So you can use a transaction will prevent leaving the database in an
intermediate state, and use the DataAdapter's events to identify the
offending row.

David