Bug 231030

Created attachment 135585 [details]
Proposed Patch

Created attachment 135602 [details]
Proposed Patch - UI

I've split the patch into two parts. The first part, committed as
http://hg.netbeans.org/core-main/rev/832e3500091e, uses default list of drivers that can use scrollable cursors. This list can be customized using system property "db.scrollable.cursors.drivers".

The second (just attached) part contains the UI for enabling or disabling scrollable cursors for individual connections. This change needs to pass an API review.

Please review API change of class
org.netbeans.api.db.explorer.DatabaseConnection.

New methods:
boolean isUseScrollableCursors()
void    setUseScrollableCursors(boolean)

Thank you.

Integrated into 'main-golden', will be available in build *201306112301* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/832e3500091e
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #231030: Make usage of scrollable cursors configurable

Hey, I had a look at the changeset today (832e3500091e). A few remarks (I'm at work and have only limited access, so I'm just reading through the diff - please take the points with that in mind ;-))

a) getTotalCount - line 1279: This fails if the resultset contains less rows, than the limit, it also fails with the extended mysql limit syntax: "LIMIT offset, count"

b) getTotalCount - line 1324: Are you sure?! setFetchSize(20000) - if it is not ignored by the driver, it might very well blow the heap

c) The optimisations in getCountSQLQuery and getCountAsSubQuery look suspicious:

SELECT * FROM (
SELECT a, b, c, count(*) FROM TABLE2 GROUP BY 1, 2, 3 ORDER BY 1, 2, 3
) TABLE1
ORDER BY 2,1

would in case of getCountAsSubQuery lead to:

SELECT COUNT(*) FROM (SELECT * FROM (SELECT a, b, c, count(*) FROM TABLE 2 GROUP BY 1, 2, 3) C2668


Related: The mysql driver/server is utterly broken with respect to handling of large number of rows.

The simple sample (c is a connection to a mysql server):

Statement s = c.createStatement();
ResultSet rs = s.executeQuery("SELECT * FROM LARGE_TABLE");
for(int i = 0; i < 20 && rs.next(); i++) {
    System.out.println(rs.getString(1));
}
rs.close();
s.close();

Blows with a heap overflow...

You can:

a) Limit the absolute number of rows to fetch:

s.setMaxRows(30);

b) Switch to streaming mode (works sort of, still read _whole_ resultset):

s.setFetchSize(Integer.MIN_VALUE);

Both solutions are no-gos (both are broken in more than one way....

(In reply to comment #5)
> a) getTotalCount - line 1279: This fails if the resultset contains less rows,
> than the limit, it also fails with the extended mysql limit syntax: "LIMIT
> offset, count".
Good point. Removed.

> b) getTotalCount - line 1324: Are you sure?! setFetchSize(20000) 
> - if it is not ignored by the driver, it might very well blow the heap
I've just reverted the method from the original implementation. It's seems that this code wasn't invoked often. 
It's probably safer to show N/A if other ways to get total count fail. So I'm removing this part of code, too.

> c) The optimisations in getCountSQLQuery and getCountAsSubQuery look
> suspicious:
It seems to work fine. The method was in use for quite a long time, so it should work reliably.
Or why do you think it's suspicious?

Fixed in http://hg.netbeans.org/core-main/rev/5ad93a1b4bde
Thank you very much.

(In reply to comment #6)
> > c) The optimisations in getCountSQLQuery and getCountAsSubQuery look
> > suspicious:
> It seems to work fine. The method was in use for quite a long time, so it
> should work reliably.
> Or why do you think it's suspicious?

I did some paper calculation :-)

    static String getCountAsSubQuery(String queryString) {
        /* Input: 
         * SELECT * FROM (
         *     SELECT a, b, c FROM TABLE2 ORDER BY 1, 2, 3
         * ) TABLE1 WHERE a = 1
         */
        String[] splitByOrderBy = queryString.toUpperCase().split("ORDER BY"); // NOI18N
        /**
         * spitByOrderBy looks like this:
         * {
         * "SELECT * FROM (\n    SELECT A, B, C FROM TABLE2 ",
         * ") TABLE 1 WHERE A = 1"
         * }
         */
        queryString = queryString.substring(0, splitByOrderBy[0].length());
        /*
         * queryString = "SELECT * FROM ( SELECT a, b, c, count(*) FROM TABLE2 
         */
        return "SELECT COUNT(*) FROM (" + queryString + ") C2668"; // NOI18N
        /*
         * Result:
         * 
         * SELECT COUNT(*) FROM (SELECT * FROM ( SELECT a, b, c, count(*) FROM TABLE2) C2668
         */
    }

The result is wrong - even if you add the missing brace, the count will be way off.

While the above query was deliberately choosen, this could happen at any time. You could try to tackle this with a full blown SQL parser, but the extra time the one ORDER you optize out will most probably not be worth it.

> > > c) The optimisations in getCountSQLQuery and getCountAsSubQuery look
> > > suspicious:
> The result is wrong - even if you add the missing brace, the count will be way
> off.
You are right, thank you for explanation and the example.

> While the above query was deliberately choosen, this could happen at any time.
> You could try to tackle this with a full blown SQL parser, but the extra time
> the one ORDER you optize out will most probably not be worth it.
I originally thought that ORDER BY is removed because some DBs don't allow it in subqueries, but it is not probably the problem.
I agree it's not worth optimizing.
Fixed in http://hg.netbeans.org/core-main/rev/090decadd5e7
Thank you.

(In reply to comment #3)
> Please review API change of class
> org.netbeans.api.db.explorer.DatabaseConnection.
If there are no objections, I'll integrate the patch on Monday.
Thanks for reviewing.

Integrated into 'main-golden', will be available in build *201306212301* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/4db74340500c
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #231030: DataView pagination broken if scrollable cursors are disabled

(In reply to comment #9)
> If there are no objections, I'll integrate the patch on Monday.
http://hg.netbeans.org/core-main/rev/a31ab5991cd5

Removing apireviews@netbeans from the CC list, as future comments will not be probably related to the API change.

For reference, increasing priority.
A regression (broken pagination) was introduced by changeset
http://hg.netbeans.org/main-golden/rev/832e3500091e.
Fixed in http://hg.netbeans.org/main-golden/rev/4db74340500c.

Integrated into 'main-golden', will be available in build *201306242301* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/090decadd5e7
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #231030: Do not try to remove ORDER BY clause from queries for getting total count

Integrated into 'main-golden', will be available in build *201306262301* on http://bits.netbeans.org/dev/nightly/ (upload may still be in progress)
Changeset: http://hg.netbeans.org/main-golden/rev/a31ab5991cd5
User: Jaroslav Havlin <jhavlin@netbeans.org>
Log: #231030: Make usage of scrollable cursors configurable - UI