2

Background

I recently had to fix a bug in an old WCF/Angular project. It is a To Do app that allows you to create tasks (A 'List' record) and mark it completed in 'ToDo' with some time calculations using a third table. Let's ignore everything wrong with it (and there's a lot) as I am certain nothing will be changed even if I made suggestions.

The Tables

The two tables look something like this:

  • List (ListId int, SiteUserId int, ListText varchar(max), Status int, CreatedBy int, CreatedDate datetime)
  • Todo (ToDoId int, [Date] date, Priority int, ListId int, Status int, ActualTime int, CalculatedTime int)

The Bug

The bug I was asked to fix is that when a user copies a ToDo Task (which is a combination of data from the three tables) to different days and makes a change in the text value, every instance of that item has their text value changed. The bug was fixed with a trivial change to the API (when a copy was being made, a new 'ToDo' item was created that pointed to the same 'List' item). However, I was also asked to fix the existing records so they do not show this behavior in the future.

My Attempt

I created a query that would create a new List item for each ToDo item that was a copy of a List item and then assign to those ToDo items the new ListId. I am fairly unfamiliar with SQL (and certainly know nothing about writing performant queries) and would like to know alternate strategies. In the end the three hours I spent reading documentation and writing the query were useless as it was decided the earlier records didn't matter. My code took about 9 minutes to execute for (if I recall correctly) about 6600 ToDo items.

My Question

I'm quite certain the primary reason the query is this slow is my use of a cursor. Is there anyway I could do this without looping over each record one at a time? I do not have a big knowledge base or good intuition for SQL and would enjoy any resources that help me get better.

My Code

-- omitted declarations
    SELECT *
    INTO #tempLists
    FROM List
    WHERE ListId IN (
            SELECT ListId
            FROM todo
            GROUP BY ListId
            HAVING count(ListId) >= 2
            );

    WITH cte
    AS (
        SELECT ToDoId,
            ROW_NUMBER() OVER (
                PARTITION BY ListId ORDER BY [Date]
                ) AS rn
        FROM ToDo
        WHERE ListId IN (
                SELECT ListId
                FROM #tempLists
                )
        )
    SELECT *
    INTO #tempToDos
    FROM cte
    WHERE rn > 1

    DECLARE db_cursor CURSOR
    FOR
    SELECT TodoId,
        ListId
    FROM ToDo
    WHERE TodoId IN (
            SELECT TodoId
            FROM #tempToDos
            )

    OPEN db_cursor

    FETCH NEXT
    FROM db_cursor
    INTO @TodoId,
        @ListId

    WHILE @@FETCH_STATUS = 0
    BEGIN
        INSERT INTO List (
            SiteUserId,
            ListText,
            [Status],
            CreatedBy,
            CreatedDate
            )
        SELECT SiteUserId,
            ListText,
            [Status],
            CreatedBy,
            CreatedDate
        FROM #tempLists
        WHERE ListId = @ListId

        UPDATE ToDo
        SET ListId = (
                SELECT SCOPE_IDENTITY()
                )
        WHERE TodoId = @TodoId

        PRINT @TodoId
        PRINT @ListId

        FETCH NEXT
        FROM db_cursor
        INTO @TodoId,
            @ListId
    END
-- omitted transaction and deallocation stuff
5
  • You could simplify your question by providing a minimal reproducible example and removing all the history/ discussion - which is irrelevant to the solution. Commented Sep 24, 2024 at 5:56
  • And for a one off fix, it doesn't matter how you solved it. Commented Sep 24, 2024 at 5:56
  • 1
    However it I in fact understand the underlying question, buried in all the detail, I think you are looking for the outout clause which allows you to capture all the new IDs and use them when creating child rows in a set-based solution rather than a RBAR. Commented Sep 24, 2024 at 5:57
  • 2
    I believe you can accomplish this as a set operation by using the OUTPUT clause on the first INSERT (or MERGE) to capture the generated identity values along with information that links it to the source data. Joining that captured data with the source table can feed the second insert. See this queation for a prior questions on this topic. Commented Sep 24, 2024 at 6:21
  • @DaleK I thought I had made a collapsable tab for the intro stuff, but it doesn't seemed to have worked. The fix is not going to be used anyway, I'd just like to know how this problem can be solved smarter. I will look into the output clause, thanks! I have only seen that in relation to parameters so far. Commented Sep 24, 2024 at 6:25

1 Answer 1

3

Using the advice from @Dale K and @TN I came up with the following to replace the cursor operations. It executed in 1 second.

DECLARE @insertedLists TABLE (
    ListId INT,
    ToDoId INT
    );

WITH cte
AS (
    SELECT tl.*,
        td.TodoId
    FROM #tempLists tl
    CROSS JOIN #tempToDos td
    WHERE tl.ListId = td.ListId
    )
MERGE List AS TGT
USING cte AS SRC
    ON 1 = 0
WHEN NOT MATCHED
    THEN
        INSERT (
            SiteUserId,
            ListText,
            [Status],
            CreatedBy,
            CreatedDate
            )
        VALUES (
            SRC.SiteUserId,
            SRC.ListText,
            0,
            SRC.CreatedBy,
            SRC.CreatedDate
            )
OUTPUT INSERTED.ListId,
    SRC.ToDoId
INTO @insertedLists;

UPDATE ToDo
SET ToDo.ListId = tl.ListId
FROM @insertedLists AS tl
WHERE ToDo.ToDoId = tl.ToDoId
Sign up to request clarification or add additional context in comments.

1 Comment

Nice! It makes a nice change when a question asker puts in the work to create the solution. +1 from me.

Your Answer

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

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.