11

I mean everything, not just schema changes. Even a simple SELECT on a primary key can't go into production, even though it has been code-reviewed by other developers (in context), without a DBA review of every statement, extracted from the code and submitted with EXPLAIN output, details of how often it will be called, etc, etc.

If you've been in such an environment, did you find it to be a net gain, or a drag on development? As someone who has been working with relational databases for years, I find the "developers can't be trusted to use databases" attitude unwarranted, and I'm curious as to how common this situation is. For what it's worth, this is just for web development, not something critical.

Improve this question
7
  • 2
    Well, we don't put SQL statements in the code. However, ALL code should be reviewed by appropriate knowledgeable individuals.
    – CaffGeek
    Jul 23, 2012 at 15:11
  • 4
    Web development is critical. The result is directly facing the customer (who is your king), and more often than not, sensitive data are accessible to the web server.
    – thiton
    Jul 23, 2012 at 15:21
  • 2
    The question is, if not every query is passed by a DBA, how do you define what should and shouldn't be passed by the DBA?
    – programmer
    Jul 23, 2012 at 16:10
  • 6
    @thiton: That's a blanket statement assuming that ALL web applications are public customer-facing and are the most important applications in the organization. That's simply not true. Sometimes web applications are third-tier or lower (compared to other applications). Some are used only by a small, internal teams. Without knowing what @ DanEllis is working on we really don't know how critical this web development work is.
    – FrustratedWithFormsDesigner
    Jul 23, 2012 at 17:26
  • 2
    You've not been bitten yet? Consider asking about why this procedure is in place...
    – user1249
    Jul 23, 2012 at 20:18

10 Answers 10

Reset to default
15

At one of my previous jobs I (along with three other programmers) were responsible for reviewing every stored procedure created or updated before it went into production for about 40+ programmers. As a reviewer it was a real drag on my time but overall it was still needed because with that many developers will make a mistake once in a while. It came about because we had off-shore programmers who wrote really bad (efficient wise) SQL statements and they flat-out refused to learn (that team was eventually shut down).

Overall we looked for:

  • Index usage - was the query doing a full table scan?
  • Maintainability - was the query going to need to be changed in a couple of weeks because something was hard-coded into it? And is the query readable?
  • Overall efficiency - could a inner join be replaced with an exists? Would adding a With block help?
  • Locking issues - would the query lock the database and prevent updates from happening? This happened more than I care to think about.

More often than not most queries had no issues and we pushed it on. But each review took between 10 minutes and two hours depending on the query. Part of me liked the idea of doing reviews because it prevented the dreaded 2 AM phone call because some report was causing an issue with another application. But at the same time I thought it was a bit overkill because we are all grown-ups and the person writing the query should do all that stuff themselves.

I think complex queries should be part of the standard code review, but doesn't need to go through a DBA. Nor is reviewing simple insert/update/delete statements is not needed. In addition, as a writer of a query you should know when it has gotten too complex and know when to ask for a review from a DBA.

Update In my very first job all queries were written by DBAs and that was a major time killer on development. I had to submit all the columns I wanted, tables where the columns were located and finally my search criteria. Even basic insert/update/delete statements needed to go through a DBA. Eventually I got to the point where I could write the SQL I wanted, have them take a look at it and make any necessary changes, and then wrap a stored procedure around it, but that was only have a couple of years there. That particular company hired a lot of recent college grads and this was the way they ensured the new guys didn't write queries that killed the performance.

Improve this answer
3
  • -1 A great answer but unfortunately the question was about review by a dba after fellow programmer review. This answer is really to the question "should all the code be reviewed" but that was not the question asked. I don't downvote much or out of spite, only when they answer isn't an answer to the question.
    – Michael Durrant
    Jul 23, 2012 at 15:46
  • Exactly. This is code that has already been reviewed in context. That's important. A query in isolation might look innocuous, but put it inside a loop and suddenly it's less so.
    – Isvara
    Jul 23, 2012 at 15:52
  • 1
    Are there any tools to automate this sort of thing? I'm thinking of a tool that creates an execution plan for all submitted queries and then flags anything with full table scans or Cartesian product. I doubt it could catch everything, but it would probably speed up review time and it could also be run by developers through a script, or even better if run automatically at code check-in time.
    – FrustratedWithFormsDesigner
    Jul 23, 2012 at 16:03
10

It totally depends on the environment, the industry and the company.

