Issue Details (XML | Word | Printable)

Key: NUCRDBMS-251
Type: Improvement Improvement
Status: Closed Closed
Resolution: Won't Fix
Priority: Minor Minor
Assignee: Unassigned
Reporter: Yang ZHONG
Votes: 0
Watchers: 1
Operations

If you were logged in you would be able to see more operations.
DataNucleus Store RDBMS

Prepared Statements should have been used instead of literals in order to improve performance big time

Created: 27/Aug/09 11:51 PM   Updated: 26/Dec/09 12:56 PM   Resolved: 10/Dec/09 11:19 AM
Component/s: None
Affects Version/s: 1.1.2, 1.1.3, 1.1.4, 1.1.5
Fix Version/s: None

File Attachments: 1. Zip Archive 251.zip (4 kB)

Environment: Java 5, Linux

Forum Thread URL: HTTP://WWW.DataNucleus.org/servlet/forum/viewthread_thread,5558
Datastore: IBM DB2
Severity: Production


 Description  « Hide
Add this to JRE/lib/logging.properties:
    DataNucleus.Datastore.Native.level=FINE
and change "java.util.logging.ConsoleHandler.level" to "FINE",

or configure log4j,

then the attached Test Case outputs:

    ... org.datanucleus.store.rdbms.SQLController executeStatementQuery
    FINE: SELECT A0.JDOCLASSX FROM A A0 WHERE 'test' = A0.S

    ... org.datanucleus.store.rdbms.SQLController executeStatementQuery
    FINE: SELECT 'org.datanucleus.test.A' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.A' UNION SELECT 'org.datanucleus.test.B' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.B' UNION SELECT 'org.datanucleus.test.C' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.C'

which should have been

    SELECT A0.JDOCLASSX FROM A A0 WHERE A0.S = ?

and

    SELECT A0.JDOCLASSX AS NUCLEUS_TYPE FROM A A0 WHERE A0.S = ? AND A0.JDOCLASSX = ? UNION SELECT A0.JDOCLASSX AS NUCLEUS_TYPE FROM A A0 WHERE A0.S = ? AND A0.JDOCLASSX = ? UNION SELECT A0.JDOCLASSX AS NUCLEUS_TYPE FROM A A0 WHERE A0.S = ? AND A0.JDOCLASSX = ?

respectively in order for much better performance.

Thanks!

Yang ZHONG made changes - 27/Aug/09 11:53 PM
Field Original Value New Value
Attachment 251.zip [ 11008 ]
Andy Jefferson added a comment - 28/Aug/09 06:52 AM
A minor improvement is not a "blocker" no matter how important you think it is.

Andy Jefferson made changes - 28/Aug/09 06:52 AM
Priority Blocker [ 1 ] Minor [ 4 ]
Yang ZHONG added a comment - 28/Aug/09 04:21 PM
Furthermore, would

SELECT distinct JDOCLASSX AS NUCLEUS_TYPE FROM A WHERE S = ? AND JDOCLASSX in (?,?,?)

return the exactly same results however much better performance than:

SELECT 'org.datanucleus.test.A' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.A' UNION SELECT 'org.datanucleus.test.B' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.B' UNION SELECT 'org.datanucleus.test.C' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.C'

Yang ZHONG added a comment - 02/Sep/09 11:50 PM - edited
In order to repeat "SELECT A0.JDOCLASSX FROM A A0 WHERE 'test' = A0.S", please
    delete from NUCLEUS_TABLES

