Bugzilla Version 2.16.7
Bugzilla Bug 1733
  Look at database versions to determine specifically supported features
     Query page      Enter new bug
Bug#: 1733   Platform:   Reporter: martin-fuchs at gmx dot net (Martin Fuchs)
Product:   OS:   Add CC:
Component:   Version:   CC:
Remove selected CCs
Status: CLOSED   Priority:  
Resolution: FIXED   Severity:  
Assigned To: werner dot guttmann at gmx dot net (Werner Guttmann)   Target Milestone:  
URL:
Summary:

Attachment Type Modified Status Actions
proposed patch patch 2004-09-15 01:51 none Edit
updated patch with DBMetaInfo member in JDBCQueryExpression patch 2004-09-16 03:53 none Edit
updated patch patch 2004-09-27 15:25 none Edit
Create a New Attachment (proposed patch, testcase, etc.) View All

Bug 1733 depends on: 1732 Show dependency tree
Show dependency graph
Bug 1733 blocks:
Votes: 0    Show votes for this bug    Vote for this bug

Additional Comments:


Leave as CLOSED FIXED
Reopen bug

View Bug Activity   |   Format For Printing

Description: Opened: 2004-09-10 13:11

There are some newer database features, which are only available for certain 
database versions. For example the proposed solution for implementing LIMIT and 
OFFSET using Oracle database connects in Bug 1732 is only available for 
versions of 8.1.6 or above. Currently the functions isLimitClauseSupported() 
and isOffsetClauseSupported() always return the value 'true'.

Wouldn't it be possible to look first at the database version, to determine if 
the keywords can be supported?

I think, it would be better to look at the information obtained by 
DatabaseMetaData.getDatabaseProductVersion() to determine if the used database 
really supports the required features.

------- Additional Comment #1 From Werner Guttmann 2004-09-10 13:40 -------
I agree that this would make sense, e.g. in the context of bug 1732.

I just had a look at the code, and to me the main problem is that to perform a
DatabaseMetaData.getDatabaseProductVersion() one needs a valid
java.sql.Connection instance. Unfortunately, at the moment of this conversation,
(JDBC)QueryExpression does not have access to a java.sql.Connection instance.

Iow, we first needed to investigate how to go about this problem ...

------- Additional Comment #2 From Werner Guttmann 2004-09-10 13:42 -------
Just an observation: JDBCQueryExpression does NOT implememt QueryExpession,
though it should (I assume) 

------- Additional Comment #3 From Bruce Snyder 2004-09-13 21:12 -------
Here's an idea, why not only support only Oracle 8.1.6 and above? We can ask on
the castor-dev at  list to see if anyone is using a version lower than this. 

------- Additional Comment #4 From Werner Guttmann 2004-09-14 01:28 -------
The more I think about this, the more I agree. Unless, Martin, you can think of
a way of introducing this new required functionality in a meaningful way. I had
a thorough look at the sourdes, but cannot think of such a solution.

------- Additional Comment #5 From Martin Fuchs 2004-09-14 12:51 -------
> I just had a look at the code, and to me the main problem is that to perform a
> DatabaseMetaData.getDatabaseProductVersion() one needs a valid
> java.sql.Connection instance. Unfortunately, at the moment of this 
conversation,
> (JDBC)QueryExpression does not have access to a java.sql.Connection instance.

Well, this problem can be overcome if you accept a interface change to 
Database.getOQLQuery():

Currently the database connection is typically opened at the first execute() 
call of a transaction. I moved this point into the OQLQueryImpl.create() call. 
This way I can access the JDBC connection and look at the version information 
in the metadata. I'm passing this on to ParseTreeWalker, and can use the 
information in OracleQueryExpression.

I need just some time clean up my patch. It will include Werner's patch for bug 
1732, since the change is meaningless without this.

------- Additional Comment #6 From Martin Fuchs 2004-09-15 01:43 -------
Register dependency on Bug 1732

