314120 - [Oracle] DBI::st::rows is used in a way that breaks Oracle

Status: RESOLVED FIXED

(Bugzilla :: Database, defect)

Description

[Lance Larsh]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041119
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.3) Gecko/20041119

In checksetup.pl (and possibly elsewhere), DBI::st::rows is used after an execute() of a SELECT without fetching any rows.  For DBD::Oracle, DBI::st::rows always returns the number of rows fetched from that statement handle so far, not the total number of rows that would be returned by the query.  Specifically, this breaks the patch for Bug 312386 when running checksetup.pl on Oracle.  (When running the code from that patch on Oracle, $sth_check->rows is always zero since no rows are fetched, and therefore it attempts to INSERT duplicate rows for the 'sudoers' group, which already exists).

Although the DBD::Oracle behavior differs from that of DBD::mysql, it is still consistent with the DBI documentation for DBI::st::rows, which states:

   Generally, you can only rely on a row count after a non-SELECT execute
   (for some specific operations like UPDATE and DELETE), or after fetching
   all the rows of a SELECT statement.

   For SELECT statements, it is generally not possible to know how many rows
   will be returned except by fetching them all. Some drivers will return the
   number of rows the application has fetched so far, but others may return -1
   until all rows have been fetched. So use of the rows method or $DBI::rows
   with SELECT statements is not recommended.

Rather than using $sth->rows to see if a query returns any rows, it is better to try to fetch some rows (or change the query to "SELECT COUNT(*) ..." and fetch the result).

Reproducible: Always

Steps to Reproduce:
1. Run checksetup.pl against a new Oracle database.
Actual Results:  
checksetup.pl: DBD::Oracle::db do failed:
   ORA-00001: unique constraint (BUGS.GROUP_GROUP_MAP_MEMBER_ID_IDX) violated
   (DBD ERROR: OCIStmtExecute)
   [for Statement "INSERT INTO group_group_map
                   (member_id, grantor_id, grant_type)
                   VALUES (7, 12, 0)"] at Bugzilla/DB/Oracle.pm line 323
    called at ./checksetup.pl line 4441

Comment 1

[Max Kanat-Alexander]

OK, fair enough. We should probably then forbid use of $sth->rows, since it's not necessary anyhow, usually.

Comment 2

[Lance Larsh]

Attachment: Patch removing use of DBI::st::rows

This patch changes the affected queries in checksetup.pl to "SELECT COUNT(*) ..." and uses DBI::st::fetchrow_array in scalar context to retrieve the result.  In Bugzilla/User/Setting.pm, I used the "SELECT 1 FROM ...".$dbh->sql_limit(1) format to be consistent with queries elsewhere in Bugzilla that are called at runtime.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 201448 [details] [diff] [review]
Patch removing use of DBI::st::rows

r=mkanat by inspection (Lance has simply made functionally-equivalent changes, and if I've somehow missed something, test-checksetup would catch it.)

I only have a few comments:

There's a few places (maybe just one) where we could be using selectrow_array instead of prepare/execute/fetch.

Also, technically we shouldn't be checking things like "$sth->fetchrow_array > 0" because fetchrow_array technically returns an array, and some DBDs may actually follow that behavior strictly (though none of ours do, AFAIK).

Comment 4

[Frédéric Buclin]

Bitrotten. Max, that's something we could easily fix. This would be one less task to do for Oracle developers when they will work on Bugzilla again. You could do it yourself. This way you don't need any review. ;)

Comment 5

[Max Kanat-Alexander]

Good point.

Comment 6

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 7

[Frédéric Buclin]

Bugzilla/User/Setting.pm:
236:    return ($sth->rows) ? 1 : 0;

Bugzilla/Install/DB.pm:
1030:        my $total = $sth->rows;
1073:        my $total = $sth->rows;
2140:            if ($trygroupsth->rows > 0) {
2143:        } while ($trygroupsth->rows > 0);
2206:        my $total = $sth->rows;

We should fix this using our "standard" way, i.e.
|my $foo = $dbh->selectcol_arrayref()| and then |if scalar(@$foo) {...}|. This must be very old code.

Comment 8

[Frédéric Buclin]

Max, this bug is still assigned to you. Are you working on it?

Comment 9

[Frédéric Buclin]

(In reply to comment #7)
> Bugzilla/User/Setting.pm:
> 236:    return ($sth->rows) ? 1 : 0;

This one no longer exists, meaning that all other occurences are in Bugzilla/Install/DB.pm only, in the upgrade section. As the concerned code is only about upgrading from Bugzilla < 2.18 or so, this means Oracle is not affected as such old releases didn't support Oracle, and so Oracle will never meet this code. This means we can close this bug as FIXED.

Comment 10

[Max Kanat-Alexander]

Nope.

Comment 11

[bigstijn]

(In reply to comment #9)
> Oracle will never meet this code. This means we can close this bug as FIXED.

Any reason why it isn't closed ?

Comment 12

[Frédéric Buclin]

Because we still want to see it fixed.

Comment 13

[Max Kanat-Alexander]

Yes, Install::DB code still runs even when it's not applicable to the database being used. So the code should be fixed.

Comment 14

[Frédéric Buclin]

Attachment: patch, v2

Remove all occurences of $sth->rows.

Comment 15

[Max Kanat-Alexander]

Comment on attachment 310095 [details] [diff] [review]
patch, v2

Looks good to me!

Comment 16

[Frédéric Buclin]

Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.49; previous revision: 1.48
done