- 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
LOL. Nice video.
Admin
That looks like something that would either
OR ([something] = @Original_something)...Admin
There's so many conditions on that query, the page scrolls almost as much as
<!-- Emoji'd by MobileEmoji 0.2.0-->t/1000…Admin
Actually such techniques are common if you want to ensure that the row has not been changed. A common concurrency problems is that person A selects a record from the database, looks at and determines it is "old"/"obsolete" and generates a "delete"...in the meantime person B has looked at the same record, updated information and posted it to the database.
If one just looks at the "key" to select the row, then Person A can easily delete Persons B's work.
Admin
Two words: Database transactions.
Admin
If the CRUD is properly wrapped in transactions, then it doesn't matter; one action will complete, and the other will raise an error that can be handled properly.
Also, Hanzo'd.
<!-- Emoji'd by MobileEmoji 0.2.0-->Admin
Transactions will save you if you're reading and writing at the same time (and you have pessimistic locking, yuck). But that doesn't prevent the sorts of race conditions that (I think) @TheCPUWizard is talking about. That's usually called optimistic locking.
For instance, you might keep a version field that you always increment when you write to the record. So you start a transaction and then make sure it hasn't changed from when you first read the data. If you don't want to "waste" the space, you can CHECK ALL THE THINGS like the article might be doing. I say might, because :wtf:...ain't nobody got time to go through and verify that shit.
Admin
Two more words: "optimistic concurrency".
Database transactions are not designed to lock records while they're being viewed or edited in an application by users.
Admin
Let's add a column or two, and then SearchGuard is no more safe until the web form has been updated too...
Admin
I can imagine something like this being generated by an ORM.
Admin
Microsoft's LINQ-to-SQL does exactly this if the table doesn't have a timestamp column to implement optimistic concurrency. It does this for both updates and deletes. I usually add a timestamp column to frequently edited tables to improve performance for this reason.
Admin
WTF: Deleting an employee record.
T**R**WTF: There is no "status" field on the employee record.
Admin
Obviously if the record exists, status is active, if not status is inactive. Duh
Admin
TRWTF is that someone thinks they know enough about programming to recognise a WTF but didn't know what this is.
It's quite obviously an auto-generated statement implementing optimistic locking.
If the row is not found (rowcount = 0) you get an error saying "someone else has edited that record. Click OK to load the latest version" or similar. Such statements are auto-generated by VB6/ADO.Net from the database schema. You don't write them yourself.
Admin
If you're deleting a record, do you really care if someone changed it?
Admin
I gotta admit, I'm not seeing the WTF. It's kind of ugly, but it's the only safe solution in a bunch of cases. I mean, obviously, yeah, this could be auto-generated code. Or:
(1) If you are in a shop that insists that everything has to be done through a stored procedure ("that's the real WTF?") then you end up with a stored proc like this for every table a front end developer might delete from.
I'm betting that's the situation here. This (possibly changed and anonymized) code is out of a stored procedure where each of those column values was passed as a parameter. It may or may not be surrounded by other code that does different verifications. It might still be a WTF but it might not be, depending on context.
(2) As TheCPUWizard suggests, this could be to make sure the row hasn't changed. Transactions won't help for a lot of types of applications where someone can pull up a record, sit there staring at it for a half an hour, then make a change and/or delete it. You can't lock the record for all that time, and this is the cheap and easy solution.
If I'm deleting a record, I think I do care if someone changed it. There's a good chance one of us is doing something wrong. It might be them, it might be me, but either way I would want to see those changes first.
Admin
Yes. I cut off the top of the code because it was long enough already, but it began
ALTER PROCEDUREIt was not. There was a list of params, then
SET NOCOUNT OFF;, then the delete statement. It may have had other lines in the codebase that the submitter removed, but then, the submitter could have made up the whole thing in the first place...Our shop does mandate procs, though, and we tend to either 1) set a deleted flag on the record or 2) delete from a series of normalized tables using the ID as the input. So the two do not go hand in hand.
Admin
Thanks for providing some context. It certainly makes me question that this is an effective way of doing things.
Although not quite a "WTF" so much as inexperience, dealing with something important like Employee records you probably do want to use a flag and do a "logical delete" or somehow deactivate the record instead of a real permanent delete.
And as you say, I do wonder why this doesn't just use an ID or primary key fields, unless it is (as TheCPUWizard suggested) some way of catching changed records. Although I suppose then that a timestamp would make a lot more sense.
So... now I'm really scratching my head, at least, wondering what the point of doing it this way was.
Admin
SearchGuard is great. It's just too bad that they found the rows to be deleted by using this select:
SearchGuard carefully matched each selected row in turn before deleting it, just as expected.
Admin
Looks exactly like the code .Net datasets generate when you let them create your stored procedures through a strongly typed dataset.
Or, someone's adaptation of that... shudder
Admin
Interesting. On the main TDWTF page, the entire article synopsis, including the hyperlinked SearchGuard, just links to the full article page. So the youtube link is hidden unless you open the full article. Bug or feature?
Bonus DiscoWTF: no underline widget in the editor!
Admin
Technically the Youtube link isn't exactly hidden, the entire article is overlaid with the <a> element inside the <article> element.
If you delete it with an HTML inspector, you'll find you can now click the YouTube link, but can't get into the article page proper.
Admin
Would you guys have preferred an embed?
Admin
I think the complaint is that clickable elements don't work on the front page, it is likely even an embed wouldn't work. Trying it now... ;)
Edit: Answer: No. No it does not.
Admin
I don't think an embed is necessary (even if it did happen to solve the problem). It would be nice if the links worked on the front page, is all. Personally I don't see why the whole synopsis needs to be clickable. A standard "Read more..." link at the bottom of each synopsis should suffice, and additionally would indicate that there's more unseen content available (which is not obvious at all in the current design).
Admin
Sadly, I have no say over the front page design :)
Admin
I figured, just rambling. Thanks for listening though. :smile:
Admin
Who actually deletes records?
Hide records? Sure! Delete records? Nope!
Admin
Admin
Very good point bringing up isolation levels as that is sometimes overlooked. If you have full control over the app, this problem can be resolved by using them.
In Oracle the default isolation levels maintains a copy of the data being changed in memory until all locks are removed, then the new value is returned. This allows reads to continue and get the full old row which helps you ensure you don't get a full dirty read (such as half the row is updated, the other half is not) while not blocking writes.
SQL Server by default blocks readers and writers but you can change the isolation level per query or server wide to act more like Oracle but it adds about 14 bytes per row if I'm not mistaken.
Admin
The query would not be much WTF if the query is generated by some query generation tool but there's no identity column on the table. (Yes I see there is employee_id, but I've seen countless time where xxx_id is not unique. So maybe TRWTF is employee_id is not identity field only)
Admin
You can submit it here, I guess: https://github.com/tdwtf/WtfWebApp
Admin
Who cares if the code is generated? Anyone who doesn't delete the rest after testing the employee ID should be shot on sight. Or are there multiple employees with the same ID?
Also:
I can't help wondering... Which is better, null sex or original sex?
Admin
Well... null sex would indicate no sex whatever. Original sex would indicate not only sex, but multiple occurrences of it.
Admin
More than ever. If the record is no longer wanted why is it being updated? What do they know about the record that I don't?
Admin
I see this kind of code quite often when dealing with slow queries (usually from a generic search page or a generic search_employees function). Depending on database this will more or less randomly pick any index on the table (since you are searching on "that" column) or it will pick no index at all since none of the indexes are good enough. Build the query with 2-3 joins on some large tables and look at the nested loop full table scans eat all your IO capacity.
Admin
Yes, for the exact reason I mentioned previously.
Admin
MVCC FTFW
Admin
Has anyone ever heard of read serializable? Locks whatever is read until the transaction completes
Admin
Gee, it's sure great that someone prepended "tbl_" to all our table names, otherwise I might forget that they were tables!
With all those conditionals and parameters, I have a feeling that this is going to have wonderful parameter sniffing issues, notwithstanding whatever the developer was sniffing when he wrote this.
Admin
At a previous job, all table names were in the form TSOMETHING, and all views were VSOMETHINGELSE
Then some tables were replaced with others. For backwards compatibility reasons, the old table names still had to be addressable, so they were replaced by views with INSTEAD OF triggers so inserts, deletes and updates work on the new table. So now they have tables with the T prefix, views with the V prefix and views with the T prefix that when updated actually update other tables with the T prefix
Oh, and another job had hungarian notation on column names, so a tinyint column would be tisomething, a varchar(30) columnwould be sz30something. Lots of columns were changed to larger data types over the years so there were ti columns holding ints and sz30 columns for varchar(200). Fun times
Admin
When I read about that hungarian notation on column names I imediately thought: Oh my, then what about later changes to data types (extending varchars for instance) and yep, your next sentence justified my fears... :smile:
Admin
It makes sense if it's a bit of a toy project or something that's only expected to last a few years before being replaced, but this was a major database for a large multinational, and the product was 20+ years old. I was there in 2010, and it was using C++/MFC for the GUI.
Admin
Sure for a toy project it's ok, but then again, for a toy project why bother?
In the case if a small project (or toy project), in most cases a single (of few) developers willwork on it and hungarian notation will not be nescesarry as they know that column 'user' in table 'xxx' is in fact not the username but the userid istead.
For larger projects (or projects expected to last long), you are much better of by using descriptive comumn names wich by themselves implies what data type it is (in the former case by using 'userid' instead of 'user'. then userid's can be shortints, ints or even (shudder) varchars without confusing the future developers.
Admin
The real wtf is this is for deletes, I would asumme that the shop either uses row-level locks (which have to be cleaned up when they are not released in a short time) or that the shop also has a procedure that handles updates in the case a second user has modified the record between the time the first user has fetched the record and he has tried to save it.
I saw a much better solution (in Oracle) which also can be implemented in sql server 2012.
Create a single sequence in your schema. For every table in your schema, add a column (call this column ROWSTAMP) that stores the next value from your schema sequence. For each table in your schema, create a row-level trigger that fires on insert/update, that gets the next value from the sequence object and sets the ROWSTAMP using that value. All Update and delete statements must include a clause that says "and ROWSTAMP = @Original_ROWSTAMP' or something similer, this prevents updates/deletes from completing unless it is still the same starting record.
Admin
As I recall from my Oracle days, INSTEAD OF triggers have this endearing quality that they are silently dropped whenever the view is updated.
Admin
No, null sex would be undefined sex. It could be interesting, but OTOH you could wind up having sex with the big hairy person of your own gender.
Admin
Thanks for providing a real-life example of the end result of what Joe Celko calls Volkswagen coders.
Admin
Definitely not! If an employee is active, (s)he might have a status of 'A' or 'F' for full-time and 'P' for part-time. The same employee might one day have a status of 'T' for TERMINATED. As in, not active.
So, let's just say, you have an employee that leaves the company in October. If your HR database DELETES the employee because (s)he is no longer working there (meaning, of course, "Active") then how do you print W-2 information the following January? If this is what you do, please tell us where you'll be so we can visit you in prison.
Admin
Obviously, you do a diff with the backup from the last tax year, and see which employees aren't there any more