------- Additional Comment #7 From Martin Fuchs 2004-09-15 01:51 -------
Created an attachment (id=664)
proposed patch

------- Additional Comment #8 From Martin Fuchs 2004-09-15 02:00 -------
The patch uses a new helper class DbMetaInfo to retrieve the database version 
from the JDBC connection and allows comparison with the required databse 
versions. For efficiency the DbMetaInfo is cached in TransactionContextImpl. It 
is passed from OQLQueryImpl.create() through ParseTreeWalker and 
JDBCQueryExpression into isLimitClauseSupported/isOffsetClauseSupported() in 
OracleQueryExpression. This is where the database version is queried and 
decided whether LIMIT/OFFSET is supported or not.

------- Additional Comment #9 From Werner Guttmann 2004-09-15 14:11 -------
Martin, I had a brief look at your patch, and I think it all makes sense. Can I
ask you two things:

a) Try to run the Castor JDO CTF suite using test.bat|sh castor.oracle, and see
what errors you get.
b) I do not like the fact that DbMetaInfo is passed to just the
isLimit|OffsetClauseSupported() when we might need this information again in the
future. Instead, can you please amend QueryExpression (and JDBCQueryExpression)
to accept a DbMetaInfo instance and store as an instance member, and then use it
from within one or more of the DB-specific implementations as required.

------- Additional Comment #10 From Martin Fuchs 2004-09-16 03:53 -------
Created an attachment (id=668)
updated patch with DBMetaInfo member in JDBCQueryExpression

------- Additional Comment #11 From Martin Fuchs 2004-09-16 04:07 -------
Because QueryExpression is only a interface, no regular class, I can't put 
DBMetaInfo as constructor parameter, so I added a setDbMetaInfo() function 
instead. Trying to extend SQLEngine.getQueryExpression() and initializing the 
dbInfo object in PersistenceFactory would result in much more code changes. So 
I just call setDbMetaInfo() in ParseTreeWalker.createQueryExpression(). A small 
disadvantage of this altered patch is, you can't be sure _dbInfo is really 
initialized any time in calls to isLimit/OffsetClauseSupported(), so you first 
have to look if it might still be null. (see OracleQueryExpression)

Running the test cases gives me the following output:


There was 1 error:
1) [castor.oracle.TC47] Persistence interface tests
org.exolab.castor.jdo.ObjectModifiedException: Transaction aborted: Object of 
type jdo.TestPersistent with identity 73 has been modified by a concurrent 
transaction (cache entry is different from database row). The following fields 
have been changed [actual/expected value]: (jdo.TestPersistent).ctime: [Thu Sep 
16 12:53:44 CEST 2004/Thu Sep 16 12:53:44 CEST 2004]
        at org.exolab.castor.jdo.engine.SQLEngine.store(SQLEngine.java:930)
        at org.exolab.castor.persist.ClassMolder.store(ClassMolder.java:1613)
        at org.exolab.castor.persist.LockEngine.store(LockEngine.java:753)
        at org.exolab.castor.persist.TransactionContext.prepare
(TransactionContext.java:1552)
        at org.exolab.castor.jdo.engine.DatabaseImpl.commit
(DatabaseImpl.java:518)
        at jdo.Persistent.runTest(Persistent.java:174)
        at harness.CastorTestCase.run(CastorTestCase.java:154)
        at harness.TestHarness.run(TestHarness.java:140)
        at harness.TestHarness.run(TestHarness.java:140)
        at Main.run(Main.java:152)
        at Main.main(Main.java:226)
There was 1 failure:
1) [castor.oracle.TC66] Size
junit.framework.AssertionFailedError: Calling size() on a non-scrollable 
ResultSet should fail (unless using PostgreSQL).
        at jdo.Size.testSizeD(Size.java:184)
        at jdo.Size.runTest(Size.java:87)
        at harness.CastorTestCase.run(CastorTestCase.java:154)
        at harness.TestHarness.run(TestHarness.java:140)
        at harness.TestHarness.run(TestHarness.java:140)
        at Main.run(Main.java:152)
        at Main.main(Main.java:226)