Some examples:

  • At NASA, multiple levels of checking and reviewing are the standard. The most well-known "should have doubled-checked" was when a probe was sent to Mars and the US did calculations in feet, but the Europeans in meters. The probe was lost.

  • At a startup with no DBA (common these days) there will be no DBA review and possibly even no programmer review (for example, if there are no other programmers in the company).

  • At a company whose product is a data warehouse of hundreds of millions of rows, making sure that queries are efficient and correct may be key issues at the core of the business that should be checked.

A company whose main product is a mainly static web site with a small number of database interactions, may consider the database and its efficiency to be less relevant. It may be that the company chooses to spend it's review "budget" on the static pages that are considered most critical.

Improve this answer
5

I've never worked in an environment like that, I'm sure I would hate it. A thought comes to mind to start keeping track of some metrics around this...

  • How much time does a dev spend pulling sql out of code and preparing it to be submitted to a DBA
  • How long does the DBA take to review the sql?
  • How often are changes requested by the DBA? Broken down by query
    type?

Keeping track of this for just a little while would likely show how much of a drag it is versus how effective it is.

Improve this answer
2
  • 6
    These are excellent things to measure. While you're at it, how much time is spent fixing broken code that was allowed into production? How many hours does it take Sales and Marketing to find customers to replace those lost customers while your site was nonfunctional because a missed WHERE clause caused repeated table scans? Code review has costs and benefits, don't neglect either.
    – user25946
    Jul 23, 2012 at 15:58
  • 4
    That's why it's important to know how many times changes are requested/needed by the DBA. The question explicitly says that everything gets reviewed, no exceptions. It's this kind of broad policy that usually has more costs than benefits. I'm a huge proponent of code review and testing but it doesn't mean that every line gets reviewed or tested. We're supposed to be professionals and there's a difference between quality control and baby sitting.
    – BZink
    Jul 23, 2012 at 17:38
4

Most developers are flat out ignorant when it comes to databases. They are not even aware of their ignorance. I used to be an ignorant developer myself.

Developers live in an optimization-is-evil world. That mindset doesn't work when you are performing operations against a disk. That mind set doesn't work when you choose a datatype that's 8 times bigger than it has to be which causes the index to be 8 times bigger than it has to be so it can no longer fit into memory. That mindset doesn't work when you program against a generic API that redundantly updates all columns in a table (no thanks Java beans).

They don't know what a full table scan is. They don't know how to process data in a single set operation instead of opening a cursor. Worse than a cursor they will query data, then row-by-row process it going back and forth to the database when a single simple plain-jane update would suffice. Resulting in HORRIBLE performance.

They don't know the serious impacts of reporting against large tables. Maybe the need for reports will require creating a star schema and a data feed into it. Your average developer will just query the exisitng tables and wonder why it takes 3 hours for the query to run (this is a real figure).

The knowledge your average developer has will suffice for "average" CRUD apps where a user simply edits a record. It will not suffice once they break out of the Ruby-on-Crack bubble and into the real world.

Improve this answer
2
  • Would you like to sound any more holier-than-thou?
    – sevenseacat
    Jul 24, 2012 at 4:52
  • 2
    @Karpie, at least (s)he took the time to respond drawing from his own experience, rather than making a worthless, sarcastic comment on someone else's answer like Karpie did.
    – Drew
    Jul 27, 2012 at 0:58
2

Depends on how big the database is, and if it's shared among multiple applications. Often the DBA team likes to know what queries will be run so they can ensure that the proper indexes are in place, or so they know who to notify if they have to partition a table. And they do tend to be a bit better than the average developer at reading query execution plans and spotting places where hints could be specified to improve performance. It's also a chance for them to get a heads-up that some high-impact queries are going to be coming down in the next release, so they can collect different statistics for the optimizer.

Improve this answer
2

I have not worked in such an environment, nor would I.

There is a difference between teamwork and hostile bureaucracy. As a developer, I'd love to have DBA input on difficult queries. But I know when to ask for help.

If I'm not competent enough for simple SELECT queries, I should be fired.

