582209 - [Oracle] Bugzilla::DB::Oracle adjust_statement LIMIT code corrupts sub-selects

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[David Taylor]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.6) 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

Comment 1

[David Taylor]

Attachment: Example input/output from adjust_statement for a broken query

Comment 2

[Michael Thomas (Mockodin)]

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.

Comment 3

[Max Kanat-Alexander]

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.

Comment 4

[Jasmin Sehic]

Attachment: Proposed refactored sql_limit() and fix for Oracle sub-select corruption

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.

Comment 5

[Jasmin Sehic]

Attachment: Proposed refactored sql_limit() and fix for Oracle sub-select corruption v2

Last patch had one small issue in Object.pm

Comment 6

[Jasmin Sehic]

Attachment: Proposed refactored sql_limit() and fix for Oracle sub-select corruption v3

v2 patch had MS-SQL patches in it by mistake. Sorry about the spam. Third time lucky.

Comment 7

[Max Kanat-Alexander]

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

Comment 8

[Jasmin Sehic]

(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 9

[Max Kanat-Alexander]

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?

Comment 10

[Jasmin Sehic]

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.

Comment 11

[Frédéric Buclin]

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).

Comment 12

[Frédéric Buclin]

Attachment: patch, v4

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.

Comment 13

[Max Kanat-Alexander]

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. :-)

Comment 14

[Frédéric Buclin]

Comment on attachment 533530 [details] [diff] [review]
Proposed refactored sql_limit() and fix for Oracle sub-select corruption v3

This one is too complex.

Comment 15

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/DB/Oracle.pm
Committed revision 7920.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.2/
modified Bugzilla/DB/Oracle.pm
Committed revision 7901.

Committing to: bzr+ssh://[e-mail]/bugzilla/4.0/
modified Bugzilla/DB/Oracle.pm
Committed revision 7642.