• Developer (unregistered)

    Bawww, people in other professions can't do my profession as well as I can.

  • Black Bart (unregistered)

    Even with the fix, 2 users can end up with the same ID. Dear Dev: Listen to the wisdom of Marle.

  • Balu (unregistered)

    SCOPE_IDENTITY?

    Well, I may just be a developer, but after reading the documentation of SCOPE_IDENTITY I'd have used an "INSERT ... OUTPUT" statement to get the last inserted ID.

    Or are triggers forbidden in their databases by The Rule Of Merle?

  • Monkey (unregistered)

    Just realised UserLoginName is actually an integer id column. <bleurgh>

  • darkyuris (unregistered)

    silly DBAs. living in past, thinking that data alone is what's important.

  • Geoff (unregistered) in reply to Monkey
    Monkey:
    Just realised UserLoginName is actually an integer id column. <bleurgh>

    I thought that was the TWTF myself.

  • regressor (unregistered)

    Being incompetent has nothing to do with them being developers. By now I think it's part of the human condition.

    Can't help but hear Steve Ballmer every time someone talks about developers! developers! developers!

  • QJo (unregistered)

    Looks like a bit of a cnt to me.

  • (cs)

    Seems like the code is an attempt to return the next available ID, rather than the one just inserted. Good advice on making the column the identity and looking at SCOPE_IDENTITY, but that would return a different answer than the code presented (or not, depending on concurrent operations).

  • t (unregistered) in reply to Balu

    umm...no. SCOPE_IDENTITY is trigger-safe, as it returns the last identity value generated in the scope. triggers execute in a different scope.

  • (cs) in reply to QJo
    QJo:
    Looks like a bit of a cnt to me.

    +1 internets

  • Smug Unix User (unregistered)

    Why not use an output on the insert?

  • The Balance (unregistered)

    I think the number of poorly trained people who drift into becoming sysadmins and DBAs generally outweighs the number of poorly trained people who drift into becoming developers.

    The difference however, is that the well trained developers can usually do the job of a DBA, whereas well trained DBAs wouldn't know the first thing about development.

  • (cs) in reply to oheso

    Well, yes, the results would be different- identity columns would work consistently, whereas this approach has a non-zero chance of running into a race condition and attempting to reuse the same identity.

  • Hannes (unregistered) in reply to regressor
    regressor:
    Can't help but hear Steve Ballmer every time someone talks about developers! developers! developers!

    I really wonder why

    I also do wonder why people like Marle always seem to think of themselves as a "know-it-all".

  • faoileag (unregistered)

    Try as I might, the reason for the "while" loop in the stored procedure escapes me. Apart from the fact that it can result in an endless loop if UserLoginName is not unique, why not simply do a "set @tempid = (select max(UserLoginName) from user_table;) + 1"?

    And why the advice "use SCOPE_IDENTITY to get the id for the inserted record"? If I understand the code correctly, it returns the id of the last inserted record + 1, so (presumably) the id of the next record to insert?

    Or am I completely misunderstanding TransactSQL?

  • faoileag (unregistered) in reply to The Balance
    The Balance:
    ...that the well trained developers can usually do the job of a DBA
    More often than not, developers, whether they are well trained or not, just simply do the job of DBA because the role of DBA does not exist on the company's organizational chart even if the company relies heavily on databases.
  • (cs) in reply to faoileag
    faoileag:
    Try as I might, the reason for the "while" loop in the stored procedure escapes me. Apart from the fact that it can result in an endless loop if UserLoginName is not unique, why not simply do a "set @tempid = (select max(UserLoginName) from user_table;) + 1"?

    And why the advice "use SCOPE_IDENTITY to get the id for the inserted record"? If I understand the code correctly, it returns the id of the last inserted record + 1, so (presumably) the id of the next record to insert?

    Or am I completely misunderstanding TransactSQL?

    What do you think would happen if two processes ran the stored procedure at the same time?
  • Matt (unregistered)

    Who says a 2-ton boulder can't be aerodynamic?

  • fragile (unregistered)

    dev's lazy which is a good thing. He's refactoring someone else's work as little as possible to get back to their job.

    Why are all the smart people in these articles women and all the dumb people men?

  • faoileag (unregistered) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    What do you think would happen if two processes ran the stored procedure at the same time?
    Pffft, dunno, havoc? Of course the stored procedure is not thread safe.

    But, as I said, the stored procedure returns the lowest not yet assigned id from UserLoginName, i.e. max(UserLoginName) + 1, and not the last inserted id.

    SCOPE_IDENTITY can only be used after an insert and only if used in the same batch as that insert.

    What if the devleoper approached adding a new entry like:

    1. get next available id from UserLoginName
    2. insert the data with that id

    After all, the UserLoginName seems to be a normal integer column, so whoever created the table's schema was new to T-SQL or simply didn't care.

  • Bloat-a-saurus (unregistered)

    Um... Why leave the previous code still commented in?

  • faoileag (unregistered) in reply to fragile
    fragile:
    Why are all the smart people in these articles women and all the dumb people men?
    You mean like Paula?
  • Dog (unregistered)

    Most DBAs couldn't code their way out of a wet paper bag. Most DBAs I've worked with are arrogant, stupid, and a fucking pain to deal with.

  • Dog (unregistered) in reply to faoileag

    That's because it was written by a fucking DBA with the brain capacity of a toadstool.

  • (cs)

    TRWTF is people who use the word "performant".

  • (cs)

    From experienced <Role> to someone who has less experience in <Role>:

    Use <Feature> which you probably did not know about.

    This is not a WTF -- it is just the proper role of someone with more experience.

    I do it all the time to my junior developers. As in: instead of writing this loop, use Linq. Or, instead of using reflection, use proper inheritance. Etc.

    This could have easily been a story about a golf pro showing a kid how to swing a club properly.

  • me (unregistered) in reply to Matt
    Matt:
    Who says a 2-ton boulder can't be aerodynamic?
    They sure seem pretty aerodynamic when they fall on coyotes. :)
  • Anon (unregistered) in reply to fragile
    fragile:
    dev's lazy which is a good thing. He's refactoring someone else's work as little as possible to get back to their job.

    Why are all the smart people in these articles women and all the dumb people men?

    DINGDINGDING

    If it ain't broke, don't fix it.

    If it is broke, do as little as possible to avoid breaking everything else.

  • usitas (unregistered) in reply to fragile
    fragile:
    Why are all the smart people in these articles women and all the dumb people men?
    Obviously part of the "fictionalization" process.....
  • (cs) in reply to faoileag
    faoileag:
    But, as I said, the stored procedure returns the lowest not yet assigned id from UserLoginName, i.e. max(UserLoginName) + 1, and not the last inserted id.

    SCOPE_IDENTITY can only be used after an insert and only if used in the same batch as that insert.

    So what would be wrong with doing the insert in the stored procedure instead of wherever it's currently being done?

    faoileag:
    What if the devleoper approached adding a new entry like: 1. get next available id from UserLoginName 2. insert the data with that id
    That way is still not thread-safe unless done inside a serializable transaction.
  • anonymous (unregistered) in reply to Dog
    Dog:
    Most DBAs couldn't code their way out of a wet paper bag. Most DBAs I've worked with are arrogant, stupid, and a fucking pain to deal with.
    Ditto.

    Our project: using Oracle DB using extended Latin encoding. Our problem: no Unicode support, users complaining. Our solution: change encoding to UTF-8 DBA's position: UTF-8 is forbidden, you may only change to WE8DEC (which is even worse than extended Latin).

    He also refuses to give performance advisory or anything if queries are not supplied using Oracle's (+) syntax. ANSI SQL is totally off limits, because "that's how we always did it".

    Out stance? EFF YOU! We use ANSI SQL, and he's forced to do his frickin' job, whether he likes the syntax or not. And we got him to migrate us to UTF-8 too, against his will.

    Feels good.

  • JoeNotCharles (unregistered)

    The best thing to do here is commit the developer's fix (since it's better than the existing code even though it doesn't fix all the problems) and file a new bug to replace the whole thing with the correct solution. Then that bug can be prioritized based on how bad the rest of the problems are and how much other work is competing for attention.

  • (cs) in reply to faoileag
    faoileag:
    Try as I might, the reason for the "while" loop in the stored procedure escapes me. Apart from the fact that it can result in an endless loop if UserLoginName is not unique, why not simply do a "set @tempid = (select max(UserLoginName) from user_table;) + 1"?

    And why the advice "use SCOPE_IDENTITY to get the id for the inserted record"? If I understand the code correctly, it returns the id of the last inserted record + 1, so (presumably) the id of the next record to insert?

    Or am I completely misunderstanding TransactSQL?

    You're understanding it correctly. However, Merle is going one step further in the analysis of "Why would they want to do this in the first place?" Her guess is reasonable, although not necessarily correct.
  • (cs) in reply to Monkey
    Monkey:
    Just realised UserLoginName is actually an integer id column. <bleurgh>

    The only thing in the code posted that even suggests that is the declaration of @tempid as an int and @tempid being set to the max value of UserLoginName. The column itself could be another data type entirely, like a numeric, that the database engine provides an implicit conversion for.

  • Valued Service (unregistered)

    Yep, developers can't make good DBAs, and DBAs can't make good developers.

    And they both suck at each other's domain.

    I mean, neither thought to use GUIDs, and avoid the calculations / identity scope issue altogether.

  • Foobar (unregistered) in reply to Frosh
    Frosh:
    UserLoginName. The column itself could be another data type entirely, like a numeric, that the database engine provides an implicit conversion for.
    Bah, that's silly. A better option is to store the user's actual name in the field, as text, then cast the resulting bitstream as an integer.

    For example, UserLoginName "ArthurPhilipDent" would logically be followed by "ArthUrpHilIpdenu". Only a complete jerk, a real kneebiter, would f**k this up.

  • (cs) in reply to faoileag
    faoileag:
    Try as I might, the reason for the "while" loop in the stored procedure escapes me. Apart from the fact that it can result in an endless loop if UserLoginName is not unique, why not simply do a "set @tempid = (select max(UserLoginName) from user_table;) + 1"?

    And why the advice "use SCOPE_IDENTITY to get the id for the inserted record"? If I understand the code correctly, it returns the id of the last inserted record + 1, so (presumably) the id of the next record to insert?

    Or am I completely misunderstanding TransactSQL?

    Yes, you are.

    It takes an IQ of roughly 70 to understand that MAX+1 is bad. I've seen someone fired for using it.

  • (cs) in reply to Anon
    Anon:
    fragile:
    dev's lazy which is a good thing. He's refactoring someone else's work as little as possible to get back to their job.

    Why are all the smart people in these articles women and all the dumb people men?

    DINGDINGDING

    If it ain't broke, don't fix it.

    If it is broke, do as little as possible to avoid breaking everything else.

    The new change still includes one of the most basic anti-patterns than any dev who has even heard of a database should know about.

  • Johnny Leroux (unregistered) in reply to anonymous

    You showed that arrogant SOB who's boss!

  • (cs) in reply to Foobar
    Foobar:
    Frosh:
    UserLoginName. The column itself could be another data type entirely, like a numeric, that the database engine provides an implicit conversion for.
    Bah, that's silly. A better option is to store the user's actual name in the field, as text, then cast the resulting bitstream as an integer.

    For example, UserLoginName "ArthurPhilipDent" would logically be followed by "ArthUrpHilIpdenu". Only a complete jerk, a real kneebiter, would f**k this up.

    When you're immortal, you live long enough to see everything.

  • Cecil Dkal (unregistered) in reply to Valued Service

    as long as the table was not acutally clustered on a GUID, as this can result in severe degradation of performance caused by excessive page splitting.

  • (cs) in reply to Valued Service
    Valued Service:
    Yep, developers can't make good DBAs, and DBAs can't make good developers.

    And they both suck at each other's domain.

    I mean, neither thought to use GUIDs, and avoid the calculations / identity scope issue altogether.

    We haven't had a good primary key flamewar in a long time. I'll start: GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

  • (cs) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    Valued Service:
    Yep, developers can't make good DBAs, and DBAs can't make good developers.

    And they both suck at each other's domain.

    I mean, neither thought to use GUIDs, and avoid the calculations / identity scope issue altogether.

    We haven't had a good primary key flamewar in a long time. I'll start: GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

    You don't think they'd make a good UserLoginName? :)

  • (cs) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    Valued Service:
    Yep, developers can't make good DBAs, and DBAs can't make good developers.

    And they both suck at each other's domain.

    I mean, neither thought to use GUIDs, and avoid the calculations / identity scope issue altogether.

    We haven't had a good primary key flamewar in a long time. I'll start: GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

    Indeed. I wonder what kind of user would want to use a GUID as their login name...

  • (cs) in reply to Planar
    Planar:
    Indeed. I wonder what kind of user would want to use a GUID as their login name...

    My name is actually a GUID, you just can't tell because I spell it in Canadian.

  • Anon (unregistered) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    We haven't had a good primary key flamewar in a long time. I'll start: GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

    GUIDs are nice when merging data between databases. Identity columns tend to cause key collisions.

  • (cs) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.
    The only real advantage is that you stand a chance to merge the IDs between different databases (which does come up as a real world requirement sometimes). That's doubly true for UUIDs. Otherwise, an integer (of relevant size) is Good Enough.

    User names aren't keys. Nor are US SS#s.

  • (cs) in reply to PedanticCurmudgeon
    PedanticCurmudgeon:
    GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

    They can possibly mitigate a security hole arising from a guessable index. Mind, if you've got such a security hole, it means your code is broken, not that you're using the wrong index.

    Or if you just don't want your user knowing he's one of the first 10 people to register for your application ... But then, this use case can be solved by seeding the index with ARBITRARILY_LARGE_INT.

  • (cs) in reply to Anon
    Anon:
    PedanticCurmudgeon:
    We haven't had a good primary key flamewar in a long time. I'll start: GUIDs are a PITA to work with and don't offer any advantages over identity columns that would compensate for that.

    GUIDs are nice when merging data between databases. Identity columns tend to cause key collisions.

    Then use GUIDs as a temporary supplemental identifier for a temporary solution. There's no need to have a GUID column just in case you need to merge data. Use an identity column, and if you need to merge the data, use a GUID, merge it, then remove the GUID. Unless you're merging (or something else that may require a GUID), it's not the right type of column.

Leave a comment on “The Fix”

Log In or post as a guest

Replying to comment #:

Log Out

« Return to Article