• LCrawford (unregistered)

    It also fails on the frist row added to the database due to no result available

  • Yazeran (unregistered)

    Ok, confession here

    I did something similar some 20 years ago when I build by first database and had only gotten to chapter 3 in 'SQL for dummies' before I had to implement something.

    In my (extremely limited) defense the primary key (which was NOT declared as such) was a varchar (and the format was 'WW_DDDDDD' where WW was a prefix and DDDDDD was a 6-digit number - think 'RT_000000','RT_000001','RT_000002' and so on) The Prefix was required by the users and i didn't get the idea to only add it on the presentation layer....

    So I had to make something similar.

    However I did use transactions and the primary key column did have a UNIQUE clause on it so it was not quite as bad as this...

    However I have learnt a lot since then, but seeing things like this bring back some unpleasant memories...


  • Junkfoodjunkie (unregistered)

    How the unholy h*ll did anyone ever think this was a good idea? 😭 Never manually increment id's, just use them for knowing which rows to delete or update, or other editing or selection use cases. How do you get a job working as a programmer with this kind of quality work? I honestly wonder, because I'm looking for a job, and it seems it should be no problem getting one.

  • (nodebb) in reply to Yazeran

    Confession for confession, a project I work on still has a similar (properly error-checked and transacted) code. In our defense, the project has to support several DBMSes, not all of which offer an easy way to retrieve the ID the sequence just generated for your new row.

    Addendum 2023-04-27 07:45: Inserting into a table with an auto-increment column is easy; knowing which value you just inserted is the hard part.

  • dpm (unregistered) in reply to Yazeran

    I did something similar too --- my excuse is that it was 39 years ago, on an operating system (VAX/VMS) which had a built-in RDBMS that did not offer an auto-increment feature. Not that I'm old or anything.

  • Tim Ward (unregistered)

    The problem with the built-in autoincrement (last time I looked, at least) is that it isn't standard, so every RDBMS has a different way of doing it - that's my guess as to the motivation here.

  • TheCPUWizard (unregistered)

    Not how sprintf works at all... it returns an int, the first parameter is the output buffer....

  • Yazeran (unregistered) in reply to Medinoc

    Addendum 2023-04-27 07:45: Inserting into a table with an auto-increment column is easy; knowing which value you just inserted is the hard part.

    Which is why I (now) always add a 'new_table_entry(name)' function when creating a new table which inserts a row and returns the ID of the new row (it may also add additional meta information for logging and/or data management purposes). At least in Postgres it is straightforward.

  • (author) in reply to TheCPUWizard

    Are you really surprised that PHP reuses a common function name to do something subtly, but not completely, different?

  • Tim (unregistered)

    my current system manually generates IDs because (at the time it was written at least), different databases used different syntax and the software had to be compatible across a number of databases. I like to avoid database-specific features where possible.

    of course it doesn't use the max()+1 formula though; it uses another table called latest_id which it exclusively locks.

  • Steve (unregistered)
    Comment held for moderation.
  • Steve (unregistered)
    Comment held for moderation.
  • Richard Brantley (unregistered)

    If the ID must be generated outside of the database, and you must guarantee uniqueness, I use GUIDs. Yes, collisions can happen, but in thirty years I haven;’t seen it happen in the wild.

  • (nodebb)

    For those who have the GREAT PLEASURE of using MS SQL, please look into @@IDENTITY, SCOPE_IDENTITY() and OUTPUT INSERTED options in detail, as in, RTFM - all of it, and keep INSTEAD OF TRIGGER in mind. I recently had an issue because of all that mess. (Not saying MS SQL is crap - it has a lot of good qualities - but all RDBMSes have issues and this is one of them.)

  • I'm not a robot (unregistered)
    Now sure, we all knew there was going to be a SQL injection vulnerability in this code
    Only if the table and/or column name is taken from an untrusted source, of which there's no evidence in the posted code.
    You know what else isn't enforced? Transactions and error handling.
    Again, we don't know that from the posted code. $this->query() might throw an exception if the query fails, and transactions might be handled at a higher level. If this runs in a transaction with "isolation level serializable" and the application is able to retry transactions that fail with a deadlock error, then it should be fine as far as I can tell, albeit with less concurrency than if it used the built-in auto-increment.
    there's nothing atomic about this
    It's a single query, of course it's atomic. Maybe you meant the entire operation that uses this function, but WE DON'T KNOW THAT FROM THE POSTED CODE.
    It also fails on the frist row added to the database due to no result available
    If there are no rows, MAX() returns NULL, which is coerced to zero by the addition. Sloppy, yes, but it won't fail.
  • Kythyria (unregistered) in reply to TheCPUWizard

    Having to supply the buffer is a C thing. Most languages that aren't C(++) just make the sprintf-equivalent allocate, and some of them (Ruby, IIRC, is among them too) even call it sprintf. It's way less error-prone, and formatting to an existing buffer is frequently done with the idea of a memory stream that works with the counterpart of fprintf.

  • (nodebb)

    The "injection vulnerability" isn't a big deal with this code; there isn't a way to use a prepared statement for this (except by hiding stuff just as bad in a callable DB function) as table and column names can't (usually) be used as things you inject into a prepared statement.

    The lack of atomicity isn't a big problem either. The caller needs to hold a transaction. Big Deal.

    The rejection of the database's own capabilities... that's the big deal.

  • Abigail (unregistered)
    Comment held for moderation.
  • Randal L. Schwartz (github)

    I remember reading a project where the client was upset because they used the serial ID as the public-facing "invoice number", and when a transaction would roll back while setting up the initial invoice creation, of course that integer would be skipped. So of course accounting got involved because "how come you can't assign numbers sequentially and how do we audit this?". Never heard how that turned out.

  • Fizzlecist (unregistered) in reply to LCrawford

    It's doing a SEELCT MAX() so that should return a single row with 0 in the current_id column

  • (nodebb)

    This isn't far off what we do at work, but at least in our case, it's because the RDMS doesn't actually have auto-increment capabilities.

    To get around the race-condition, we have a sequence table, which has a serial number that is unique for each type of sequence we want to create and the current sequence value.

    We then lock the record in question, increment the value and use that new value as the ID for a new record in a table.

    To deal with potential issues that might arise from a failure to actually create the record, the sequence tables rows get updated with a NO-UNDO (We use Progress Openedge), that way, although the transaction itself gets undone, the sequence itself remains at its new value.

  • (nodebb)

    I inherited a public-facing system in one job that used unique "ticket numbers" instead of an auto-incrementing serial number. I think they were 12 alphanumeric digits with dashes in strategic spots. There was no way to change that - and the existing code would generate a random "ticket number" and see if the insert failed. And of course, as the business grew, collisions grew more common.

    Eventually I rewrote the insert logic to generate a prefix, fetch all "ticket numbers" with that prefix and generate a new one that was not in that set. And then insert the new record. Did I say there was no way to just use a proper auto-incrementing field and turn it into one of these "ticket numbers"?

  • Brain (unregistered)

    sad to say that ive come across this particular anti pattern dozens of times in my career in various business systems... Practically exactly this same code. Never a transaction in sight.

  • Morten (unregistered) in reply to Yazeran
    Comment held for moderation.
  • Tim (unregistered)

    if you're generating values yourself using a separate sequence table, really you should generate them in a separate transaction (maybe even on a separate database connection). you definitely don't want to be locking that master sequence table any longer than necessary

  • (nodebb) in reply to Tim

    In our case, locks are per-record not per-table :)

    Would hate to use an RDBMS that forced you to do table-locks only.

    I basically just do 'FIND Sequence EXCLUSIVE-LOCK WHERE Serial_No = 1 NO-ERROR', if it doesn't exist, create it and set it to 0, then increment, othewise just increment and use new value :)

    Addendum 2023-04-28 07:54: Once I have finished with it, I either 'FIND CURRENT Sequence NO-LOCK NO-ERROR' or just 'RELEASE Sequence'.

  • (nodebb) in reply to dpm

    I did something similar too --- my excuse is that it was 39 years ago, on an operating system (VAX/VMS) which had a built-in RDBMS that did not offer an auto-increment feature. Not that I'm old or anything.

    DEC's (later Oracle's) Rdb I think? Fun times...

  • (nodebb) in reply to Fizzlecist

    That would be a WTF in itself.

    In fact, if the table is empty you'll get a single row with a NULL value.

  • TrayKnots (unregistered)
    Comment held for moderation.

Leave a comment on “An Incremental Replacement”

Log In or post as a guest

Replying to comment #:

« Return to Article