Yang ZHONG added a comment - 05/Sep/09 12:47 AM
Only for the only purpose of Proof of Concept, changing org.datanucleus.store.rdbms.RDBMSStoreHelper.getClassNameForIdKeyUsingUnion from

        while (optionsIter.hasNext())
        {
            ...
            SQLStatement sqlStmt = new SQLStatement(storeMgr, table, null, null);

            // Select NUCLEUS_TYPE
            ...

            // LEFT OUTER JOIN to all direct subclasses
            ...
            SQLExpression fieldVal = exprFactory.newLiteral(sqlStmt, idMapping, sm.getObject());
            sqlStmt.whereAnd(fieldExpr.eq(fieldVal), true);
 
            // Discriminator for this class
            ...
                sqlStmt.whereAnd(discrimExpr.eq(discrimVal), false);
        ...

        // Perform the query

to

    protected static final class SQL
    {
        protected SQL(SQLStatement sql,JavaTypeMapping discrimMapping)
        {
            statement = sql;
            discriminator = discrimMapping;
        }
        final SQLStatement statement;
        final JavaTypeMapping discriminator;

        protected final SQLExpression discriminator(SQLExpressionFactory exprFactory)
        {
            if (table == null)
            {
                table = statement.getPrimaryTable();
            }
            return exprFactory.newExpression(statement, table, discriminator);
        }
        SQLTable table/* = null*/;

        @SuppressWarnings("unchecked")
        protected Collection discriminators/* = null*/; // not empty
    }
        Map<Object,SQL> SQLs = new HashMap<Object,SQL>();
        while (optionsIter.hasNext())
        {
            ...
            // Select NUCLEUS_TYPE
            final JavaTypeMapping discrimMapping;
            SQL sql = SQLs.get(table);
            if (sql == null)
            {
            SQLStatement sqlStmt = new SQLStatement(storeMgr, table, null, null);
                discrimMapping = table.getDiscriminatorMapping(false);
                sql = new SQL(sqlStmt, discrimMapping);
                SQLs.put(table, sql);
                if (discrimMapping == null)
                {
            JavaTypeMapping typeMapping = storeMgr.getMappingManager().getMapping(String.class);
            String classname = StringUtils.leftAlignedPaddedString(schemaDataOption.getName(), metadata_id_len);
            sqlStmt.select(exprFactory.newLiteral(sqlStmt, typeMapping, classname), "NUCLEUS_TYPE");
                }
                else
                {
                    sqlStmt.setDistinct(true);
                    sqlStmt.select(sql.discriminator(exprFactory), "NUCLEUS_TYPE");
                }

            // LEFT OUTER JOIN to all direct subclasses
            ...
                SQLExpression fieldVal = exprFactory.newLiteralParameter(sqlStmt, idMapping, sm.getObject());
            sqlStmt.whereAnd(fieldExpr.eq(fieldVal), true);

            } // sql == null
            else
            {
                discrimMapping = sql.discriminator;
            }

            // Discriminator for this class
            if (discrimMapping != null)
            {
                Object value = null;
                if (cmd.getDiscriminatorStrategy() == DiscriminatorStrategy.CLASS_NAME)
                {
                    value = schemaDataOption.getName();
                }
                else if (cmd.getDiscriminatorStrategy() == DiscriminatorStrategy.VALUE_MAP)
                {
                DiscriminatorMetaData discrimMetaData = cmd.getInheritanceMetaData().getDiscriminatorMetaData();
                    value = discrimMetaData.getValue();
                }
                else
                {
                    continue;
                }
                if (sql.discriminators == null)
                {
                    sql.discriminators = new ArrayList();
                }
                sql.discriminators.add(value);
            }
        } // optionsIter

        for (SQL sql : SQLs.values())
        {
            SQLStatement sqlStmt = sql.statement;
            if (sql.discriminators != null)
            {
                sqlStmt.whereAnd(sql.discriminator(exprFactory).in(new CollectionLiteral(sqlStmt, sql.discriminator, sql.discriminators, true)), true);
        ...
        sqlStmtMain.setRange(0, 1);

        // Perform the query

and org.datanucleus.store.rdbms.sql.expression.CollectionLiteral#CollectionLiteral from

            if (value instanceof Collection)
            {
                this.value = (Collection)value;
            }
            ...

to

            if (value instanceof Collection)
            {
                this.value = (Collection)value;
                char c = COLLECTION;
                for (Object v : this.value)
                {
                    st.append(c);
                    st.appendParameter(mapping, v);
                    c = ',';
                }
                if (c != COLLECTION)
                {
                    st.append(')');
                    return;
                }
            }
            ...
    static protected final char COLLECTION = '(';

will generate

    SELECT distinct A0.JDOCLASSX AS NUCLEUS_TYPE FROM A A0 WHERE A0.S = ? AND A0.JDOCLASSX in (?,?,?)

instead of

    SELECT 'org.datanucleus.test.A' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.A' UNION SELECT 'org.datanucleus.test.B' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.B' UNION SELECT 'org.datanucleus.test.C' AS NUCLEUS_TYPE FROM A A0 WHERE 'test' = A0.S AND A0.JDOCLASSX = 'org.datanucleus.test.C'

At the same time, since RDBMSStoreHelper.getClassNameForIdKeyUsingUnion only needs only one result, I recommend
        sqlStmtMain.setRange(0, 1);
for maximum performance possible.

Thanks.

Yang ZHONG added a comment - 06/Sep/09 06:18 AM - edited
Only for the only purpose of Proof of Concept, changing org.datanucleus.store.rdbms.RDBMSStoreHelper.getClassNameForIdKeyUsingDiscriminator from

        SQLExpression sqlFldVal = exprFactory.newLiteral(sqlStmt, idMapping, sm.getObject());
        sqlStmt.whereAnd(sqlFldExpr.eq(sqlFldVal), true);

to

        SQLExpression sqlFldVal = exprFactory.newLiteralParameter(sqlStmt, idMapping, sm.getObject());
        sqlStmt.whereAnd(sqlFldExpr.eq(sqlFldVal), true);
        sqlStmt.setRange(0, 1);

will generate

    SELECT A0.JDOCLASSX FROM A A0 WHERE A0.S = ?

instead of

    SELECT A0.JDOCLASSX FROM A A0 WHERE 'test' = A0.S

At the same time, since RDBMSStoreHelper.getClassNameForIdKeyUsingDiscriminator only needs only one result, I recommend
        sqlStmt.setRange(0, 1);
for maximum performance possible.

Thanks.

Yang ZHONG added a comment - 10/Sep/09 11:17 PM
We're seeing almost 40% reduction in Database CPU being saved in statement preparation.

Yang ZHONG added a comment - 15/Oct/09 04:41 PM
Same fix is also applicable to 1.1.6.

Andy Jefferson added a comment - 10/Dec/09 11:19 AM
Please actually demonstrate something. As forum posts have all said, JDOQL2 is where work happens and I see no issue. Provide a testcase using that and showing clearly in the log what is used where ... and then this issue can be reopened. I'm not interested in "Proof of Concept"; if you want to demonstrate something with the code and some changes then patch (unified diff) is the standard way of showing this and nothing else

Andy Jefferson made changes - 10/Dec/09 11:19 AM
Status Open [ 1 ] Resolved [ 5 ]
Resolution Won't Fix [ 2 ]
Andy Jefferson made changes - 26/Dec/09 12:56 PM
Status Resolved [ 5 ] Closed [ 6 ]