Best Practice for Oracle Code Review

Posted on 2011-09-29
Last Modified: 2012-08-29
Would like to hear from gurus, what should be the starting point or can say; what are the best practices for a code review, this is in reference of Oracle 11gR2 on Solaris box, having single and multiple instances too (RAC).

Code can either be a simple, but pretty long Sql statement (recursive joins & a lot more) or a 1000 lines of a pl/sql package, having many procedures & functions.

Thoughts, suggestions, advises are welcome.
Question by:js4oracle
  • 7
  • 6
LVL 73

Accepted Solution

sdstuber earned 250 total points
ID: 36817195
pl/sql is pretty much no different than any other language.

examine the exposed vs private components.
look at each

same as you would do with a java class or file or any other source code.

sql can be a bit different though since it's a single statement.
however,  if it's a query built on WITH clause factoring (aka common table expressions)  or inline views.  You can think of them sort of like subroutines
and explore them as entities in themselves.

similarly you can look at joins in sections.

if your query is a single table query that just happens to be large, then it's really not all that complicated, it just has a lot of keystrokes.

go through the filtering conditions, look for index usage and any function calls
it would be good to include an explain plan as well

LVL 15

Assisted Solution

by:Devinder Singh Virdi
Devinder Singh Virdi earned 125 total points
ID: 36817938
1. Avoid cursors,
2. Avoid nested loops and IF conditions. It is possible that you can write sql in such a way using decode, rownumber() over() function etc to avoid this.
3. Use bind variables.
4. look for more than 1 plan_hash_value in dba_hist_sql_plan and try to find the cause and best plan.
5. Look for any missing FK.
6. Look if your code can be written to to use truncate instead of delete.
7. Regularly generate AWR reports for long running sql and identify its source.
8. look for any blocking locks.
9. look for tables/indexes with low initrans, extent size etc, which can create performance level issues.
10. Check if code is using parallel hint or any other hint and justify if they are used properly.
LVL 73

Assisted Solution

sdstuber earned 250 total points
ID: 36818003
yikes, I would avoid a "check list" approach, especially that list.

1 - no - cursors are not good or bad.  Like any code construct, if used improperly it's bad, the syntax itself is not bad
2 - no - nested loops and if's are not bad either.
3 - yes, usually best practice.  However, if working with DW, maybe not
4 - sure - tuning is good
5 - yes but that's not really a "code" review
6 - the two aren't interchangeable, but "if" appropriate, sure
7 - only if you are licensed to do so
8 - you won't see that in a code review
9 - these aren't "code", but more importantly structural elements are the last place to look for tuning
10 - try to avoid hints, they are crutch that is rarely needed.
Windows Server 2016: All you need to know

Learn about Hyper-V features that increase functionality and usability of Microsoft Windows Server 2016. Also, throughout this eBook, you’ll find some basic PowerShell examples that will help you leverage the scripts in your environments!

LVL 15

Expert Comment

by:Devinder Singh Virdi
ID: 36819200
I would like to explore more about point 1 and 2.
Many Developers I saw in my current company is writing PL block to open a cursor, use IF condition to just insert records based on certain condition, where we dont have to open cursor.
When it comes to me, I can just rewrite a query that is using Decode.
Here is there sample code

  cursor x is select * from table1 where condition=1;
  rec x%type;
  open x;
    fetch x into rec;
    exit when rec%notfound;
    if rec.filed = trunc(sysdate - 10) then
      new_dt:= trunc(sysdate) - 20;
    end if;
   insert into new_tab values (rec1.v1, new_Dt,...);
  end loop;

In this case, we are calling insert multiple time. This insert will go with many HARD parse and put burden on Oracle.
Again we need to see if we really need nested loop or not. In above case we even dont want any loop.

Whenever you are doing code review, send queries to DBA for approval so that they will see many things. For you it may looks good, ie. query is using index, but for DBA its not, therefore overall result is not good.

LVL 73

