Reputation: 103
Do you think always having a transaction around the SQL statements in a stored procedure is a good practice? I'm just about to optimize this legacy application in my company, and one thing I found is that every stored procedure has BEGIN TRANSACTION
. Even a procedure with a single select or update statement has one. I thought it would be nice to have BEGIN TRANSACTION
if performing multiple actions, but not just one action. I may be wrong, which is why I need someone else to advise me. Thanks for your time, guys.
Upvotes: 6
Views: 2724
Reputation: 453426
I don't know of any benefit of not just using auto commit transactions for these statements.
Possible disadvantages of using explicit transactions everywhere might be that it just adds clutter to the code and so makes it less easy to see when an explicit transaction is being used to ensure correctness over multiple statements.
Additionally it increases the risk that a transaction is left open holding locks unless care is taken (e.g. with SET XACT_ABORT ON).
Also there is a minor performance implication as shown in @8kb's answer. This illustrates it another way using the visual studio profiler.
(Testing against an empty table)
CREATE TABLE T (X INT)
SET NOCOUNT ON
DECLARE @X INT
WHILE ( 1 = 1 )
BEGIN
BEGIN TRAN
SELECT @X = X
FROM T
COMMIT
END
SET NOCOUNT ON
DECLARE @X INT
WHILE ( 1 = 1 )
BEGIN
SELECT @X = X
FROM T
END
Both of them end up spending time in CMsqlXactImp::Begin
and CMsqlXactImp::Commit
but for the explicit transactions case it spends a significantly greater proportion of the execution time in these methods and hence less time doing useful work.
+--------------------------------+----------+----------+
| | Auto | Explicit |
+--------------------------------+----------+----------+
| CXStmtQuery::ErsqExecuteQuery | 35.16% | 25.06% |
| CXStmtQuery::XretSchemaChanged | 20.71% | 14.89% |
| CMsqlXactImp::Begin | 5.06% | 13% |
| CMsqlXactImp::Commit | 12.41% | 24.03% |
+--------------------------------+----------+----------+
Upvotes: 3
Reputation: 11406
You mentioned that you'll be optimizing this legacy app.
One of the first, and easiest, things you can do to improve performance is remove all the BEGIN TRAN and COMMIT TRAN for the stored procedures that only do SELECTs.
Here is a simple test to demonstrate:
/* Compare basic SELECT times with and without a transaction */
DECLARE @date DATETIME2
DECLARE @noTran INT
DECLARE @withTran INT
SET @noTran = 0
SET @withTran = 0
DECLARE @t TABLE (ColA INT)
INSERT @t VALUES (1)
DECLARE
@count INT,
@value INT
SET @count = 1
WHILE @count < 1000000
BEGIN
SET @date = GETDATE()
SELECT @value = ColA FROM @t WHERE ColA = 1
SET @noTran = @noTran + DATEDIFF(MICROSECOND, @date, GETDATE())
SET @date = GETDATE()
BEGIN TRAN
SELECT @value = ColA FROM @t WHERE ColA = 1
COMMIT TRAN
SET @withTran = @withTran + DATEDIFF(MICROSECOND, @date, GETDATE())
SET @count = @count + 1
END
SELECT
@noTran / 1000000. AS Seconds_NoTransaction,
@withTran / 1000000. AS Seconds_WithTransaction
/** Results **/
Seconds_NoTransaction Seconds_WithTransaction
--------------------------------------- ---------------------------------------
14.23600000 18.08300000
You can see there is a definite overhead associated with the transactions.
Note: this is assuming your these stored procedures are not using any special isolation levels or locking hints (for something like handling pessimistic concurrency). In that case, obvously you would want to keep them.
So to answer the question, I would only leave in the transactions where you are actually attempting to preserve the integrity of the data modifications in case of an error in the code, SQL Server, or the hardware.
Upvotes: 2
Reputation: 432331
One plus point is you can add another INSERT (for example) and it's already safe.
Then again, you also have the problem of nested transactions if a stored procedure calls another one. An inner rollback will cause error 266.
If every call is simple CRUD with no nesting then it's pointless: but if you nest or have multiple writes pre TXN then it's good to have a consistent template.
Upvotes: 2
Reputation: 8395
BEGIN TRANSACTION / COMMIT syntax shouldn't be used in every stored procedure by default unless you are trying to cover the following scenarios:
You include the WITH MARK option because you want to support restoring the database from a backup to a specific point in time.
You intend to port the code from SQL Server to another database platform like Oracle. Oracle does not commit transactions by default.
Upvotes: 1
Reputation: 4219
When performing multiple insert/update/delete, it is better to have a transaction to insure atomicity on operation and it insure that all the tasks of operation are executed or none.
For single insert/update/delete statement, it depends upon what kind of operation (from business layer perspective) you are performing and how important it is. If you are performing some calculation before single insert/update/delete, then better use transaction, may be some data changed after you retrieve data for insert/update/delete.
Upvotes: 2
Reputation: 2901
I can only say that placing a transaction block like this to every stored procedure might be a novice's work.
A transaction should be placed only in a block that has more than one insert/update statements, other than that, there is no need to place a transaction block in the stored procedure.
Upvotes: 1
Reputation: 131512
It is entirely unnecessary as each SQL statement executes atomically, ie. as if it were already running in its own transaction. In fact, opening unnecessary transactions can lead to increased locking, even deadlocks. Forgetting to match COMMITs with BEGINs can leave a transaction open for as long as the connection to the database is open and interfere with other transactions in the same connection.
Such coding almost certainly means that whoever wrote the code was not very experienced in database programming and is a sure smell that there may be other problems as well.
Upvotes: 5
Reputation: 46923
The only possible reason I could see for this is if you have the possibility of needing to roll-back the transaction for a reason other than a SQL failure.
However, if the code is literally
begin transaction
statement
commit
Then I see absolutely no reason to use an explicit transaction, and it's probably being done because it's always been done that way.
Upvotes: 4