Right way to do a insert

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 @.idMichael,
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
"Michael Fllgreen" <mfa@.ulle.se> wrote in message
news:OpostIYZGHA.1192@.TK2MSFTNGP04.phx.gbl...
> 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
>|||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
--
"Michael Fllgreen" <mfa@.ulle.se> wrote in message
news:OpostIYZGHA.1192@.TK2MSFTNGP04.phx.gbl...
> 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
>|||"Michael Fllgreen" <mfa@.ulle.se> wrote in message
news:OpostIYZGHA.1192@.TK2MSFTNGP04.phx.gbl...
> 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
>
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
--|||"David Portas" <REMOVE_BEFORE_REPLYING_dportas@.acm.org> wrote in message
news:eblLZSYZGHA.3652@.TK2MSFTNGP03.phx.gbl...
> INSERT INTO dbo.tbl (name) VALUES(@.name);
> SET @.ID = SCOPE_IDENTITY();
> SET @.r = @.@.ERROR ;
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
--|||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...
> "Michael Fllgreen" <mfa@.ulle.se> wrote in message
> news:OpostIYZGHA.1192@.TK2MSFTNGP04.phx.gbl...
> 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
> --
>|||"Adam Machanic" <amachanic@.hotmail._removetoemail_.com> wrote in message
news:e79chVYZGHA.4836@.TK2MSFTNGP05.phx.gbl...
> "David Portas" <REMOVE_BEFORE_REPLYING_dportas@.acm.org> wrote in message
> news:eblLZSYZGHA.3652@.TK2MSFTNGP03.phx.gbl...
> 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.
>
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|||Adam Machanic wrote:
> "David Portas" <REMOVE_BEFORE_REPLYING_dportas@.acm.org> wrote in message
> news:eblLZSYZGHA.3652@.TK2MSFTNGP03.phx.gbl...
> 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?
True. Good catch.

> 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 t
he
> app handle the problem, IMO.
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
--|||Michael F=E4llgreen wrote:
> 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
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
--|||"Adam Machanic" <amachanic@.hotmail._removetoemail_.com> wrote in message
news:%23yvmfQYZGHA.1200@.TK2MSFTNGP03.phx.gbl...
>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.
>
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.

> 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.
>
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.

> 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
> --
>
> "Michael Fllgreen" <mfa@.ulle.se> wrote in message
> news:OpostIYZGHA.1192@.TK2MSFTNGP04.phx.gbl...
>