- Feature Articles
- CodeSOD
- Error'd
-
Forums
-
Other Articles
- Random Article
- Alex's Soapbox
- Announcements
- Best of…
- Best of Email
- Best of the Sidebar
- Bring Your Own Code
- Coded Smorgasbord
- Mandatory Fun Day
- Off Topic
- Representative Line
- News Roundup
- Editor's Soapbox
- Software on the Rocks
- Souvenir Potpourri
- Sponsor Post
- Tales from the Interview
- The Daily WTF: Live
- Virtudyne
Admin
It also fails on the frist row added to the database due to no result available
Admin
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...
Yazeran
Admin
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.
Admin
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.
Admin
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.
Admin
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.
Admin
Not how sprintf works at all... it returns an int, the first parameter is the output buffer....
Admin
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.
Admin
Are you really surprised that PHP reuses a common function name to do something subtly, but not completely, different?
Admin
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.
Admin
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.
Admin
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.)
Admin
Admin
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.
Admin
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.
Admin
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.
Admin
It's doing a SEELCT MAX() so that should return a single row with 0 in the current_id column
Admin
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.
Admin
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"?
Admin
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.
Admin
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
Admin
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'.
Admin
DEC's (later Oracle's) Rdb I think? Fun times...
Admin
That would be a WTF in itself.
In fact, if the table is empty you'll get a single row with a
NULLvalue.