SQL -- How to do this better

[Karla]

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 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

[izzion]

@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?

[Karla]

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.

[izzion]

@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?

[Karla]

@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.

[izzion]

@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?

[djls45]

@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

[Karla]

@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.

[izzion]

Where does the GUID come in to the schema then?

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

[Karla]

@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

[Karla]

@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]

@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.

[izzion]

@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.

[djls45]

@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.

[Groaner]

@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.

[Karla]

@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.

[Karla]

@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.

[Karla]

@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.

[izzion]

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

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.

[Karla]

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

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

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.

[darkmatter]

@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.

[Karla]

@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.

[djls45]

@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.

[djls45]

@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.

[Karla]

@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.

[Karla]

@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:

http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx

[Groaner]

@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:

http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx

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.

[Karla]

@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:

http://sqlblog.com/blogs/rob_farley/archive/2012/06/12/merge-gives-better-output-options.aspx

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.

[djls45]

@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:

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 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.

[izzion]

@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

[Karla]

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.