Every SQL statement has to be reviewed by a DBA — common?

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

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.

Filed under: softwareengineering - @ 14:22

Thẻ: code-reviews, dba

Thiết kế website giá rẻ

Danh mục

Every SQL statement has to be reviewed by a DBA — common?