1

Developers at my company have this habit of putting multiple semi-related functions within a stored procedure, calling the appropriate one based on a passed-in parameter, e.g.:

if @function = 1

begin
    truncate table holiday_list
end

if @function = 2

begin
    -- huge SQL statement populating the holiday_list table 
end

Is this a bad practice? It seems to me that SQL Server would not create a good execution plan, as the code branches into two disparate paths (this is a simple example, though many times there will be 4 or 5 such branches, doing unrelated tasks except for sharing the same base table).

  • 1
    dba.stackexchange.com/questions/9835/… Seems like some good info here. – Jacob H Jun 6 '17 at 18:41
  • Have you looked at the execution plan with and without the "if" functions? – Jacob H Jun 6 '17 at 18:42
  • 2
    Personally, even if the execution plans are perfect, from a code maintenance and "followability" point of view, I would not have complete disparate action (clear vs. populate a table) in the same procedure. (Especially if the difference between clear and populate from the caller is an argument being 1 or 2; a lot harder to follow what's happening if trying to debug the caller....) – RDFozz Jun 6 '17 at 19:10
  • I'm not clear on whether an execution plan is formed separately for each distinct statement in the procedure One or other statement will be executed, depending on the parameter. I believe there is a level at which any change in the procedure will cause all stored plans for its execution to be discarded, but that may be not so important unless the procedure is run a lot (say, several times an hour), which it may be. The big statements could be wrapped in EXEC(N'...') so that they exist as separate batches, to get around that. – Robert Carnegie Jun 7 '17 at 1:10
1

In summary, this is poor practice. While not "wrong" in an absolute sense it introduces avoidable risks.

SQL Server optimizes each statement separately. The execution plan for the "everything" stored procedure will not be noticeably different to the combined plans of having each bit in its own stored procedure (SP). There will be a very tiny overhead of evaluating the IFs, but you would never measure it, much less notice it.

It seems they're treating the SP as a plug-in library. SPs are not libraries. They should be treated as methods. Each will have its own clear, distinct purpose and implementation.

How would code review treat application code that had a class with a single mega-method? Would the devs accept this style in the UI or business tier? Likely not. Don't do it in the DB tier either.

There are some run-time implications. The mega-SP has a single plan. Changing any part of the code will cause the whole plan to be re-compiled, stalling all clients wishing to run any part of that plan, even if they're executing a function whose code has not changed. The time to compile a large SP is longer than the time to compile a small one. Plans have to be loaded in toto to be executed, even if only a small fraction of the code path is traversed. So fewer plans can be in memory at any one time, meaning plans get evicted from cache more often, causing re-comiles and client slowness. If the server has more memory than it needs and is never under memory pressure this may never manifest as an actual issue.

Project management becomes more complicated, too. With a single SP lots of changes will affect that one module risking code merge fails. (Your DB code is in source control, right?)

The code is less self-documenting. What does function "1" mean in this SP? Is it these same meaning in that SP? Should every SP have a function "1"?

The parameters must be a superset of all the functions. Now you need even more documentation to say "function 1 uses @a, @c and @f; function 2 uses @a and @g but don't populate @c or year-end crashes." Likely every parameter will be optional losing further built-in documentation and validation.

| improve this answer | |
0

I'd say while it may make sense and if the functions didn't cause explain plan issues it won't cause problems but not the best design to follow. Just because you can do something doesn't mean you really should. The possible repercussions of this design can bite you and could be a pain to hunt down the issue.

| improve this answer | |

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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