Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

As a DBA, most SQL is submitted to my team for review. We do not have a SQL developer, so the code is frequently very, very inefficient.

Our current process is:

  1. SQL is submitted for review right before it goes to beta
  2. Issues are pointed out but frequently not resolved because the application has already been written around it, and is scheduled to be deployed to beta the same week
  3. If the DBA team identifies an issue, the developer response is "show me how to fix it, then"

It should be noted that the DBA team has no requirements around any code, no context and no idea what 1000s of lines of a stored procedure is supposed to be doing.

Is this a typical scenario? Who is responsible for fixing the code? Does your DBA typically re-write the stored procdedure for you?

share|improve this question
7  
1. SQL is submitted for review right before it goes to beta 2. Issues are pointed out but frequently not resolved because the application has already been written around it, and is scheduled to be deployed to beta the same week Is it not fairly obvious what your problem is then? –  pdr Apr 16 '13 at 15:33
3  
Since your dev team does not get anything from those SQL code reviews, why are you doing them? Seems like a lot of wasted time. ;-) –  Doc Brown Apr 16 '13 at 15:54
1  
Clearly, your sql code reviews are done wrong, as the code is getting checked in (otherwise, how would it be 'written around') before the review is approved. Don't allow checkins without an approved review. –  Steve Evers Apr 16 '13 at 15:54
    
@Amanda: As there are multiple meanings for the acronym DBA and the distinction is relevant, can you clarify in your question with what meaning you use DBA? –  Bart van Ingen Schenau Apr 17 '13 at 10:54
add comment

closed as too localized by Robert Harvey, Jimmy Hoffa, Kilian Foth, thorsten müller, World Engineer Apr 17 '13 at 14:20

This question is unlikely to help any future visitors; it is only relevant to a small geographic area, a specific moment in time, or an extraordinarily narrow situation that is not generally applicable to the worldwide audience of the internet. For help making this question more broadly applicable, visit the help center.If this question can be reworded to fit the rules in the help center, please edit the question.

5 Answers

This review is happening way too late in the process. These reviews should happen as a dev finishes a piece of work, not before the actual release.

There must be a common pattern that you are seeing eg. incorrect indexes, using cursors etc. Maybe you should put together a crib sheet of what you will reject, and what you would replace it by.

For what it's worth, it sounds like the devs are putting way too much business logic in the database.

share|improve this answer
    
ha, yeah, they are. we've sent them basic SQL best practices which are essentially ignored. I'm a production DBA, but I can read the code, but if i don't know the requirements and the application is already built there isn't much i can do. I have suggested to management we no longer do "code reviews", but i wanted some developer input, just in case i have the responsibilities confused. –  Amanda Apr 16 '13 at 16:41
    
A random minus, at least tell us what u disagreed with! –  Ozz Apr 16 '13 at 17:46
add comment

There is constant confusion about this and it's caused by the fact that there are two different types of DBA: DataBase Architect and DataBase Administrator.

Many developers work with a database Architect who will design the database structures and write stored procedures. Database Administrators however often live on IT teams doing backups, knowing nothing of coding or engineering practices.

It sounds like your devs expect you to be the Architect, where you are an Administrator. I think you need to fix this expectation with them and then you won't have these problems. Also, if you are just an administrator who doesn't understand the code or engineering that went into designing the structures and writing the stored procedures, it's entirely likely the developers do not take your critiques of their work seriously. If you have suggestions or issues with the engineering work, it may be best if you can get a database Architect with more engineering know-how to champion those to the developers for you.

Also, I have to question why you are reviewing the SQL the developers write if you aren't an engineer familiar with reading complex stored procedures. It sounds like management may be trying to get their IT department to do what their engineering department does, and not understanding that they are completely different skillsets. You may wish to raise this with management.

share|improve this answer
    
on the other hand, this DBA IS involved in reviewing the sql code. –  Ozz Apr 16 '13 at 15:49
1  
@Ozz which I would say shouldn't be occurring if it's an administrator. She said herself she doesn't understand the stored procedures the engineers write, so why is she reviewing them? –  Jimmy Hoffa Apr 16 '13 at 15:51
    
they might not understand the context, but they'll sure be able to spot poor sql practices. It's quite common in large companies, but I'd expect something more formal in place as part of the process, not just an after thought. –  Ozz Apr 16 '13 at 15:56
add comment

DBA as Architect:

  • If the DBA team is rejecting code, then yes the DBA team should be giving the reason why they are rejecting the code.
  • If the DBA team is rejecting code because it is inefficient, the DBA team should probably re-write the code to be efficient, and return it to developers. Developers need to validate that the code you re-wrote still passes their tests.
  • If the DBA team is rejecting code because it incorrectly implements requirements, then the DBA team should actually be writing the code themselves. If they know the requirements, then it makes far more sense to have SQL experts writing SQL code to some API agreed to by the DBA team and developer team.

DBA as Administrator:

  • The DBA team should not be tasked with code review. Such a team would likely not have the skills to effectively review code, and are rather just a "rubber stamp" approval that feels good to management but doesn't actually improve the process.
share|improve this answer
add comment

You can only make changes in the context of what information and skills you have. Since you don't have the requirements, you have to assume the outcomes of the existing code are correct. Any changes you make for performance should not change the results. This is easy to test.

You may be able to make changes to improve the code without changing the code itself. Queries can be analyzed to see if an index would improve the performance. Other database settings or hardware changes can be made as well.

To a certain extent, your review process is almost asking the DBAs for a rubber-stamp of approval. If you want to do more than this, you need to request the requirements, get involved in development, and request your approval be made earlier in the development process.

share|improve this answer
add comment

Generally, when a DBA has a problem with a query it's because it is slow. In that regard, it is up to the programmer to fix the query, because they actually know what the query is supposed to be doing, are part of the normal application development workflow, and will likely be responsible for changes in the future; however, it is very likely that the DBA will have to assist in the optimization process. Very few programmers I know can reliably look at a query and a query plan and figure out how to remove the bottlenecks.

share|improve this answer
add comment

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