locked
UPDATE then SELECT best practice RRS feed

  • Question

  • Hi experts, 

    I have a stored proc which evaluates a table of 'tasks' to return the task with the highest priority for a given user - at the same time, once that task has been found - I want to allocate that user to the task. 

    I have created a stored proc that works...however I'm sure this requirement /pattern is common & I would like some advice/recommendations on best practice for this pattern of update and select within a transaction.

    Here is some sample data:

    use tempdb;
    go
    
    if OBJECT_ID('Tasks', 'U') is not null
    drop table tasks;
    go
    
    
    create table tasks (
    	TaskId int identity  primary key,
    	TaskPriority tinyint not null,
    	UserAllocationPending char(10) null,
    	UserAllocation char(10) null,
    	TaskCreatedDate Datetime2 not null);
    
    
    insert into tasks 
    values 
    (1, 'user1', null, '20150101'),
    (1, null , null, '20150102'),
    (1, 'user1', null, '20150103'),
    (1, 'user2', null, '20150101'),
    (1, 'user1', null, '20150104'),
    (1, 'user1', null, '20150103');

    And here's what my current stored proc looks like;

    if OBJECT_ID('pGetNextTask', 'P') is not null
    drop proc pGetNextTask;
    go
    
    create proc pGetNextTask (
    	@UserID char(10),
    	@TaskID int output
    )
    as 
    begin
    
    begin try;
    
    begin transaction;
    
    	with GetTopTask as (
    		select top 1 case when UserAllocationPending = @UserID then 1
    			else 2
    			end as UserAllocatedPriority,
    		TaskId
    		from tasks
    		order by 
    		UserAllocatedPriority,
    		TaskPriority)
    	select @TaskID = TaskId
    	from GetTopTask;
    
    	update tasks 
    		set UserAllocation = @UserID
    	where TaskId = @TaskID;
    
    	commit transaction;
    
    end try
    begin catch;
    	if XACT_STATE() = -1 rollback transaction;
    
    	if XACT_STATE() = 1 commit transaction;
    
    
    end catch;
    end;
    
    go
    
    declare @MyTask int
    
    exec pGetNextTask @UserID = 'User1',   @TaskID = @MyTask output
    
    select @MyTask

    Appreciate any advice.

    Cheer, Clay




    Friday, May 15, 2015 10:07 AM

Answers

  • First, everything CELKO said is wrong. 

    >Procedures do not return values;

    Yes they do.

    >Identifiers are never numeric  

    Obviously false.

    >Your silly “P_Get_Next_Task” . . .

    Since stored procedures can fetch data, change data, etc, this is a reasonable convention.

    >But the worst error is that your task (entity) table all has the assignments . . .

    For a 1-Many relationship it's fine not to have a separate "relationship" table.  That's modeling 101.

    I would write it like this

    create proc pGetNextTask (
    	@UserID char(10),
    	@TaskID int output
    )
    as 
    begin
    begin transaction
    
    
        declare @task table(TaskId int);
    
        with TopTask as 
    	 (
    		select top 1
    		TaskId,
    		UserAllocation
    		from tasks with (updlock)
    		where UserAllocation is null
    		order by 
    		case when UserAllocationPending = @UserID then 1 else 2 end,
    		TaskPriority
    	 )
    	update TopTask set UserAllocation = @UserId
    	output inserted.TaskId into @task;
    
    	select @TaskID = TaskId from @task;
    
    commit transaction;
    end

    If you want more concurrent access, at the cost of not waiting for the exact best task you can use (updlock,rowlock,readpast), which will allow you to skip locked rows.

    David


    David http://blogs.msdn.com/b/dbrowne/

    • Proposed as answer by Kalman Toth Saturday, May 16, 2015 1:08 PM
    • Marked as answer by clay123123123 Sunday, May 17, 2015 1:28 PM
    Friday, May 15, 2015 3:47 PM

