SQL -- How to do this better
-
I have a stored procedure that has multiple cursors ( 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 4I have to insert a copies of each
Address 1a has Note 3a
Address 2a has Note 4aMy 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
-
@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 ) I think the cursors are causing the sproc to fail and the user cannot change the marital status.
I hope that explains 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?
-
@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.
-
@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.
-
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.
-
@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 twouuid
s: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 withinserted
anddeleted
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.
-
@Karla
You have 9 different sets. So you have 9 different set based operationsYou 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 operationsYou 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.
-
@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 ofinsert
s,update
s, and/ordelete
s.
-
@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 ofinsert
s,update
s, and/ordelete
s.A quick search showed usefulness for insert only.
The last example:
http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx
-
@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 ofinsert
s,update
s, and/ordelete
s.A quick search showed usefulness for insert only.
The last example:
http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx
Exactly.
MERGE
gives you more access than just theinserted
table for your output clauses, which makes using just the insert portion useful. Why we don't get the same capabilities with a regularINSERT
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 ofinsert
s,update
s, and/ordelete
s.A quick search showed usefulness for insert only.
The last example:
http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx
Exactly.
MERGE
gives you more access than just theinserted
table for your output clauses, which makes using just the insert portion useful. Why we don't get the same capabilities with a regularINSERT
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 ofinsert
s,update
s, and/ordelete
s.A quick search showed usefulness for insert only.
The last example:
http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx
I can't figure out a way to combine the address and notes
insert
s into onemerge
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
uuid
s are actually unique. If they aren't, first, why?! And second, the only differences would be to add thenotestype
filter toinsert into notes...
, copy it along with eachmerge...
for each of the base tables, and include adelete from #tmp
between each set.
-
@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 -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.