May 8, 2017 at 4:57 am
jonathan.crawford - Friday, May 5, 2017 12:49 PMBrandie Tarvin - Friday, May 5, 2017 10:18 AMIn my workplace, we use DROP (after existence check) and then CREATE. Never ALTER except for tables and columns.The reason being is we had a spate of no-longer-working-for-us "devs" who would push through ALTERs on procs that created problems months down the road. We couldn't tie the ALTERs to a specific release since SQL doesn't track that kind of modification automatically. So the standard was created that it must be a DROP-CREATE so that we could look at the CREATE date of a proc and know exactly when it got changed. Documentation (which is supposed to be in every function, view, and proc) isn't consistent or always done. The "created on" date of the object gives us the reference we need when six months to a year later that once-in-a-blue-moon problem crops up.
People actually got fired for the stuff that prompted this particular standard. One got fired the day after he pushed an ALTER PROC which was missing a WHERE clause, which also prompted peer-reviews of code before they go into test/qa. "DELETE FROM Table". WHOOPS.
...aaand, now I'm paranoid. Thanks!
You're very welcome. @=)
May 8, 2017 at 5:02 am
Jeff Moden - Friday, May 5, 2017 9:29 PMAs a bit of a sidebar, we're religious about revision history in the header of all code objects. You can say what you want about version control tools, etc, etc, but having the revision history in the object header has been a huge lifesaver that has saved on much time and confusion. I'll also say that the auditors of all types absolutely love it because we also include the "ticket #" and description of the ticket. It also makes a search of the database for everything affected by one or more tickets a snap.If you've ever had upper levels of management decide they wanted to change whatever version control you've had to something else, you'll find that revision history in the header is a lifesaver.
We started doing this a few years ago. Actually, we started requiring it a few years ago because some people are allergic to documentation. It got really bad when a third party contracted Dev outfit was creating code for a project we'd hired them for. They seemed not to understand what documentation was and would put the proc title in the version comments. Plus other ridiculous stuff. "Definition: Procname. Version History: Gets data. Fixed Procname. Add column". It was such a pain pushing back against them to get more detail put in those comments.
Again, though, not everyone remembers to add that version note and if a peer review doesn't catch it, at least we have the CREATE date of the proc to refer back to.
May 8, 2017 at 7:18 pm
Brandie Tarvin - Friday, May 5, 2017 10:18 AMPeople actually got fired for the stuff that prompted this particular standard. One got fired the day after he pushed an ALTER PROC which was missing a WHERE clause, which also prompted peer-reviews of code before they go into test/qa. "DELETE FROM Table". WHOOPS.
I don't see how DROP/CREATE with a missing WHERE clause would posibly change the course of events.
Can you explain the advantage of your policy in more details?
because I don't see any at all.
_____________
Code for TallyGenerator
May 9, 2017 at 5:23 am
Sergiy - Monday, May 8, 2017 7:18 PMBrandie Tarvin - Friday, May 5, 2017 10:18 AMPeople actually got fired for the stuff that prompted this particular standard. One got fired the day after he pushed an ALTER PROC which was missing a WHERE clause, which also prompted peer-reviews of code before they go into test/qa. "DELETE FROM Table". WHOOPS.I don't see how DROP/CREATE with a missing WHERE clause would posibly change the course of events.
Can you explain the advantage of your policy in more details?
because I don't see any at all.
I believe she's saying that a DELETE FROM without a WHERE clause prompted peer reviews of code.
Whether or not in was a DROP/CREATE or ALTER is (IMHO) immaterial; the peer reviews was the result of the error.
May 9, 2017 at 5:26 am
Luis Cazares - Friday, May 5, 2017 8:28 AMBut you shouldn't DROP and CREATE tables. That will eventually cause data loss.
Depends on what the table is used for. If it's used to stage data for some process and is truncated each run, Drop/Create is perfectly fine.
When I write a script for such a table, I put a note in the header comments: "-- Drop/Create is OK because the table is truncated each run."
May 9, 2017 at 10:27 pm
Ed Wagner - Tuesday, May 9, 2017 5:23 AMSergiy - Monday, May 8, 2017 7:18 PMBrandie Tarvin - Friday, May 5, 2017 10:18 AMPeople actually got fired for the stuff that prompted this particular standard. One got fired the day after he pushed an ALTER PROC which was missing a WHERE clause, which also prompted peer-reviews of code before they go into test/qa. "DELETE FROM Table". WHOOPS.I don't see how DROP/CREATE with a missing WHERE clause would posibly change the course of events.
Can you explain the advantage of your policy in more details?
because I don't see any at all.I believe she's saying that a DELETE FROM without a WHERE clause prompted peer reviews of code.
Whether or not in was a DROP/CREATE or ALTER is (IMHO) immaterial; the peer reviews was the result of the error.
I believe she was talking about this:
The reason being is we had a spate of no-longer-working-for-us "devs" who would push through ALTERs on procs that created problems months down the road. We couldn't tie the ALTERs to a specific release
The only reason I can see here is to help with finger-pointing after untested code destroyed the database.
I do not see how DROP/CREATE can help prevent such occasions.
But honestly, if that kind of code can make its way into Prod database, than there must be no any kind of testing in place whatsoever.
With this kind of practices in place - I don't think any coding standards can be really helpful.
Well, only for a blame game maybe.
_____________
Code for TallyGenerator
May 10, 2017 at 5:27 am
Ed Wagner - Tuesday, May 9, 2017 5:23 AMSergiy - Monday, May 8, 2017 7:18 PMBrandie Tarvin - Friday, May 5, 2017 10:18 AMPeople actually got fired for the stuff that prompted this particular standard. One got fired the day after he pushed an ALTER PROC which was missing a WHERE clause, which also prompted peer-reviews of code before they go into test/qa. "DELETE FROM Table". WHOOPS.I don't see how DROP/CREATE with a missing WHERE clause would posibly change the course of events.
Can you explain the advantage of your policy in more details?
because I don't see any at all.I believe she's saying that a DELETE FROM without a WHERE clause prompted peer reviews of code.
Whether or not in was a DROP/CREATE or ALTER is (IMHO) immaterial; the peer reviews was the result of the error.
The issue was within the Procedure that someone was ALTERing, a Dev wrote a DELETE statement without a WHERE clause. The only reason we were able to trace the proc to a release was this happened to a very important (used daily) table that was supposed to consistently have data in it. The DELETE was a "let's delete today's data so we can reload it fresh if this proc gets re-run on the same day more than once" kind of thing. So when all the data got lost, we noticed and went back through every change that happened the evening before. We found the proc and the Dev who created it. He got the boot and we used this as a reason to hard-line implement the database standards & policies that we had been wanting to implement but didn't have manager buy-in before this point.
So both the peer review and the CREATE / DROP policies (among others) came out of this incident along with other past incidents we could point to.
May 10, 2017 at 5:39 am
Sergiy - Tuesday, May 9, 2017 10:27 PMI believe she was talking about this:The reason being is we had a spate of no-longer-working-for-us "devs" who would push through ALTERs on procs that created problems months down the road. We couldn't tie the ALTERs to a specific release
The only reason I can see here is to help with finger-pointing after untested code destroyed the database.
I do not see how DROP/CREATE can help prevent such occasions.But honestly, if that kind of code can make its way into Prod database, than there must be no any kind of testing in place whatsoever.
With this kind of practices in place - I don't think any coding standards can be really helpful.
Well, only for a blame game maybe.
It wasn't about a blame game. It was about people taking responsibility (or failing to in certain cases) for the work they did. We didn't have an official test team at the time (getting one was another outgrowth of all this). Certain Devs had achieved a "status quo" kind of attitude about their work or got easily distracted by one or two items on the requirements and this wasn't the first time a forgotten WHERE clause query made it to production without the business unit testers noticing.
So the hammer came down. The Devs and DBAs (including myself) were expected to code it right "the first time" meaning before the code made it to test. The DBA team made the list of database standards to adhere to, IT management instituted peer reviews, and we all had to unit test the heck out of our code before saying it was ready to move. Plus we officially got our non-BU test team started up with professional testers. All together, it made a world of difference in our coding. Our emergency bug-fixes went down to maybe 5 a year instead of the 2-or-3-after-every-release we'd been used to. And even my 5 a year estimate might be too high now.
May 10, 2017 at 6:05 am
Brandie, that really sounds like a great outcome. Someone had released code to production that shouldn't have been released, but that does happen. The great part is that everyone learned from it. The company put policies in place, established coding standard and peer reviews. They created a test team and testing procedures. That sounds like a great reaction to an unfortunate event.
May 10, 2017 at 6:45 am
Yeah. It was nice that not only was IT able to come to the same place, but we were able to convince the BU management teams that this was for their benefit. They didn't want to believe it, but once they started seeing results almost everyone jumped on board.
There are still some business people annoyed they can't have their thing put into production right after they ask for it, but they are the minority fortunately and not in charge (double-fortunately).
Viewing 10 posts - 16 through 24 (of 24 total)
You must be logged in to reply to this topic. Login to reply