• Gerry (unregistered)

    There is absolutely nothing wrong with this code. I just tried it out and it is great! I will actually use this in the near future with one of my projects. Thanks!

  • Ben Hutchings (unregistered)

    Is 'select coalesce(max(' + @table_name + '_id) + 1, 1) from ' + @table_name too simple?

  • anonymouse (unregistered)

    I prefer 'select congeal(max(' + @table_name + '_id) + 1, 1) from ' + @table_name...note the difference.

  • cm (unregistered)

    Owww!! What happens if you need to have more than 2.14 billion records in a table?

  • To answer your question... (unregistered)

    Truncate your data. :-)

  • Andrei (unregistered)

    Surprised to not see a cursor

  • Phil Scott (unregistered)

    I kinda cringe when I see the coalesce function. At a glance, coalesce looks pretty close to a certain goat website...

  • anonymouse (unregistered)

    Phil: that's why I use the congeal function.

  • Alex B. (unregistered)

    Is Donald K. Burleson the new mascot of this site? I sure hope so!

    Phil Scott: Whatever it looks like from up-close, COALESCE is a great function in both its simplicity and power, wouldn't you agree?

  • Jeff S (unregistered)

    Even without the IN() part of the WHERE clause, in general stored procs like this are a bad idea due to concurrency issues. Two processes may receive the same "new ID" for the same table if they requist it at the same time .

    And, of course, the overall concept of randomly assigning primary keys to tables in general is a whole 'nother discussion.

  • anonymouse (unregistered)

    Who is Donald K. Burleson?

  • Not Donald K. Burleson (unregistered)

    I think any $500/hr consultant who pontificates the virtues of being a redneck on his company website deserves quite a many links from this site.

  • Alex B. (unregistered)

    anonymouse: If you have to ask, you don't deserve to make $500/hour for your vast Oracle knowledge.

  • anonymouse (unregistered)

    No one is worth $500/hr. Especially when you can farm the same work out to Singapore for minimum wage.

  • Donald K. Burleson Fan (unregistered)

    >> I think any $500/hr consultant
    >> who pontificates the virtues of
    >> being a redneck on his
    >> company website deserves
    >> quite a many links from this
    >> site.

    Yeehaw!

  • Jason (unregistered)

    But Mr. Burleson has over 20 years experience, and is much more productive than having 3 or 4 high-end DBA's from another respectable company ;)

  • Anon (unregistered)

    Jeff,

    <quote>
    Even without the IN() part of the WHERE clause, in general stored procs like this are a bad idea due to concurrency issues. Two processes may receive the same "new ID" for the same table if they requist it at the same time .
    </quote>

    Exactly...

    And in SQL Server, you have to ask - why do this rather than just use IDENTITY?

  • me (unregistered)

    How would this work at all? Two different calls could be made right after each other which would return the same id to both. Oracle sequences would be far better than this mess.

  • BradC (unregistered)

    If I'm reading this right, this stored procedure is functionally different than finding the largest ID and adding 1. It should return the smallest ID value not currently used in the table.

    This actually looks simpler than the "identity gap filling" Example B in the MSDN link you gave.

  • anonymouse (unregistered)

    Who cares about the code snippet, let's get back to the Donald K. Burleson discussion! Or maybe we can discuss Wilford Brimley?

  • carter (unregistered)

    burleson cracks me up every time...
    @_0

  • Cliff (unregistered)

    don't we use identities when we want a unique number? may as well take the highest and increment in almost every case (or insert the record and just use @@identity to return the new identity).

  • anonymous (unregistered)

    Or if you don't like @@Identity use SCOPE_IDENTITY():
    last identitiy generated in the current scope and session

    http://msdn.microsoft.com/library/default.asp?url=/library/en-us/tsqlref/ts_sa-ses_6n8p.asp

  • Spidey (unregistered)
  • Spidey (unregistered)

    Hang on - do you think he charged himself $500/hr for these reviews?

  • Ben Hutchings (unregistered)

    Jeff, Anon, me: Doesn't transactional isolation prevent that from happening? Or am I missing something? (Maybe the fact that a lot of people reduce isolation as a performance hack...)

  • Ben Hutchings (unregistered)

    Is the problem that there's no guarantee that the ID is actually used within the same transaction as it was generated?

  • Viagra (unregistered)

    Hey, do u know the pharmaceutical term for viagra? ......


    Mycoxafloppin :-D

  • Jeff M (unregistered)

    Spidey,

    Apparently, Mr. Burleson never met a book he didn't like.

  • Prakash (unregistered)

    You guys dont post anything realted to C/C++

  • George Prado (unregistered)

    #include "pascal.h"
    http://thedailywtf.com/archive/2004/10/27/2962.aspx

    that one I beleive is C/C++ one or the other, and if you want to say that they don't post on any language I would say that VFoxPro has not shown up yet...

    vive le C#
    french is the language of love,
    for everything else there's C#

  • Guayo (unregistered)

    I don’t get it!
    This sproc doesn’t give the next id! This sproc gies you the first unused id! Maybe this isn’t the best code to do that but the bashing of this code based on a wrong premise is a bigger WTF to me.
    BTW, here is an interesting article about finding such sequence gaps in SQL Server:
    http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnsqlpro04/html/sp04d8.asp

  • blanon (unregistered)

    The Daily WTF for Wednesday goes to all those Red states in middle America...

  • Bustaz Kool (unregistered)

    I'd like to enter the race for "spot the number of ... irregularities" in the code. We can debate a few or theoretical grounds but I hope we can agree on the b8ulk of them:

    1) Procedure declared without owner specified.
    2) Table name variable is Nullable. How we are expected to use a Null table name is unclear.
    3) "MIN('+@table_name+'_id)+1 IS NULL" - Why are we operating (arithmetic addition) on a potentially Null value and then asking if it's Null.
    4) Replace Case with coalesce. (Alex B. Coalesce is great; I agree)
    5) "FROM '+@table_name+'" - Again, no owner specified for the object.
    6) "and '+@table_name+'_id < (2147483647 - 1)" - Overflow is detected but NOT handled. Overflow results in a Null being retuned.
    7) "set @id =( select * from #temp)" - Where to start? Is the problem the fact that the SET uses a SELECT or is it that the SELECT returns * as the column to use? You decide.
    8) "DROP TABLE #temp" - Extraneous since the temp table is about to go out of scope.

    Systemic Issues: In no particular order

    9) Concurrency - What's to stop two threads from accessing the same value?
    10) pa_actab001_id_001 - Crystal clear naming convention
    11) table_name.table_name_id - Why put the table name into the column name?
    12) Surrogate keys - Why not use Natural keys?

    13) "This procedure searches an unexistent id (identifier) and valid for a table." - I am seeing you both now and directly. The whole in my head is from laughter. Please to do again, yes.

    On a more personal note - Gerry, we got your resume for the DBA position. Unfortunately, the position has already been filled. We will keep your name in mind though since we want to make sure that we only hire the most qualified candidates.

    Piece out...

  • Guayo (unregistered)

    @Bustaz Kool...
    1) No owner? So what, what if the owner is dynamic? What if this sp is going to be put in more than one schema (owner)?
    2) The code will fail if Null table name is passed... so what? I agree that it's better to specify all the constraints you can and be prepared to handle invalid data but the lack of null handling is not a WTF IMO.
    3) Yes the code will work if he don't do the add like: WHEN MIN('+@table_name+'_id) IS NULL THEN 1... but what's the big deal here?
    4) Why? Yes coalesce is shorter, so what? I would put ISNULL(MIN('+@table_name+'_id)+1, 1) but who gives a rat about this?
    5) What I said on 1
    6) This sp NERVER returns Null, It would return 1 (I guess), yes it doesn't really handle when you are out of integers to your keys... who says this is the code responsible of handle such thing?
    7) Yes... not very pretty but what's the big deal?
    8) It's considered good practice to explicitly drop any temp table created in the sp. So kudos to the sp author.
    9) Maybe he deals with concurrent sessions in a way that this is handled. How can anyone see what's the way concurrency is handled just by looking at this sp?
    10) Well, the name suggest a naming convention being used, yes it's not easy to understand but it is a good thing some naming convention is used.
    11) Is not the first time I see this naming scheme, I personally don't like it to much but what's the big deal? Some people say it makes easier to understand joins is select statements.
    12) Because some requirement or spec we don't know?... Is that to hard to believe?


    Anyway you can find whatever irregularities you want but I still think the WTF here is believing the sp just return the next id.

  • Anon (unregistered)

    Guayo,

    <quote>
    I don’t get it!
    This sproc doesn’t give the next id! This sproc gies you the first unused id! Maybe this isn’t the best code to do that but the bashing of this code based on a wrong premise is a bigger WTF to me.
    </quote>

    1. Its a bloody ugly block of SQL. It will scale hideously. Lets do two tablescans to get an ID!
    2. OK, so it isn't exactly the same as IDENTITY - it fills gaps. But what is the benefit of doing that? Are the downsides (speed, concurrency etc) worth it in this case? I doubt it.

    <quote>
    9) Maybe he deals with concurrent sessions in a way that this is handled. How can anyone see what's the way concurrency is handled just by looking at this sp?
    </quote>

    3. Lets be reasonable. Someone who wrote that stored proc is highyl unlikely to be using the necessary isolation levels etc etc to make the stored proc actually work.

  • Bustaz Kool (unregistered)

    @Guayo ... CHILLAX!

    My good friend Gerry said "There is absolutely nothing wrong with this code". I was able to spit out over a dozen things wrong with it without breaking a sweat.

    I'll address some of your concerns now...

    2) How can any self-respecting professional say that trying to draw data from a table when the table name is undefined is anything but a WTF? Especially when you have to go out of your way to ALLOW the table name to be undefined.

    5) The use of the owner when invoking a procedure is faster. It's only a small bump but why not institutionalize the useage.

    6) The code does not return Null... Mea Culpa.

    9) Concurrency - How could a higher level handle this issue? The caller asks for a number and uses it; even if its a duplicate. How would you propose the caller detect that the number returned is a duplicate?

    10&11) Naming Convention - Using a GUID as part of a procedure naming convention also implies that a convention is being used - it just doesn't imply that it is a good convention. I've seen several variations on the convention used here and they all obfuscate rather than illuminate. Just because people do it doesn't mean that it's a good idea. Foolish consistency...blah, blah, etc.

    12) It has been shown that surrogate keys will get out of sync with natural keys. I know that this is a very common technique to use but it still doesn't mean that it's a good one.

    1,3,4,7) None of these results in erroneous data being generated. Rather they are indicitive of the pandemic issue of writing poorly thought out and constructed code.

    IN RE: "but I still think the WTF here is believing the sp just return the next id."

    It was explained to me that this was an "implementation of a sequential id (identifier) generator in T-SQL". That's the spec that I'm operating off of. Crystalline? Heck, no.

    I've seen this type of procedure many times before. Yes, they have applications other than GetNextSurrogateKey (Hey, we can't use that for a procedure name - we'll never know which Division Manager ordered it or which shift will be using it!!!) BUT it's a small leap of faith to say that this is a routine to get the next surrogate key.

    That's all....Lovers

  • Anon (unregistered)

    Ben,

    <quote>
    Jeff, Anon, me: Doesn't transactional isolation prevent that from happening? Or am I missing something? (Maybe the fact that a lot of people reduce isolation as a performance hack...)
    </quote>

    Not with SQL Server's default isolation level (Read committed).

    Example scenario (assume current max is 10 and two PCs):

    Bob's PC calls sproc and gets next ID of 11.
    Mary's PC calls sproc and gets next ID of 11.
    Bob inserts new record (ID = 11).
    Mary inserts new record (ID = 11). boom

  • Guayo (unregistered)

    @Bustaz Kool
    Ok, Ill chill out... precisely that's what I'm trying to say here... chill out and look at the code before jumping at the coder throat. I'm pretty much laugh at WTF code as anyone here but I hate when people bash something they just don't understand and their criticism is based in their misconception.
    I mean... It's this sp so hard? How can anyone believe this code just return the next id in the sequence?
    I'm not saying the code is perfect, damn it!... I kind of agree with some of the things you said but I do believe all of what you said hardly makes this sp a WTF.
    We don't know where, how, when this sp will be used so I don't have the mood to engage in a discussion here about ways to handle concurrency knowing just this sp from the entire system.

  • Guayo (unregistered)

    @Anon
    1) It will scale hideously if this is the way of getting the next id for new records. And that depends of the size of the tables, However you have a point here... it could be that this sp is in fact used to get the next key for all the tables (the fact this sp is so generic it's suspicious) but we don't know that. That would be a info that would definitely change my opinion about the WTFness of this sp (instead we have Oracle' jokes. BTW I take Oracle sequences over SQL Server identity columns any day, BTW#2, I find the whole Donald Burleson thing hilarious).
    You may believe this SQL it's ugly, why don't you post a snip to get the first unused id in any table to see the good way of doing it.

    2) You ask what are the benefits of finding id gaps? I don't know if just ignoring the gaps would be possible in this system, I ask: what are the requirements this sp solves?

    3) Damn... why this black and white... there are a lot of ways to deal with concurrency... sometimes the best way is just to retry a transaction when a concurrency error occurs instead of blocking a resource ahead of time.

  • Bustaz Kool (unregistered)

    Guayo,

    Did you...(ya' know)... write this code? You're acting awfully defensive for someone who only has an academic interest in the topic.

    As my children are wont to note, "He who denied it; supplied it." (Wink, wink)

  • Anon (unregistered)

    Guayo,

    <quote>
    1) It will scale hideously if this is the way of getting the next id for new records. And that depends of the size of the tables, However you have a point here... it could be that this sp is in fact used to get the next key for all the tables (the fact this sp is so generic it's suspicious) but we don't know that. That would be a info that would definitely change my opinion about the WTFness of this sp (instead we have Oracle' jokes. BTW I take Oracle sequences over SQL Server identity columns any day, BTW#2, I find the whole Donald Burleson thing hilarious).
    You may believe this SQL it's ugly, why don't you post a snip to get the first unused id in any table to see the good way of doing it.
    </quote>

    My point is it will tablescan twice per call. Nothing to do with whether it handles multiple tables or not.

    <quote>
    3) Damn... why this black and white... there are a lot of ways to deal with concurrency... sometimes the best way is just to retry a transaction when a concurrency error occurs instead of blocking a resource ahead of time.
    </quote>

    The code smells. Badly. For example - why does he use a temp table? Its completely unnecessary. And use Select * to query it? Ouch.

    My point being - if the programmer is making those kind of elementary 'mistakes' its hardly likely he has built in a retry mechanism like you propose (which can be quite tricky, especially when dealing with FKs). In all likelihood (though of course I can't prove it) the author didn't even think of transactions...

  • Bustaz Kool (unregistered)

    Hey Viagra,

    Nice joke on the pharmaceutical term. ROFL

  • George Prado (unregistered)

    @viagra; Nice sense o'humor. But how is your post relevant to this WTF? this itself is a WTF

  • Viagra (unregistered)

    Hey hey hey... I was just trying to bring some fun into you sick programmers.... seems like you guys have completely forgotten the meaning of the word fun... Morons

  • (cs) in reply to Bustaz Kool
    Bustaz Kool:
    Guayo,

    Did you...(ya' know)... write this code? You're acting awfully defensive for someone who only has an academic interest in the topic.

    As my children are wont to note, "He who denied it; supplied it."; (Wink, wink)

    But what about the older proverb, "He who smelt it, dealt it."?

Leave a comment on “GEN_ID()”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article