FAILURES!!!
Tests run: 37,  Failures: 1,  Errors: 1


But this is exactly the same when running them against the unchanged CVS 
source, so this doesn't seem to be related. The error of test case 47 doesn't 
occure every time - the first time I run it today it didn't report a problem.

------- Additional Comment #12 From Werner Guttmann 2004-09-17 02:33 -------
Martin, thans for the patch. I have a look at some of the code, but as it
duplicates most of bug 1732, I'd rather wait until bug 1732 has been committed,
which shouldn't be too far in the future.

Can I please ask you to bear with me for the time being, and possibly m aybe
supply a new patch once bug 1732 has been committed.

------- Additional Comment #13 From Bruce Snyder 2004-09-19 20:18 -------
I like the DbMetaInfo object, but I wonder if we could formalize it a bit more?
The reason I'm asking is because it would be nice to be able to house relational
database metadata or LDAP metadata in the *MetaInfoImpl object. 

------- Additional Comment #14 From Martin Fuchs 2004-09-20 03:31 -------
Sure, DbMetaInfo can be extended to expose more than just the (JDBC) database 
version. For example there are functions like nullPlusNonNullIsNull() or 
supportsOrderByUnrelated() in DatabaseMetaData.

I don't know anything about using LDAP.
What LDPA properties would you like to expose?
Is there also a database version to query?

------- Additional Comment #15 From Werner Guttmann 2004-09-22 07:35 -------
Bruce, please feel free to add a NEW bug report, as this bug is in support of 
bug 1732. And as we have (internally) agreed upon new release for Nov 7th, 
2004, I am sure you'll understand. Unless of course you happen to have some 
more time to provide a patch yourself .. ;-).

------- Additional Comment #16 From Werner Guttmann 2004-09-27 14:02 -------
Martin, can you please re-post your patch after you've updated your code from
the CVS server, as I've just committed the changes for 1732. Pretty please ?

------- Additional Comment #17 From Martin Fuchs 2004-09-27 15:25 -------
Created an attachment (id=699)
updated patch

------- Additional Comment #18 From Martin Fuchs 2004-09-27 15:27 -------
The new patch now applies to the current CVS.
It also contains some minor import and comment cleanups.

------- Additional Comment #19 From Werner Guttmann 2004-09-28 02:36 -------
Committed patch as provided. Please don't forget to close this bug once you've
asserted its correctness.

------- Additional Comment #20 From Martin Fuchs 2004-09-28 03:36 -------
There is a small typo in the comment to JDBCQueryExpression.getStatement():

Index: src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java
===================================================================
RCS file: /cvs/castor/castor/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExp
ression.java,v
retrieving revision 1.6
diff -u -p -d -w -b -r1.6 JDBCQueryExpression.java
--- a/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java   28 Sep 2
004 09:23:11 -0000      1.6
+++ b/src/main/org/exolab/castor/jdo/drivers/JDBCQueryExpression.java   28 Sep 2
004 10:35:07 -0000
 at  at  -363,7 +363,7  at  at  public class JDBCQueryExpression

     /**
         * Creates a SQL statement.
-     * In genral, for a RDBMS/JDBC driver with a full support of the SQL 
standard/JDBC
+     * In general, for a RDBMS/JDBC driver with a full support of the SQL 
standard/JDBC
         * specification, this will return a valid SQL statement. For some 
features,
         * a particular RDBMS might indidate that it doe snot support this 
feature by
         * throwing a SyntaxNotSupportedException.

Anything other is OK. So Closed.

------- Additional Comment #21 From Werner Guttmann 2004-09-28 04:35 -------
Fixed. Thanks .. ;-).

     Query page      Enter new bug
This is Bugzilla: the Mozilla bug system. For more information about what Bugzilla is and what it can do, see bugzilla.org.
Actions: New | Query | bug # | Reports   New Account | Log In