Improve this answer
5
  • 1
    Whilst that's a reasonable viewpoint you have to allow that we're very few of us quite as smart as we'd like to think we are and the worst offenders are the most ignorant (almost by definition not those hereabouts) so the way you avoid the issue is by having a blanket rule - everyone treat equally
    – Murph
    Jul 23, 2012 at 20:29
  • 2
    @Murph - absolutely. I could make a mistake. But I could also take 2-hour bathroom breaks every day. Should everyone have to track their bathroom time to prevent that? Should we have to sign forms to get paperclips so we avoid paperclip theft? At some point, you have to either trust people or fire them. My slow query has a cost, but so does a soul-sucking, time-consuming review process. Only if small database performance differences would cost millions (or lives) would I accept this kind of thing.
    – Nathan Long
    Jul 23, 2012 at 20:34
  • 1
    The problem with this is that you're likely not the average developer; and very likely not the problem developer. I've worked at a place like this, and it was kinda sucky; but you know what? Our DBAs were the ones that got the call in the middle of the night when queries broke, not me. If they're the ones being held responsible, they've the right to review the code they're responsible for.
    – Telastyn
    Jul 23, 2012 at 20:38
  • @Telastyn - the "not average developer" I don't buy; I still say don't hire bad developers. But yeah, if the DBAs have to get the call, I sympathize a bit more.
    – Nathan Long
    Jul 23, 2012 at 21:11
  • @NathanLong its about context - in the contexts in which you work its presumably not an issue, in others is clearly it has the potential to be and one of the ways you avoid certain problems is by having what can seem to be mindless rules (though, like the fact that I always use {} in my if statements, its done to a purpose). Its bad "because I don't like it and I think I'm safe so it shouldn't apply to me" is a questionable argument (the same logic is applied to ignoring speed limits). Hmm, that's argumentative )-: Sorry.
    – Murph
    Jul 24, 2012 at 9:33
2

I've seen this done at a couple of different places. It's great in theory, but I have rarely seen it be effective. Here's why. Firstly, if you have a team of DBAs (or other people), I've generally found that the least competent or least liked person of the group gets the brunt of the review work. Why, you say? It's because, no one else wants to do it, and everyone else is busy doing other things that are probably more urgent. Ever seen a DBA sit around and say, "Man everything is running perfect; I can just sit back and surf the net. I wish I had something to do." Me either, at least not the good ones. They are as busy or busier than everyone else. This means that the person who is least capable is most likely doing the reviewing, and this is exactly the person you don't want doing it. The code you want reviewed is the really really hard code that people look at and pass it off as some sort of black magic. Junior DBAs or just plain bad ones, won't ever be able to catch the subtleties of how a really hard query works. Rarely, like never, does someone ever say, "Man I didn't think of selecting a single row out of a table using the primary key! Thanks DBA you're a lifesaver." So in this scenario, really all you are doing is creating a lot of work for little value.

Secondly, it's just plain more work for the DB group. What is probably going to happen even if they do look at other things is they are going to take a quick look at it and something is going to get missed. They are busy people, and reviewing code is really time consuming. In truth it's not fair that they get tasked with this, because it's an excuse for everyone else to be lazy and use them as an out, which is ultimately what happens. Something breaks in production, and the developer quickly points out, "Well the DBA reviewed it." Now is this true all the time, no, but it is true part of the time and often from the people who need to have their code really reviewed. So you have buried the DBA with extra work and forced that person to be responsible for someone else's mistakes, when that person probably doesn't have enough time to fix all the mistakes currently in production.

The only way to really solve the problem is to have people who know how to write SQL code write it. Should they get input from the DBAs from time to time? Of course they should, but I have always found, if you don't have time to do it right the first time, when are you going to find time to fix it.

Improve this answer
2

I've worked in that sort of environment. All system changes were peer reviewed by appropriate peers. For me it just meant I had to plan ahead and submit my changes a few days in advance. SQL went to the DBAs who would ultimately release the SQL into production.

My SQL is usually fine, they caught a couple of silly errors. It feels overly bureaucratic at first, but I work in finance. You saw what happens (If you're in the UK) a few months ago when a major bank here messed up a routine upgrade and messed up all all the transactions leading to millions (At the least hundreds of thousands) of people unable to get at their money.

Improve this answer
0

I would even go further. I'd check the surrounding code and see how are variables passed to the query. It's often to see how developers expose their application to sql injection attacks due to their lack of basic knowledge ("this can't be hacked" false assumptions).

Also unexperienced developers may drag the database down to its knees with a misuse of ORM or a query that is far from optimal.

In the most recent project I work at, none front-end developer is allowed to access database directly. They raise a demand for a function returning a given set of data and the back-end developers prepare it for them. It's working quite well, front-end devs write UI and process data supplied by functions written by back-end devs.

Improve this answer
0

In our development team there is a sub-team of 4 Database Developers experienced in the DBMS platform we use. They write all SQL or at least reviews and make changes to meet their coding standards any SQL written by .Net developers. They are part of project team. The production DBAs belong to a completely different department and their job is operational. They do not review our SQL code or schema and even deployment is scripted,all they do is run the script. The most they help is in performance tuning.

Improve this answer

Not the answer you're looking for? Browse other questions tagged or ask your own question.