All replies

  • In this code block:

    begin transaction;
    
    	with GetTopTask as (
    		select top 1 case when UserAllocationPending = @UserID then 1
    			else 1
    			end as UserAllocatedPriority,
    		TaskId
    		from tasks
    		order by 
    		UserAllocatedPriority,
    		TaskPriority)
    	select @TaskID = TaskId
    	from GetTopTask;
    
    	update tasks 
    		set UserAllocation = @UserID
    	where TaskId = @TaskID;
    
    	commit transaction;

    The default isolation READ COMMITTED usually works best in most scenarios however in your case the shared locks on the rows touched by the select statement will be released as soon as the select is satisfied and it may be possible that others can also see the same row and may allocate the same Task ID (Rarity but possible)

    To enhance this further you may use UPDLOCK while selecting (And Locking the row by placing U lock) the row so no one else could update that same row till your transaction is committed:

    select top 1 case when UserAllocationPending = @UserID then 1
    			else 1
    			end as UserAllocatedPriority,
    		TaskId
    		from tasks WITH(UPDLOCK)
    		order by 
    		UserAllocatedPriority,
    		TaskPriority)
    	select @TaskID = TaskId
    	from GetTopTask ;
    HTH


    Good Luck!
    Please Mark This As Answer if it solved your issue.
    Please Vote This As Helpful if it helps to solve your issue

    Friday, May 15, 2015 10:46 AM
  • >> I have a stored proc which evaluates a table of 'tasks' to return the task with the highest priority for a given user - at the same time, once that task has been found - I want to allocate that user to the task. <<

    Procedures do not return values; functions return scalar values. Thisw is not RDBMS, but simple, basic programming. Thanks for posting DDL, but your design is fundamentally wrong. Your SQL is good, but you never had a data modeling or RDBMS course. 

    Identifiers are never numeric  – we do not do math with them. We never, never never use IDENTITY in RDBMS! Why do your think the count of physical insertions on one disk is part of a data model??  

    Your silly “P_Get_Next_Task” comes from the 1950-1960's software that had to have meta data in the names. Back in FORTRAN I integer variables started with I-thru N to signal the compiler. Today we follow ISO 11179 rules instead. This error is called a “tibble” if you want to know the terms.

    But the worst error is that your task (entity) table all has the assignments (relationship) in it! NO! In the first week of any Data Modeling course, you would learn that we never mix entities an relationships in one table. 

    85-95% of the work in SQL is done in DDL! 

    -- entity 
    CREATE TABLE Tasks 
    (task_id CHAR(5) NOT NULL PRIMARY KEY,
     task_priority TINYINT NOT NULL
     CHECK (task_priority >= 0) UNIQUE);

     I am making a guess that the tasks are well ordered, hence the UNIQUE constraint. If not, how do you settle ties? Your proprietary TOP n is random. 

    -- entity 
    CREATE TABLE Users
    (user_id CHAR(10) NOT NULL PRIMARY KEY,
     ..);

    -- relationship 
    CREATE TABLE Task_Allocations
    (task_id CHAR(5) NOT NULL 
     REFERENCES Tasks (task_id)
     ON UPDATE CASCADE
     ON DELETE CASCADE,
     user_id CHAR(10) NOT NULL
     REFERENCES Users(user_id)
     ON UPDATE CASCADE
     ON DELETE CASCADE,
     PRIMARY KEY (task_id, user_id),
     task_allocation_date DATE DEFAULT CURRENT_TIMESTAMP NOT NULL);

    WOW! Suddenly, your problem goes away. Try writing an insertion statement with this corrected schema. Here is a hint to get you started

    WITH X
    AS
    (SELECT task_id, task_priority, 
           MIN(task_priority) OVER() AS task_priority_min
      FROM Tasks)
     SELECT task_id
       FROM X
      WHERE task_priority = task_priority_min;

    Think of doing this in one statement.


    --CELKO-- Books in Celko Series for Morgan-Kaufmann Publishing: Analytics and OLAP in SQL / Data and Databases: Concepts in Practice Data / Measurements and Standards in SQL SQL for Smarties / SQL Programming Style / SQL Puzzles and Answers / Thinking in Sets / Trees and Hierarchies in SQL

    Friday, May 15, 2015 2:59 PM
  • First, everything CELKO said is wrong. 

    >Procedures do not return values;

    Yes they do.

    >Identifiers are never numeric  

    Obviously false.

    >Your silly “P_Get_Next_Task” . . .

    Since stored procedures can fetch data, change data, etc, this is a reasonable convention.

    >But the worst error is that your task (entity) table all has the assignments . . .

    For a 1-Many relationship it's fine not to have a separate "relationship" table.  That's modeling 101.

    I would write it like this

    create proc pGetNextTask (
    	@UserID char(10),
    	@TaskID int output
    )
    as 
    begin
    begin transaction
    
    
        declare @task table(TaskId int);
    
        with TopTask as 
    	 (
    		select top 1
    		TaskId,
    		UserAllocation
    		from tasks with (updlock)
    		where UserAllocation is null
    		order by 
    		case when UserAllocationPending = @UserID then 1 else 2 end,
    		TaskPriority
    	 )
    	update TopTask set UserAllocation = @UserId
    	output inserted.TaskId into @task;
    
    	select @TaskID = TaskId from @task;
    
    commit transaction;
    end

    If you want more concurrent access, at the cost of not waiting for the exact best task you can use (updlock,rowlock,readpast), which will allow you to skip locked rows.

    David


    David http://blogs.msdn.com/b/dbrowne/

    • Proposed as answer by Kalman Toth Saturday, May 16, 2015 1:08 PM
    • Marked as answer by clay123123123 Sunday, May 17, 2015 1:28 PM
    Friday, May 15, 2015 3:47 PM