Expert Comment

ID: 36841848
yes, using a cursor when you don't need to is bad.  
but that doesn't mean cursors should be avoided.
using loops that aren't needed is bad, but that doesn't mean loops are bad.

it just means inefficient coding should be avoided

by the same logic you could say "avoid SQL"  because it's possible to write a bad query
LVL 15

Expert Comment

by:Devinder Singh Virdi
ID: 36889926
>> by the same logic you could say "avoid SQL"  because it's possible to write a bad query
Exactly, that what I mean Avoid Cursor,

>> yikes, I would avoid a "check list" approach, especially that list.
In your previous comments, you agree in almost all points, then why you want to avoid this check list.
If you dont have list, how will you ensure that you have reviewed the code as per your best knowledge?

As per my knowledge, code review is for two purpose, business requirement and efficiency. Therefore point 5 and 9 are valid. Many jobs are creating temp tables that are used in Sql and because of missing indexes or lack of parameters give bad performance. It happens many time that temp tables were created, modified by jobs, dml are performed, but stats were not collected on these staging/intermediate tables, and therefore code started providing bad execution plan.

Reviewing may also need to check performance and therefore if performance of code is not good, then you may need DBA help. Many times, blocking locks were the issue of slowness. It may be possible that blocking locks are due to contention, and therefore point 8 is also valid.
LVL 73

Expert Comment

ID: 36891172
>>> Exactly, that what I mean Avoid Cursor,
but that's not what you said "Avoid cursors"  does not imply  "avoid except when you shouldn't"  avoid means avoid.

I avoid a check list like this, especially one with absolutes, because the parts that I agree with basically boil down to "make sure the code does good things"
and as they were originally written they aren't good advice,  they need caveats and exceptions to make it clear when to follow and when not to.

you could reverse many of the elements in the check list and they'd still be valid as long as you put proper caveats on them
maybe you "meant" to include exceptions, but the list "as written" doesn't

