SQL -- How to do this better



  • I have a stored procedure that has multiple cursors (:wtf: I know). I'm pretty sure this is causing errors.

    I'm trying to find a readable way of replacing them.

    We have our parent table - Person
    A person can have one or more Addresses, one or more phones, etc. A total of 9 tables can have one or more rows associated with person.
    Each different table can have a note associated with it and all notes are stored in a single table with a Type and the primary key of the entity it is a note for. (I hate this)

    Existing code does the following:

    Cursor for each address

    • create a guid
    • insert new address with guid that is a copy of the existing address (changing a state field)
    • get the primary key of new address using guid (yes, I know @@identity)
    • insert note with the foreign key of new address (that is copy of the existing note attached to the pre-existing address )

    If there were only one row of each this would be easy.

    The problem comes when two addresses exists with corresponding notes.
    Address 1 has Note 3
    Address 2 has Note 4

    I have to insert a copies of each
    Address 1a has Note 3a
    Address 2a has Note 4a

    My first drafted started with a bunch of temp tables and most can't even be CTEs to keep the ids of 1 and 1a together through multiple insert statements.

    I've searched a bit on using CTEs with OUTPUT but I can't get output with the original key from the CTE.

    Basically, if there is something that joins the CTE resultset with the OUTPUT resultset.

    ;with tmpAddress as (select blah, blah, newid() as addressuuid from Address where fk = 999999)
    insert into Address
     (blah, blah, uuid)
    select blah, blah, addressuuid 
    output inserted.addressid, tmpAddress.addressid --the reference to tmpAddress here is not allowed
    from tmpAddress
    

  • Impossible Mission Players - A

    @Karla
    I'm having trouble following what's going on through the (I assume) anonymization.

    You're trying to duplicate existing rows? Or you're trying to create a new address from the application's input and then copy notes from old address(es) to the new one?



  • Yeah, I knew I was somewhat confusing.

    Let's say we have single person Jill who has all of these child records of addresses, phone numbers, children, family members.

    When an event that changes her status (let's use marital status) we must preserve all her data as is at the time of the status change (in this case from single to married).

    But this information can still change after she's married. Therefore, all of the original data with her marital status of single is now readonly. To allow editing of it we copy it and insert new rows that are just a copy of the original rows with the new marital status.

    At the moment of this sproc completing there are duplicate rows off all data differing only in IDs, GUIDs, and marital status.

    I suspect since traffic has been higher, (apparently April is a rush month for changes to marital status :wink: ) I think the cursors are causing the sproc to fail and the user cannot change the marital status.

    I hope that explains better.


  • Impossible Mission Players - A

    @Karla
    Hm, ok. Is the stored procedure doing the copy of the Person row, or what data does it have access to via parameters? If the sproc isn't doing the copy, can you make any changes to the application?



  • @izzion said in SQL -- How to do this better:

    @Karla
    Hm, ok. Is the stored procedure doing the copy of the Person row, or what data does it have access to via parameters? If the sproc isn't doing the copy, can you make any changes to the application?

    There is only one Person row and it does not get copied. Nothing on the Person row is directly editable from the frontend (it only gets changed by marital status change events).

    PersonId and MaritalStatusId are the only parameters passed.

    Prior to a marital change event the original data is editable from the frontend. After a marital change event only the data connected to the current marital status is editable from the front end.


  • Impossible Mission Players - A

    @Karla
    Hrm.

    So:

    Person Table:
    PK_Field
    Person Data...
    
    Address Table:
    PK_Field (GUID)
    FK_PersonPK
    Address Data...
    
    Notes Table:
    Note_Type
    FK_NoteEntity (e.g. AddressPK)
    Note Data...
    

    Is Address.PK_Field actually the UNIQUEIDENTIFIER data type?



  • @Karla Would something like this do what you want?

    create table #tmpAddress (blah datetime, blah varchar(42), old_id uuid, new_id uuid)
    
    insert into #tmpAddress
    select blah, blah, addressuuid, newid()
    from address addr
    where addr.fk_id = param_conditions
    
    insert into address
    select blah, blah, new_id
    from #tmpAddress
    
    insert into notes (more, things, fk_id)
    select more, things, t.new_id
    from notes n
    inner join #tmpAddress t on n.fk_id = t.old_id
    
    drop table #tmpAddress
    


  • @izzion said in SQL -- How to do this better:

    @Karla
    Hrm.

    So:

    Person Table:
    PK_Field
    Person Data...
    
    Address Table:
    PK_Field (GUID)
    FK_PersonPK
    Address Data...
    
    Notes Table:
    Note_Type
    FK_NoteEntity (e.g. AddressPK)
    Note Data...
    

    Is Address.PK_Field actually the UNIQUEIDENTIFIER data type?

    No, we have the standard autonumber identity key. Remember this was done originally in like 2008.


  • Impossible Mission Players - A

    Where does the GUID come in to the schema then?

    Mostly I'm headed down the road @djls45 just suggested.



  • @djls45 said in SQL -- How to do this better:

    @Karla Would something like this do what you want?

    create table #tmpAddress (blah datetime, blah varchar(42), old_id uuid, new_id uuid)
    
    insert into #tmpAddress
    select blah, blah, addressuuid, newid()
    from address where fk = 999999
    
    insert into address
    select blah, blah, new_id
    from #tmpAddress
    
    insert into notes (more, things, fk_id)
    select more, things, t.new_id
    from notes n
    inner join #tmpAddress t on n.fk_id = t.old_id
    
    drop table #tmpAddress
    

    That's what I started with...it just felt like there had to be a better way.

    Functionally the first query would have to be:

    insert into #tmpAddress
    select blah, blah, addressuuid, newid(), also blah, blah --from notes
    from address left join notes address.pk = notes.fk and notestype = 1
    where fk = 999999
    


  • @izzion said in SQL -- How to do this better:

    Where does the GUID come in to the schema then?

    Mostly I'm headed down the road @djls45 just suggested.

    Because raisins and maybe whomever first developed it didn't know about @@identity



  • @Karla said in SQL -- How to do this better:

    @izzion said in SQL -- How to do this better:

    Where does the GUID come in to the schema then?

    Mostly I'm headed down the road @djls45 just suggested.

    Because raisins and maybe whomever first developed it didn't know about @@identity

    Also, I think older devs would often do transactions in the ColdFusion code before it could get identity.


  • Impossible Mission Players - A

    @Karla
    No, the first query doesn't have to touch the notes table.

    You would structure the first query to build the rows that are going to be put in the address table (plus importing the current FK value that the notes table references). Then you would do your inserts into the address table from the temp table, and also use the temp table to do the insert into the notes table.

    IF OBJECT_ID('tempdb.dbo.#addressRowsToCopy', 'U') IS NOT NULL
    DROP TABLE #addressRowsToCopy;
    
    CREATE TABLE #addressRowsToCopy
    (
        newAddressNotesFK UNIQUEIDENTIFIER
       ,oldAddressNotesFK UNIQUEIDENTIFIER
       ,blah
       ,blah 
    )
    
    INSERT INTO #addressRowsToCopy
    SELECT newid(), addressNotesFK, blah, blah
    FROM address
    WHERE personFK = @personBeingUpdated
    
    UPDATE address
    SET ActiveRecord = 0
    WHERE personFK = @personBeingUpdated
    
    INSERT INTO address
    SELECT newAddressNotesFK, blah, blah
    FROM #addressRowsToCopy
    
    UPDATE notes
    SET ActiveRecord = 0
    WHERE EXISTS (
        SELECT 1
        FROM notes AS n
        INNER JOIN #addressRowsToCopy AS addressCopy
        WHERE n.FK_NoteEntity = addressCopy.oldAddressNotesFK
    )
    
    INSERT INTO notes
    SELECT addressCopy.newAddressNotesFK, noteType, noteData
    FROM notes AS n
    INNER JOIN #addressRowsToCopy AS addressCopy
    ON n.FK_NoteEntity = addressCopy.oldAddressNotesFK
    
    DROP TABLE #addressRowsToCopy
    

    EDIT: Changed the code to account for inactivating old rows. Not 100% sure if the method of the notes is the most performant option, maybe someone else can give me an assist on that.



  • @Karla said in SQL -- How to do this better:

    @djls45 said in SQL -- How to do this better:

    @Karla Would something like this do what you want?

    create table #tmpAddress (blah datetime, blah varchar(42), old_id uuid, new_id uuid)
    
    insert into #tmpAddress
    select blah, blah, addressuuid, newid()
    from address where fk = 999999
    
    insert into address
    select blah, blah, new_id
    from #tmpAddress
    
    insert into notes (more, things, fk_id)
    select more, things, t.new_id
    from notes n
    inner join #tmpAddress t on n.fk_id = t.old_id
    
    drop table #tmpAddress
    

    That's what I started with...it just felt like there had to be a better way.

    Functionally the first query would have to be:

    insert into #tmpAddress
    select blah, blah, addressuuid, newid(), also blah, blah --from notes
    from address left join notes address.pk = notes.fk and notestype = 1
    where fk = 999999
    

    The difference here is that I've separated copying the notes from copying the address. The #tmp table could potentially even be stripped down to just the two uuids:

    insert into #tmp (old_id, new_id)
    select addressuuid, newid()
    from address
    where fk_person = PersonId
    

    And then use them to find and copy both the address and the notes (again, separately {I don't think it's even possible to insert into multiple tables at once, unless there's some funky trigger business going on}):

    insert into address (street1, street2, city, state, zip, fk_person, editable, addressuuid)
    select addr.street1, addr.street2, addr.city, addr.state, addr.zip, addr.fk_person, 0, t.new_id
    from address addr
    inner join #tmp t on t.old_id = addr.addressuuid
    
    insert into notes (note_text, editable, notestype, fk_id)
    select n.note_text, 0, n.notestype, t.new_id
    from notes n
    inner join #tmp t on t.old_id = n.fk and notestype = 1
    

    IMO, this is the best way. It's simple, which helps make it readable, which helps keep it maintainable. Trying to do too much in one step overcomplicates things and has more potential to introduce bugs. There may be differences in performance, but I doubt it would be very much, to the point of practical insignificance.



  • @Karla Have you considered MERGE? You can access the source row along with inserted and deleted in its output clause, unlike regular insert output clauses. It often comes in handy when you need to clone graphs.



  • @izzion said in SQL -- How to do this better:

    EDIT: Changed the code to account for inactivating old rows. Not 100% sure if the method of the notes is the most performant option, maybe someone else can give me an assist on that.

    Don't need to inactivate the old rows. Marital status is just anonymized...current status is consecutive and passed around in client variables.



  • @izzion said in SQL -- How to do this better:

    @Karla
    No, the first query doesn't have to touch the notes table.
    You would structure the first query to build the rows that are going to be put in the address table (plus importing the current FK value that the notes table references). Then you would do your inserts into the address table from the temp table, and also use the temp table to do the insert into the notes table.

    I need the specif

    @izzion said in SQL -- How to do this better:

    @Karla
    No, the first query doesn't have to touch the notes table.

    You would structure the first query to build the rows that are going to be put in the address table (plus importing the current FK value that the notes table references). Then you would do your inserts into the address table from the temp table, and also use the temp table to do the insert into the notes table.

    IF OBJECT_ID('tempdb.dbo.#addressRowsToCopy', 'U') IS NOT NULL
    DROP TABLE #addressRowsToCopy;
    
    CREATE TABLE #addressRowsToCopy
    (
        newAddressNotesFK UNIQUEIDENTIFIER
       ,oldAddressNotesFK UNIQUEIDENTIFIER
       ,blah
       ,blah 
    )
    
    INSERT INTO #addressRowsToCopy
    SELECT newid(), addressNotesFK, blah, blah
    FROM address
    WHERE personFK = @personBeingUpdated
    
    UPDATE address
    SET ActiveRecord = 0
    WHERE personFK = @personBeingUpdated
    
    INSERT INTO address
    SELECT newAddressNotesFK, blah, blah
    FROM #addressRowsToCopy
    
    UPDATE notes
    SET ActiveRecord = 0
    WHERE EXISTS (
        SELECT 1
        FROM notes AS n
        INNER JOIN #addressRowsToCopy AS addressCopy
        WHERE n.FK_NoteEntity = addressCopy.oldAddressNotesFK
    )
    
    INSERT INTO notes
    SELECT addressCopy.newAddressNotesFK, noteType, noteData
    FROM notes AS n
    INNER JOIN #addressRowsToCopy AS addressCopy
    ON n.FK_NoteEntity = addressCopy.oldAddressNotesFK
    
    DROP TABLE #addressRowsToCopy
    

    EDIT: Changed the code to account for inactivating old rows. Not 100% sure if the method of the notes is the most performant option, maybe someone else can give me an assist on that.

    Also I was trying not to do this 9 times for 9 related tables.



  • @Groaner said in SQL -- How to do this better:

    MERGE

    I think this may help simplify the code. I will update in the morning when I get a chance to try it.


  • Impossible Mission Players - A

    @Karla
    You have 9 different sets. So you have 9 different set based operations :shrug:

    You could potentially build your "to the notes!" temp table as one big temp table as you go, include a third field in @djls45's two column old/new UUID temp table that indicates which base table it's for. I think I would prefer separate intermediate temp tables for each base table from a readability / maintainability standpoint but I don't know which way would be more performant and by what order of magnitude.

    Either set based approach should be super more performant than cursors.



  • @izzion said in SQL -- How to do this better:

    @Karla
    You have 9 different sets. So you have 9 different set based operations :shrug:

    You could potentially build your "to the notes!" temp table as one big temp table as you go, include a third field in @djls45's two column old/new UUID temp table that indicates which base table it's for. I think I would prefer separate intermediate temp tables for each base table from a readability / maintainability standpoint but I don't know which way would be more performant and by what order of magnitude.

    Either set based approach should be super more performant than cursors.

    I know 9 temp tables better than 9 cursors...I just hate repeating the same shit 9 times and thought there had to be a better way.

    I ask when it seems like there should be an easier way.

    I taught our former tech lead about DatabaseGeneratedOption property because I figured setting the EF should know how to set CreatedDate and ModifiedDate without me manually coding. I asked him, he didn't know, and I was sure this had to be a solved problem.

    ETA: So that's what I was hoping here.


  • :belt_onion:

    @Karla could put the meat of some of that into an SP that only requires calling with the new id & table affected so you don't have to do the whole drop/insert/update/create every time with a slight change, just call the SP 9 times with diff parameters... but that would also be a crazy mess to manage. Being generic would make it more of a pain in the ass to decypher any problems too.
    But it would achieve your desire of not having to write 9 full sets of temp table commands.



  • @darkmatter said in SQL -- How to do this better:

    @Karla could put the meat of some of that into an SP that only requires calling with the new id & table affected so you don't have to do the whole drop/insert/update/create every time with a slight change, just call the SP 9 times with diff parameters... but that would also be a crazy mess to manage. Being generic would make it more of a pain in the ass to decypher any problems too.
    But it would achieve your desire of not having to run 9 full sets of temp table commands.

    And wouldn't that require dynamic SQL in the function? I try to avoid that...except in overnight processes where it handles dozens of tables.



  • @izzion @Karla
    You could add a third column that indicates the base table (address, phone, etc.), or you could clear the table between each set: delete from #tmp. That avoids having to filter by the third column, which slightly increases performance.
    Edit: And reduces the number of temp tables needed down to one.



  • @Groaner @Karla's use case sounds fairly simply and straightforward: copy (insert) rows. MERGE is for cases where rows may need a combination of inserts, updates, and/or deletes.



  • @djls45 said in SQL -- How to do this better:

    @izzion @Karla
    You could add a third column that indicates the base table (address, phone, etc.), or you could clear the table between each set: delete from #tmp. That avoids having to filter by the third column, which slightly increases performance.

    Ohh...let me sleep on that. I think that and @Groaner 's MERGE may make a better solution. Or at least that doesn't so tedious for me.



  • @djls45 said in SQL -- How to do this better:

    @Groaner @Karla's use case sounds fairly simply and straightforward: copy (insert) rows. MERGE is for cases where rows may need a combination of inserts, updates, and/or deletes.

    A quick search showed usefulness for insert only.

    The last example:



  • @Karla said in SQL -- How to do this better:

    @djls45 said in SQL -- How to do this better:

    @Groaner @Karla's use case sounds fairly simply and straightforward: copy (insert) rows. MERGE is for cases where rows may need a combination of inserts, updates, and/or deletes.

    A quick search showed usefulness for insert only.

    The last example:

    Exactly. MERGE gives you more access than just the inserted table for your output clauses, which makes using just the insert portion useful. Why we don't get the same capabilities with a regular INSERT is a great question for another day.



  • @Groaner said in SQL -- How to do this better:

    @Karla said in SQL -- How to do this better:

    @djls45 said in SQL -- How to do this better:

    @Groaner @Karla's use case sounds fairly simply and straightforward: copy (insert) rows. MERGE is for cases where rows may need a combination of inserts, updates, and/or deletes.

    A quick search showed usefulness for insert only.

    The last example:

    Exactly. MERGE gives you more access than just the inserted table for your output clauses, which makes using just the insert portion useful. Why we don't get the same capabilities with a regular INSERT is a great question for another day.

    I knew I should have tagged you in the OP.



  • @Karla said in SQL -- How to do this better:

    @djls45 said in SQL -- How to do this better:

    @Groaner @Karla's use case sounds fairly simply and straightforward: copy (insert) rows. MERGE is for cases where rows may need a combination of inserts, updates, and/or deletes.

    A quick search showed usefulness for insert only.

    The last example:

    I can't figure out a way to combine the address and notes inserts into one merge statement to do it all in one go without duplicating too many rows in address. The best I can see is to copy address and output the old and new uuids into #tmp, and then using a second insert to copy notes. It is one fewer statement (one merge and one insert instead of three inserts), but it's up to you how much it affects readability and maintainability:

    create #tmp (old_id uuid, new_id uuid)
    
    merge into address targt
    using (select blah, fk_personId, addressuuid from address
           where fk_personId = parameter_PersonId) as src
    on 1=2 --nothing matches so everything gets copied; filtering is done in the where clause above
    when not matched by target then insert (blah, fk_personId, addressuuid)
    	values (blah, fk_personId, newid())
    output src.addressuuid, inserted.addressuuid
    into #tmp;
    
    --Copy and repeat for phones, family_members, etc.
    --Each "merge ... output ... into #tmp" will add the needed old_uuid/new_uuid association for copying notes.
    
    insert into notes (things, notestype, fk)
    select things, notestype, new_id
    from notes n
    inner join #tmp t on t.old_id = n.fk
    
    drop table #tmp
    

    This assumes that the uuids are actually unique. If they aren't, first, why?! And second, the only differences would be to add the notestype filter to insert into notes..., copy it along with each merge... for each of the base tables, and include a delete from #tmp between each set.


  • Impossible Mission Players - A

    @Karla
    With the caveat that I still think the 9 separate temp tables is more readable & maintainable... You could potentially do it with a single CTE that's loaded with a series of UNION statements...

    WITH recordsToCopy (oldUUID, newUUID, recordTable) 
    AS
    (
    SELECT addressUUID, NEWID(), 'Address'
    FROM address
    WHERE FK_Person = @personToUpdate
    UNION
    SELECT fooUUID, NEWID(), 'Foo'
    FROM foo
    WHERE FK_Person = @personToUpdate
    UNION
    ...
    )
    
    INSERT INTO address (street1, street2, city, state, zip, fk_person, addressuuid)
    SELECT addr.street1, addr.street2, addr.city, addr.state, addr.zip, addr.fk_person, copy.newUUID
    FROM address addr
    INNER JOIN recordsToCopy copy 
    ON copy.oldUUID = addr.addressuuid
    
    INSERT INTO foo (bar, baz, fk_person, foouuid)
    SELECT foo.bar, foo.baz, foo.fk_person, copy.newUUID
    FROM foo
    INNER JOIN recordsToCopy copy 
    ON copy.oldUUID = foo.foouuid
    
    ...
    
    INSERT INTO notes (note_text, note_type, fk_id)
    SELECT n.note_text, n.note_type, copy.newUUID
    FROM notes n
    INNER JOIN recordsToCopy copy 
    ON copy.oldUUID = n.fk_id


  • OK...I ended up not doing anything fancy and kept it with temp tables.

    Though initially I was overdoing it because I was trying too hard to use CTEs.

    The overall result was cleaner than what I was doing before. Overall, I really didn't need the GUIDs (all tables have an autonumber PK). Joining on PKs with new status turned out more performant...no need to refactor if it works now.

    Also, a reason behind the :wtf: -ery of the cursors, they are a remnant from former boss's boss who forbid using temp tables.

    Thank you everyone for the input. I think talking it through helped me do it a little better than I would have initially and also not be bothered that I couldn't find a better way.

    There's no obvious difference in performance when doing it one off but the temp tables should be less likely to have concurrency issues than the multiple and nested cursors.


Log in to reply
 

Looks like your connection to What the Daily WTF? was lost, please wait while we try to reconnect.