Closed Bug 582209 Opened 10 years ago Closed 9 years ago
[Oracle] Bugzilla::DB::Oracle adjust
_statement LIMIT code corrupts sub-selects
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:126.96.36.199) Gecko/20100625 Firefox/3.6.6 Build Identifier: Bugzilla 3.6.0 The code in adjust_statement to add a 'WHERE rownum <= n' clause to an SQL statement does not take into account the possibility of a sub-select with a WHERE clause. See the attached example. The query is split into three parts by the two 'WHERE' tokens. The query is then changed to split-1 'WHERE rownum <= n' split-2. The 'WHERE' split-3 part is lost, as only the first two elements of the list returned by split() are used. The straightforward fix is to add a limit parameter (2) to the split call in Oracle.pm line ~393. But this will only work correctly if the first WHERE token found is for the main query, which is not necessarily the case (but generally seems to be for Bugzilla). Reproducible: Always
I think I brought this up before else where and met with a smack down, but the method used for limit is not.. nice to begin with. I think we should consider changing how sql_limit() is implemented to begin with. Instead of doing this $sql .= $dbh->sql_limit(1); It should be written as $sql = $dbh->sql_limit($sql,1); That was there is real support for multiple database platforms. Instead up supporting one or two and expecting the others to implement what is, a hack. So databases that support a trailing limit operator like mysql can be happy and databases like Oracle and MSSQL can support their own methods. It would certainly make adjust_sql much.. cleaner(?) than it current is for oracle and mssql.
mockodin: That's certainly something that we can consider. If you want to attach a proof-of-concept patch, I'd be willing to consider it. I thought LIMIT was ANSI, but it's not--it's just the most-common implementation of this feature.
Oracle implementation needs verification. I am using mockodin's MS-SQL patches on Bugzilla trunk r7817 and changes were tested there and there is some very slight differences between MS-SQL and Oracle but it should work.
Last patch had one small issue in Object.pm
Attachment #533511 - Attachment is obsolete: true
v2 patch had MS-SQL patches in it by mistake. Sorry about the spam. Third time lucky.
Attachment #533528 - Attachment is obsolete: true
Comment on attachment 533530 [details] [diff] [review] Proposed refactored sql_limit() and fix for Oracle sub-select corruption v3 Cool! Make sure to request review on your patches. :-) Our development process is here: http://wiki.mozilla.org/Bugzilla:Development
(In reply to comment #7) > Cool! Make sure to request review on your patches. :-) Our development > process is here: > > http://wiki.mozilla.org/Bugzilla:Development Thanks! Yeah I was trying to look for what ? + - all meant but couldn't find any info on it so ended up leaving it blank. I will make sure I follow this process next time.
Comment on attachment 533530 [details] [diff] [review] Proposed refactored sql_limit() and fix for Oracle sub-select corruption v3 Review of attachment 533530 [details] [diff] [review]: ----------------------------------------------------------------- Thank you so much for this PoC patch! I would have accepted seeing the change done in just one place, BTW, for just a PoC, so you know for the future. :-) I'm sorry that it took so long for me to get around to reviewing it, also! So, now that I look at this, it does seem a bit harder to read than our current system, except in places like Search.pm, where it's totally simple. Search.pm is also going to be the place where it's the most important. Any thoughts on other ways to fix this that might be more readable and require less change?
We can always refactor the code so that we perform a limit first and store it in another variable and then we do selectall_arrayref calls after. Right now these calls are nested and I agree it is easy to miss them. This would mean an additional line of code but at least the selectall_arrayref calls would be easier to read.
I can easily reproduce the problem when using quicksearch and looking for two words. Bugzilla dies badly. I have a fix, much smaller than the one proposed by Jasmin (which looks too invasive for branches).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 4.0
Version: unspecified → 3.6
This patch also fixes the problem with FROM when an offset is present. /(.*)foo(.*)/ doesn't do what you expect when "foo" appears several times. This regexp catches the last "foo" in the string, not the first one. See perlre for more explanations. And I doubt we want to catch occurences appearing in subselects here.
Attachment #552813 - Flags: review?(mkanat)
Comment on attachment 552813 [details] [diff] [review] patch, v4 Review of attachment 552813 [details] [diff] [review]: ----------------------------------------------------------------- I haven't tested this, but this looks much more sensible, yeah. :-)
Attachment #552813 - Flags: review?(mkanat) → review+
Comment on attachment 533530 [details] [diff] [review] Proposed refactored sql_limit() and fix for Oracle sub-select corruption v3 This one is too complex.
Committing to: bzr+ssh://email@example.com/bugzilla/trunk/ modified Bugzilla/DB/Oracle.pm Committed revision 7920. Committing to: bzr+ssh://firstname.lastname@example.org/bugzilla/4.2/ modified Bugzilla/DB/Oracle.pm Committed revision 7901. Committing to: bzr+ssh://email@example.com/bugzilla/4.0/ modified Bugzilla/DB/Oracle.pm Committed revision 7642.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.