1 - try to use cursors (except when they aren't appropriate)
2 - make sure you are using loops and conditionals to capture all business logic appropriately
3 - don't use bind variables (unless parsing costs are an issue)
6 - make sure you are using delete instead of truncate to avoid implicit commits  (unless that would be a good thing)
7 - don't use AWR or ADDM because they are separately licensed  - also,  if the code is already running then somebody skipped the review process anyway.
10 - remove all hints (except for testing purposes)

so the point I'm trying to make is, code reviews aren't simply a matter of do-this, don't-do-that.  
If it was that simple there would be automated tools built into the compiler to fix them.
If the code produces correct results and efficiently then it should pass the review.
Anything more specific than that is overly simplistic.

rather than a check list of practices,  I suggest working on a check list of things to "examine"
with no recommendation as to what should or should not be there

1 - check cursors
2 - check loops
3 - check variables
4 - check execution plans
5 - check underlying structures (this would combine your previous 5 & 9)
6 - check all commits and rollbacks, both explicit and implicit through ddl
7 - check adherence to company coding standards

these aren't strictly code review, but if available from any initial testing could be useful

8 - check results, locks, profiler output, tracing and "maybe" you could include AWR here, but only if licensed

LVL 15

Expert Comment

by:Devinder Singh Virdi
ID: 36891856

I have given an example where you should avoid. If cursor are very bad, then i would preferred to write Always avoid cursor.

>>as they were originally written they aren't good advice,
Neighter you have code, nor I and many things which is decided is based on code.

>> don't use bind variables (unless parsing costs are an issue)
Which cost you preferred HARD or SOFT?

>>make sure you are using delete instead of truncate to avoid implicit commits  (unless that would be a good thing),
Issue I am talking about more redo more time and more resources.

>>don't use AWR or ADDM because they are separately licensed  - also,  if the code is already running then somebody skipped the review process anyway.
Why, if you have licence. Here neither nor I know if user has valid license.

>>remove all hints (except for testing purposes)
I have a batch job, where we need to provide hint for faster access.
I have a situation where oracle is selecting nested loop on very long tables used by job, it took nearly 1 day to fininsh, if we use parallel with full, job finishes in 2 hours. As per our requirement we need to decide parallel degree. Here your example fails.

Again for developer point of view or with junior DBA,
1 - check cursors: he will check syntax and will not think to convert big plblock to one sql.
2 - check loops: For them loop is required, check means if loop required. We need to specify that avoid loops if not required.
3 - check variables: Rather say do you really need variables
4 - check execution plans: Here we need DBA assistance, because for them joining two tables with PK is ALWAYS okay.
5 - check underlying structures (this would combine your previous 5 & 9): Developer dont know what to check, what is TM and TX contention, what are the parameters in table to see
6 - check all commits and rollbacks, both explicit and implicit through ddl.
7 - check adherence to company coding standards

LVL 73

Expert Comment

ID: 36892245

My advice is to try to help both the asker and YOU too.

your first suggestion consisted solely of 2 words "avoid cursors"
I corrected that with caveats and it seems like you are agreeing with everything I've said.

All of your followup comments are the same thing I've said - there is no reason to get angry.  We're in agreement!

For example....

>>>> don't use bind variables (unless parsing costs are an issue)
>>Which cost you preferred HARD or SOFT?

this looks like you are trying to argue with me.  But why?
my caveat "unless parsing costs are an issue"  is the same thing you're saying with "HARD or SOFT"  

IF parsing costs are an issue, it's because you are spending time parsing statements when your cpu/memory/latches/etc could be used executing the sql.

However, I'm working with multiple datawarehouses.  SQL query reuse is not common and queries often run for very long time.  Parsing is not an issue this environment.  However,  you also know I work with OLTP systems where sql query reuse is very common and in those systems parsing costs add up if you don't use binds.   So "use binds" is BAD advice for my first set of projects but is GOOD advice for my second set of projects.

Therefore,  my suggestion is to say "check variables"  because we don't know which environment the asker or future readers will be working in.

All of your advice is good IF you make certain assumptions about the asker's environment and code.   If those assumptions are wrong, then the suggestions fall apart.

All of my corrections/exceptions/caveats above are simply clarifying the assumptions and indicating what one can do if the assumptions are backwards.

And finally, let me repeat.  I am not trying to FIGHT you, I'm am trying to HELP you.
If you want to expand/correct or provide exceptions to any of my suggestions, feel free.
I won't take offense.

Also,  feel free to click the Request Attention link for Moderator review.  I'm always willing to have someone look over my shoulder and correct me if I'm wrong.
LVL 73

Expert Comment

ID: 36892281

I want to apologize for what appears to be a hijacking of your thread.
Hopefully within any arguments above you are able to find useful information.
As I noted above,  virdi_ds' posts ARE CORRECT for SOME systems, it's very possible all of his advice will work 100% for you.

If that is true, please award him all of the points.
If you want to do a split,  give me the minority of the points and him the majority
LVL 15

Expert Comment

by:Devinder Singh Virdi
ID: 36892383
Thanks Stubber, what made me anger is
yikes, I would avoid a "check list" approach, especially that list.

We both agree that that list may or may not work, I also want to say that checking cursor etc.. is also not correct.

I dont want EE point, rather I was talking about point you are talking is more powerful than my list.

You might have seen in my other post that if I want to correct someone, my way was very polite in the sence "lets expand take an example etc.  " in that way other will not get hurt.

Sorry js4oracle,
LVL 15

Expert Comment

by:Devinder Singh Virdi
ID: 36892557
>>I edited your comment above, removing reference to non-EE person.

Thats fine, no issues

>>If you wish to pursue anything else of a personal matter please email me offline. My contact info is in my profile
There are no personal matters.


Assisted Solution

Anacreo earned 125 total points
ID: 36893831
Throwing my 2 cents in here...

I wouldn't even worry about the PL/SQL factor just take the standard approach to code review:

1. Formatting
-- Is the code well formatted and easy to read; trust me white space isn't going to kill your parser.
2.  Uniformity
-- Did the developer cut and paste from 4 different sources without bringing variable and function definitions in alignment with each other?
-- Are the function and variable naming schemes scalable, these elements should start from the most general to the least general and follow the same convention.
-- ie.  variables of $item_count, $item_descriptions, $item_placement is better than $count_item, $itemDescriptions, $placement_item within the same context; same holds true for Functions.
-- Are function parameters clearly defined and uniform in style
3.  Commenting
-- Don't accept code that doesn't have commenting for each definition, any cryptic lines of code should also be accompanied by comments, if you can't tell what a line does on the first pass ask for it to be commented.
4. Language "magic"
-- Make a standard for magic function, method, property, and variable definitions.  There is good reasons for functions and classes to have well defined parameters, it makes it easier to incorporate in whole into a future project and makes it easier to ensure that things are done right before you ever run the code (variable casting, etc).
-- Examples of this would be don't overload a function without good reason, can the same thing be accomplished by defining the function with default values.
-- This may apply to a language more like PHP and Perl than PL/SQL; but still look for this kind of shortcut/tricks and don't accept them without good justification.
-- Eval statements; is it ABSOLUTELY necessary?  They have their place and can achieve great results; but liberal use of them is usually sloppy coding most of the time.
5.  Meats and Potatoes of the actual code
-- Look for Patterns; are you seeing the same code over and over again with just slightly different syntax, could this be brought into a function which is more maintainable and debuggable?
-- Is this a procedural code that warrants being written as OOP; or vice versa, is this too complex for what was required?
6.  Security
-- Writing code with security in mind will gnerally produce better security AND better code.  Don't let a developer put SQL code all throughout their code or OS calls, they should go through functions that ensure they are well composed and fields are escaped properly.  Don't let a single line of SQL get through review without properly escaping any type of input or using data that could potentially have the wrong data type.
-- Are there proper checks and balances on everything; does your developer check to ensure that comamnds are SANE before executing them... Do DELETE statements check for a where clause to be existant before running unless necessary that there is none.  Do SELECT statements impose a limit on results where one should be expected?
7.  Error Checking; and this should probably be 1st.
-- Error checking, don't mask an error make sure things are tight and error checking is being done.  
-- Is there a proper audit log being generated by the code that will facilitate debugging down the road?

Lastly, ask your developers to do a peer review with a (opposite of themselves) junior or senior member of the team before bringing it in for final review.  If they themselves are senior it will help educate the junior members; and obviously the inverse is true.  This really does help team building and will get people to use similar contexts when coding, it will also get rid of stupid issues before it ever comes to your desk.

Cheers and I hope this helps!
LVL 73

Expert Comment

ID: 36893851
agreed - nice list

Featured Post

Netscaler Common Configuration How To guides

If you use NetScaler you will want to see these guides. The NetScaler How To Guides show administrators how to get NetScaler up and configured by providing instructions for common scenarios and some not so common ones.

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
Question about consuming GB from Comcast 5 53
Error Creating Foreign Keys in SQL Database 7 33
How to join on ID, with prefix? 15 58
Oracle - Query link database loop 8 37
Using SQL Scripts we can save all the SQL queries as files that we use very frequently on our database later point of time. This is one of the feature present under SQL Workshop in Oracle Application Express.
As technology users and professionals, we’re always learning. Our universal interest in advancing our knowledge of the trade is unmatched by most industries. It’s a curiosity that makes sense, given the climate of change. Within that, there lies a…
This video explains what a user managed backup is and shows how to take one, providing a couple of simple example scripts.
This video shows how to copy an entire tablespace from one database to another database using Transportable Tablespace functionality.

813 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question

Need Help in Real-Time?

Connect with top rated Experts

17 Experts available now in Live!

Get 1:1 Help Now