Who is responsible for fixing failed SQL code reviews? [closed]

Question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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.