2

I'm writing a database trigger on Table A. In said trigger, I insert a new record into Table B by doing:

  1. Look up the PK ID of Table B
  2. Increment the PK ID and insert new row into Table B

(Note: Table B does not use Identity. That is, the PK ID is not auto-incrementing. This is not something I can change. Reason being is the database schema is vendor-controlled. They don't use identity in any of their tables (auto-incrementing). Vendor keeps track of the next ID when it inserts records. That is how they designed their app. I want to be consistent with their design practices, as veering off course and adding identity to the table could break during a patch or upgrade cycle.)

I want to ensure two things during my transaction:

  1. Avoid deadlocks on Table B
  2. Prevent other insert queries/transactions from inserting into Table B (PK ID should be consistent)

Keep in mind, in my use case, the PK ID is not an integer but a varchar; it is an alphanumeric value with a 3-letter prefix followed by several digits.

Here is my code:

CREATE TRIGGER [dbo].[AfterINSERT_TableA_Trigger]
   ON  [dbo].[TableA]
   AFTER INSERT
AS 
BEGIN
    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    -- [Table A code goes here]

    -- Table B code

    -- Instruct SQL Server to rollback entire transaction and abort the batch when a run-time error occurs
    SET XACT_ABORT ON;

    BEGIN TRANSACTION TableB
    
    -- Ex: SUB1234567890
    DECLARE @subID VARCHAR(15)

    DECLARE @newSubID VARCHAR(15)

    -- Lookup last PK ID inserted into Table B
    SELECT
        @subID = submissionID
    FROM
        TableB WITH (TABLOCKX, HOLDLOCK)
    ORDER BY
        submissionID DESC

    -- Increment PK ID, separating prefix, increment by 1, and concatenate
   SELECT @newSubID = LEFT(@subID, 3) + CAST((RIGHT(@subID, LEN(@subID) - 3) + 1) AS VARCHAR(15))

   -- Insert new record into TableB
   INSERT TableB WITH (TABLOCKX, HOLDLOCK)
           (SubmissionID, Description, Status)
   VALUES
           (@newSubId, 'Test', 0)

   COMMIT TRANSACTION TableB

    -- Turn off rollback flag
    SET XACT_ABORT OFF;
END

I read that HOLDLOCK is equivalent to SERIALIZABLE (https://learn.microsoft.com/en-us/sql/t-sql/queries/hints-transact-sql-table?view=sql-server-ver15), so I'm not sure it makes a difference which one I use.

Does the code look OK such that it'll satisfy the criteria of avoiding deadlocks and blocking other insert transactions on Table B while my transaction is running? Do I need the WITH locks on both the SELECT and the INSERT statements? Should I be using Read Committed or similar at the transaction level instead of or in addition to?

To be clear, I don't want other insert transactions on the table to necessarily fail. I just want them to wait their turn until my transaction is complete, while keeping the PK ID column consistent.

3
  • I must admit, I'm not intimately familiar with sequences. The ID includes a 3-letter prefix. It's not a integer value. The other issue is the database schema is set by the vendor. While I'd have liked to use identity, that's not how they designed their app. I have to work within their parameters to keep things consistent in the system. I'm creating a trigger on one of their tables and I have to mimic how they insert records into their tables. if I can use sequence for an alphanumeric PK IDs and it won't break the vendor's schema and it satisfies the criteria, I can give it a go. Aug 3, 2021 at 21:25
  • 1
    How does the vendor handle their non-identity incrementing values in their code when they insert rows into the database? Is there a problem with their coding pattern that you need to solve with your code?
    – AMtwo
    Aug 3, 2021 at 23:24
  • I think they have a separate table they reference for the next id for all their tables in their database but they may just be grabbing the max ID. I have a somewhat minor customization I'm working on which is outside the scope of their app code. Too cumbersome to integrate with their app. It's a RAD app. Takes 10 steps to do one task what I can do in 1 or 2 steps with coding. I found sql trigger was the quickest to market for a solution for business stakeholders and easiest to maintain. But I don't want to color outside the lines too much (i.e. add identity to their tables). Aug 4, 2021 at 4:42

2 Answers 2

3

You should use a sequence for this, and get the next value with NEXT VALUE FOR or sys.sp_sequence_get_range to reserve a range of values for a multi-row insert.

Sequences only provide a numeric value, but you can easily prefix the character code yourself.

An alternative would be an identity column with a computed column incorporating the prefix, but you have said you do not wish to go that route.

The advantage of sequences and identity columns is they are maintained by the system without participating in the current user transaction. It can be extremely difficult to avoid blocking and deadlocking using alternative code.

If you really want to avoid all the built-in solutions for this type of problem, another robust alternative is a manual sequence as I describe in an old article of mine about Sequence Tables.

It sounds like the vendor is using that type of approach for their other tables, but I would be very surprised if they have implemented it correctly. I have personally never seen it done properly in vendor code. Perhaps yours is a first.

In any case, if you really want to follow their pattern, you should look into that more closely. That said, you should probably contact them about your requirement instead of making your own changes, depending on your licencing agreement with them.

3

You have a number of major flaws in your trigger:

  • It does not take into account there being multiple (or zero) rows in the inserted table.
  • XACT_ABORT is always ON in a trigger anyway, and I can't see a reason to try turn it off.
  • BEGIN TRAN \ COMMIT is also a waste of time because triggers run in the same transaction.
  • You need HOLDLOCK, UPDLOCK to keep the lock on the table until the end of the transaction, TABLOCK is not necessary.
  • I don't know the exact logic behind the numbering system, you may need to adjust it, but ROW_NUMBER would be necessary here, because you have multiple rows.
CREATE TRIGGER [dbo].[AfterINSERT_TableA_Trigger]
   ON  [dbo].[TableA]
   AFTER INSERT
AS 

    -- SET NOCOUNT ON added to prevent extra result sets from
    -- interfering with SELECT statements.
    SET NOCOUNT ON;

    IF (NOT EXISTS (SELECT 1 FROM inserted))
        RETURN;  -- early bail-out

    INSERT TableB
        (SubmissionID, Description, Status)
    SELECT
        CONCAT(
            LEFT(b.submissionID, 3),
            CAST(RIGHT(b.submissionID, LEN(b.submissionID) - 3) AS int) + ROW_NUMBER() OVER (ORDER BY (SELECT NULL)
        ),
        'Test',
        0
    FROM inserted i
    CROSS APPLY (
        SELECT TOP (1) submissionID
        FROM TableB WITH (HOLDLOCK, UPDLOCK)
        ORDER BY submissionID DESC
    ) b;

GO

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.