History | Log In     View a printable version of the current page.  
Issue Details (XML | Word | Printable)

Key: CASTOR-872
Type: Improvement Improvement
Status: Closed Closed
Resolution: Fixed
Priority: Minor Minor
Assignee: Werner Guttmann
Reporter: Martin Fuchs
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
castor

Look at database versions to determine specifically supported features

Created: 10/Sep/04 01:11 PM   Updated: 19/Apr/05 10:52 AM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Text File castor-1733.patch (20 kb)
2. Text File castor-1733b.patch (20 kb)
3. Text File castor-DbMetaInfo.patch (34 kb)

Environment:
Operating System: All
Platform: All

Bugzilla Id: 1733


 Description  « Hide
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.



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Werner Guttmann - 10/Sep/04 01:40 PM
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 ...


Werner Guttmann - 10/Sep/04 01:42 PM
Just an observation: JDBCQueryExpression does NOT implememt QueryExpession,
though it should (I assume)

Bruce Snyder (bugzilla) - 13/Sep/04 09:12 PM
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.

Werner Guttmann - 14/Sep/04 01:28 AM
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.

Martin Fuchs - 14/Sep/04 12:51 PM
> 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.


Martin Fuchs - 15/Sep/04 01:43 AM
Register dependency on Bug 1732

Martin Fuchs - 15/Sep/04 01:51 AM
Created an attachment (id=664)
proposed patch

Martin Fuchs - 15/Sep/04 02:00 AM
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.

Werner Guttmann - 15/Sep/04 02:11 PM
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.


Martin Fuchs - 16/Sep/04 03:53 AM
Created an attachment (id=668)
updated patch with DBMetaInfo member in JDBCQueryExpression

Martin Fuchs - 16/Sep/04 04:07 AM
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.


Werner Guttmann - 17/Sep/04 02:33 AM
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.


Bruce Snyder (bugzilla) - 19/Sep/04 08:18 PM
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.

Martin Fuchs - 20/Sep/04 03:31 AM
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?


Werner Guttmann - 22/Sep/04 07:35 AM
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 .. .

Werner Guttmann - 27/Sep/04 02:02 PM
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 ?

Martin Fuchs - 27/Sep/04 03:25 PM
Created an attachment (id=699)
updated patch

Martin Fuchs - 27/Sep/04 03:27 PM
The new patch now applies to the current CVS.
It also contains some minor import and comment cleanups.

Werner Guttmann - 28/Sep/04 02:36 AM
Committed patch as provided. Please don't forget to close this bug once you've
asserted its correctness.

Martin Fuchs - 28/Sep/04 03:36 AM
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.


Werner Guttmann - 28/Sep/04 04:35 AM
Fixed. Thanks .. .