Comments (11)
Based on the severity of these violations I think (now) that the following two rules would be good:
Never commit within a cursor loop Try to move transactions within a non-cursor loop into procedures
Works for me. 👍
from plsql-and-sql-coding-guidelines.
And just to be clear...
a "cursor loop" is not only
for rec in cursor loop
but also
open ...;
fetch ... into ...;
while ...%found loop
and
open ...
loop
fetch ... into ...
exit when ...%notfound;
hth
from plsql-and-sql-coding-guidelines.
from SonarSource Rules:
"COMMIT" should not be used inside a loop
Frequent commits are widely understood to negatively impact performance. Thus, committing inside a loop (even when only executed conditionally once every n iterations) is highly likely to cause unwanted performance impacts.
Further, in general use COMMIT should only be used at the end of a transaction. Code that is not structured to have one transaction per loop iteration could yield unexpected results if COMMIT is nonetheless used inside the loop. Code that is structured to have one transaction per loop iteration should probably be reconsidered.
Note that when dealing with very large data sets, a COMMIT may be required every n iterations, but the goal should be to avoid COMMITs inside loops.
Noncompliant Code Example
FOR item IN itemlist
LOOP
-- ...
COMMIT; -- Noncompliant
END LOOP;
Compliant Solution
FOR item IN itemlist
LOOP
-- ...
END LOOP;
COMMIT;
from plsql-and-sql-coding-guidelines.
From my point of view this is not always bad, for example when the loop is already handling bulk data (e.g. large updates, dynamic SQL in ETL, etc). A simple fix is to move the commit into a procedure and call this procedure in the loop. Is this really wanted?
from plsql-and-sql-coding-guidelines.
I see your point. Is there a possibility in PL/SQL Cop to throw a warning (like: Are you aware of...? Do you realy want to...?)? If so, this might be a possible candidate for this.
from plsql-and-sql-coding-guidelines.
Actually all guideline violations are warnings. But as a developer I want to get rid of a warning, because I do not want to assess it over and over again. So, I have the following options:
a) disable the guideline check for this rule
b) add a -- NOSONAR
comment to suppress the warning next time
c) change the code in a way that the violation does not occur again.
If "c)" is more often wrong than right, then the rule is probably bad and I will disable it anyway.
I'm not against this rule (yet), but I'm not convinced that this is a good rule as defined here. Maybe @kibeha and @rogertroller like to share their opinion on this topic.
from plsql-and-sql-coding-guidelines.
Hmmm.....
it depends. Not only on what the code inside the loop does but also on the loop-type.
So I agree if the loop is iterating on a cursor (could be any loop-type, is the case on a cursor-for-loop). In this case you really should not commit inside the loop.
If your problem is a large update or insert, then a loop is perhaps the wrong way to do it, possibly a parallel_execute to split the operation would be more appropriate.
If the loop does many operations which are "self-contained" - there will be no problem that part of the changes may be seen by other users - the restartability of whole prcoess does not suffer...then yes, a commit inside the loop would not be wrong. But in this case we are iterating over a bunch of "transactions" and the processing could be placed in a procedure (local/private/public?) as Philipp mentioned earlier.
So it's not always black/white.
from plsql-and-sql-coding-guidelines.
So I agree if the loop is iterating on a cursor (could be any loop-type, is the case on a cursor-for-loop). In this case you really should not commit inside the loop.
Good point. For large transactions this might lead to an ORA-1555 while reading the cursor. So, if the commits are transactions then it would make sense to process them differently, e.g. reading main cursor into a collection and looping over the collection (assuming that the size of the collection is relatively small).
I agree with the rule if we change the rule to something like:
Avoid commits within a cursor loop.
from plsql-and-sql-coding-guidelines.
Good points mentioned so far.
"Avoid commits within a cursor loop" could be the rule, but conversely you cannot state that commits within a non-cursor loop is always acceptable. I think it'll be hard to create a rule that can figure out whether the commit is acceptable or not - identifying loop type would be helpful, but not sufficient.
Like Roger said, it's not black/white - which means it's harder to create an algorithm for it. My thinking is to take the high-level approach and leave the gray-zone decision to the developer.
A commit in a loop can be categorized 3 ways:
- A bad idea.
- A case of a loop doing individual contained transactions for each iteration.
- A case of handling large bulk operations with proper code for restartability in place.
In my opinion for 2) it would be best practice to let the loop call a procedure that wraps the individual transaction and has the commit.
I believe that 3) is more rare than 1), so personally I'd be happy that the rule warns on every commit within a loop (all loops), so that I as developer is forced to think about if I have a case of 1), 2) or 3).
If I have a case 2) I should move my logic to a procedure. If I have a (rare) case 3) I should explicitly use --NOSONAR to indicate in the source that I have given it thought and decided that committing in the loop is called for in this case.
To help guide the developer, maybe split the rule in two.
- Commits in a cursor loop gets warning: "consider moving transaction to a procedure".
- Commits in a non-cursor loop gets warning: "if logic is restartable, consider using --NOSONAR".
Would help the developer make his decision what to do ;-)
Just my 2 cent.
from plsql-and-sql-coding-guidelines.
Good idea to split the rule, thanks @kibeha. However, regarding the
commits in a cursor loop
I think they are always wrong (Kim's case 1 - a bad idea). Hence, the rule could be named Never commit within a cursor loop.
Moving the transaction to a procedure will eliminate the warning, but the risk of an ORA-1555 is still there. The same is true for the "NOSONAR" comment. In this case I see two sensible recommendations:
- Move the COMMIT outside of the loop.
- this addresses the increased risk of an ORA-1555
- and makes the process restartable (handled by the database transaction)
- Populate a collection containing all the "transactions" to be processed in non-cursor loops
- this addresses the increased risk of an ORA-1555 as well
- but does not make the process automatically restartable. Restartability becomes the responsibility of the application (e.g. the state of transactions must be deducible by the process populating the collection of transactions)
commits in a non-cursor loop
These are either Kim's
- case 2 - a loop doing individual contained transactions for each iteration.
- case 3 - handling large bulk operations with proper code for restartability in place (also transactions)
In both cases the transactions could be moved into dedicated procedures including the commit. This would clearly document that the procedure is handling a transaction. And most probably it will also reduce the code within the loop, making it more readable. That would be my recommendation for "commits in a non-cursor loop".
Based on the severity of these violations I think (now) that the following two rules would be good:
- Never commit within a cursor loop
- Try to move transactions within a non-cursor loop into procedures
I like these rules better. Maybe because a "NOSONAR" comment is not a recommended/accepted solution anymore and moving code into procedures has some value regarding readability and maintainability of the code.
from plsql-and-sql-coding-guidelines.
Thanks to all. The feedback was very helpful. It almost felt like one of those conversations we would normally have near a coffee machine. ☕️☕️☕️☕️
We're planing to add two rules based on this issue.
from plsql-and-sql-coding-guidelines.
Related Issues (20)
- G-3120: eliminate violations
- G-9030: eliminate violations HOT 1
- G-1040: add NOSONAR Hint in G-7250
- G-7110: eliminate violations
- G-3210: eliminate violations
- G-4320: eliminate violations
- Various guidelines: eliminate violations (2-4 issues per guideline) HOT 1
- New rule: Always specify column aliases instead of positional references in GROUP BY clauses.
- CASE / WHEN is indentend backwards HOT 2
- Fix catagorization of guideline severties.
- New rule: Always specify column names instead of expressions in GROUP BY clauses.
- The tools do not work from a Windows host with git-bash HOT 1
- Use 3 literals in the bad example of G-1050
- Simplify G-1050 and related examples
- Include column alias in G-3182 HOT 1
- Fix revision history in about page
- Highlight the lines that violate a guideline in the bad examples (and the fixed lines in the good examples)
- G-8310 value_error good_example HOT 3
- New rule: Avoid autonomous transactions
- New rule: Avoid using a FOR LOOP for a query that should return not more than one row HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from plsql-and-sql-coding-guidelines.