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

Key: CASTOR-850
Type: Bug Bug
Status: Open Open
Priority: Major Major
Assignee: Unassigned
Reporter: Martin Fuchs
Votes: 0
Watchers: 1
Operations

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

master detail OQL queries - explicit sort order for SQL queries with joined tables

Created: 06/Aug/04 01:59 PM   Updated: 22/Dec/06 04:48 PM
Component/s: JDO
Affects Version/s: 0.9.5.3
Fix Version/s: None

Time Tracking:
Not Specified

File Attachments: 1. Java Source File A.java (0.1 kb)
2. Java Source File B.java (0.1 kb)
3. Text File castor-1711-850-1.1.patch (2 kb)
4. XML File jdo-conf.xml (0.4 kb)
5. XML File mapping.xml (0.9 kb)
6. Java Source File OrderTest.java (3 kb)

Environment:
Operating System: All
Platform: All

Bugzilla Id: 1711


 Description  « Hide
While testing an application we had to see, Castor has sometimes problems to
read objects with master-detail connections. It doesn't find all children
records in arrays, vectors or any other collections.
I traced down this problem to the functions nextIdentity() and fetchRaw() in
class SQLEngine. This functions expect datasets with equal identity keys
(e.g. "PKA") to be fetched in continuous order.

But this guaranteed for SQL statements like:

select A.PKA, B.PKB
from A, B
where A.PKA = B.PKA

This is at least the behaviour of Oracle databases.
I am not sure about other systems.
So I implemented the fix only for the class OracleQueryExpression.
May be other DBs also they need a similar fix.

The attached patch adds an explicit explicit sort order using the identity
columns when joining tables to make dataset order predictable.
If there is specified an other ORDER BY expression, this is not changed.



 All   Comments   Work Log   Change History      Sort Order: Ascending order - Click to sort in descending order
Martin Fuchs - 06/Aug/04 02:00 PM
Created an attachment (id=615)
Patch for src/main/org/exolab/castor/jdo/drivers/OracleQueryExpression.java

Bruce Snyder (bugzilla) - 02/Sep/04 08:05 PM
Martin, do you have any kind of a test case to prove this problem? If so, please
post it.

Martin Fuchs - 03/Sep/04 12:02 AM
The problem is, you can't reproduce the missing order reliably.
However you can try if you see the behaviour using this SQL script:

drop table B
/
drop table A
/

create table A(NR_A number)
/
ALTER TABLE A ADD CONSTRAINT PKA PRIMARY KEY(NR_A)
/

create table B(NR_A number, NR_B number)
/
ALTER TABLE SCOTT.B ADD CONSTRAINT FK_A_B
FOREIGN KEY(NR_A) REFERENCES A(NR_A)
/

insert into A(NR_A) values(1)
/
insert into A(NR_A) values(2)
/
insert into A(NR_A) values(3)
/
insert into A(NR_A) values(4)
/
insert into A(NR_A) values(5)
/
insert into A(NR_A) values(6)
/
insert into A(NR_A) values(7)
/
insert into A(NR_A) values(8)
/
commit
/

insert into B(NR_A, NR_B) values(2, 1)
/
insert into B(NR_A, NR_B) values(2, 2)
/

insert into B(NR_A, NR_B) values(1, 1)
/
insert into B(NR_A, NR_B) values(2, 3)
/
insert into B(NR_A, NR_B) values(3, 2)
/
insert into B(NR_A, NR_B) values(3, 3)
/
insert into B(NR_A, NR_B) values(3, 1)
/

insert into B(NR_A, NR_B) values(1, 2)
/
insert into B(NR_A, NR_B) values(1, 3)
/

commit
/

select A.NR_A, B.NR_B
from A, B
where A.NR_A = B.NR_A
– no ORDER BY
/

What I get as result of the SELECT is for example:

NR_A NR_B
2 1
2 2
1 1 <-
2 3
3 2
3 3
3 1
1 2
1 3

Note the marked data set 1/1. It is fetched between 2/2 and 2/3.
But Castor would expect it in continuous order with 1/2 and 1/3.

When inserting an ORDER BY clause a Castor-compatible result set is fetched:

select A.NR_A, B.NR_B
from A, B
where A.NR_A = B.NR_A
order by A.NR_A
/

NR_A NR_B
1 1
1 2
1 3
2 1
2 3
2 2
3 2
3 3
3 1


Martin Fuchs - 03/Sep/04 04:05 AM
If you compute statistics for table A the behaviour can be reproduced more
reliable:

analyze table A compute statistics
/


Martin Fuchs - 03/Sep/04 04:31 AM
Created an attachment (id=640)
OrderTest.java

Martin Fuchs - 03/Sep/04 04:31 AM
Created an attachment (id=641)
A.java

Martin Fuchs - 03/Sep/04 04:31 AM
Created an attachment (id=642)
B.java

Martin Fuchs - 03/Sep/04 04:32 AM
Created an attachment (id=643)
jdo-conf.xml

Martin Fuchs - 03/Sep/04 04:32 AM
Created an attachment (id=644)
mapping.xml

Martin Fuchs - 03/Sep/04 04:36 AM
The attached test program queries the tables A and B both using Castor and
plain JDBC.

With the unpatched Castor version (that of 05.08.2004 in CVS), I can see the
problem. It doesn't report correct object counts for the OQL selects:

A-Sum [Castor]: 10 (should be the same as the following number)
A-Sum [SELECT]: 8
B-Sum [Castor]: 9 (should be the same as the following number)
B-Sum [SELECT]: 15

With my patch the last output lines are correct:

A-Sum [Castor]: 8 (should be the same as the following number)
A-Sum [SELECT]: 8
B-Sum [Castor]: 9 (should be the same as the following number)
B-Sum [SELECT]: 9


Werner Guttmann - 03/Sep/04 04:42 AM
Excuse my ignorance, but what if somebody specified an ORDER BY clause that
would prevent the (patched) code from performing successfully, iow an ORDER BY
clause that isn't 'Castor-compatible' ?

Bruce Snyder (bugzilla) - 03/Sep/04 04:52 AM
Thanks for the test. This is exaclty what I needed. I was never quite able to
figure out why results.size() was incorrect. I always knew that it was giving a
total number of objects found in the graph but I didn't go far enough to see
that the joins were affecting it in this way.

My only question is exactly what Werner just asked. I tried to commit this a
moment ago and Bugzilla reported a Mid-Air Collision because the bug had been
updated while I was typing these comments.


Martin Fuchs - 03/Sep/04 05:41 AM
> Excuse my ignorance, but what if somebody specified an ORDER BY clause that
> would prevent the (patched) code from performing successfully, iow an ORDER BY
> clause that isn't 'Castor-compatible' ?

Yes, this is still a problem. If the user specifies an order clause, it has to
include the identity columns of the master table as first sort criterium. One
solution would be to report an error, if this is not the case.
Of course the best solution would be to fix the algorithm in SQLEngine. But I
fear, this isn't an easy task. For example it would imply to resize arrays on
the fly when detecting more objects for an identity processed previously.
Ehm - I hope, I am clear enough, and you can understand, what I want to says
with this.


Martin Fuchs - 10/Sep/04 11:55 AM
Another problem, which is not handled quite satisfactory by my proposed patch
(It's more a work around than a fire and forget solution), is the handling of
database locks:
  • sql.append( " FOR UPDATE" );
    + if (!use_order_by) // Locking by "FOR UPDATE" cannot be used with
    ORDER BY at the same time.
    + sql.append( " FOR UPDATE" );
    +

I chose to ignore the lock request.
Another way would be to throw an exception, because locks using "FOR UPDATE" in
Oracle queries are not compatible to the use of an ORDER BY clause. But this
would disallow any locks for OQL queries with master/detail data.

Perhaps we should better fix SQLEngine algorithm (at least as a long term
solution). I might give it a try if I find some time to look deeper into this.
However the fix already posted is better then nothing and might save a few
people data damage.


Martin Fuchs - 16/Sep/04 07:20 AM
There would be at least two different ways to come up with a solution without
adding additional ORDER BY clauses to the generated SQL:
  • "lazy loading" of detail data:
    Don't use outer joins to fetch master and detail data at once.
    Of course this would decrease the performance of SQL queries since we need to
    execute many more queries: one for the master table and as much for the detail
    table as there are identities in the master table.
  • fetch all data of the unchanged outer join queries at once:
    Currently the result objects are fetched once by once when calling
    QueryResults.hasMore() or QueryResults.next(). If all objects would internally
    be fetched at once, this would make possible to collect any detail data
    relating to the master data objects without missing incomming un-ordered SQL
    datasets. The resulting objects could be moved into the cache while reading the
    result set, however it has to be guarantied, all objects have space there.
    Sicne this solution could be a very memory consuming task, it should only be
    used for the case of 1:n queries. The algorithm for simple queries could remain
    unchanged.

Any better ideas?


Werner Guttmann - 23/Sep/04 03:00 PM
I do not know whether you've all seen Martin's comments above, and I am sure we
have already seen similar threads at this very place .. but I'd like to see
comments. And anyhow, does anybody know any bug numbers out there that we can
link to this one ?


Bruce Snyder (bugzilla) - 14/Oct/04 09:04 PM
As you state, Martin, one solution is to fetch all data using multiple queries
or using one great big query with some pretty awful joins. For a long time I've
been curious to investigate offering users a configurable option of which of
these strategies to use. Some people don't mind multiple queries while others
do. And different situations influence this issue differently. What about
offering this by way of a property in the castor.properties file? There is
really no easy answer to this issue. It's similar to the LIMIT/OFFSET isssue
with Oracle where we finally just agreed to support that feature only in
Oracle 8.1.6 +. Comments?

Werner Guttmann - 16/Oct/04 02:29 AM
I think that's the right way to go about it, by making it configurable. Though I
do not agree that castor.properties is the right place to configure this. Imho,
a more granular approach is required. Why not overload (again?) the various
persistence-related methods of Database (as well) ? Iow, allow users to
configure a default mode in castor.properties, and give them means to override
this default on a per 'query' base. Hmm ?

Martin Fuchs - 16/Oct/04 03:08 AM
Yes, creating a new configuration switch is the way to make all users happy.
But of course it will increase the complexity even more. You will have to
implement and maintain both possibilities. I also think to introduce this
switch on a per query base is more convenient. This enables to optimize the
performance for the particular cases. If we add such a switch I would vote to
be the method "lazy loading of detail data" the default. This is the most
general approach. Querying all data of a complete table at once can use an
unpredictable amount of memory and can take quite long, depending on the
available data.

So what would be the allowed values of this new switch?
Let me enumerate all methodes describes in the comments above:

enum DETAIL_DATA_METHOD {
DDM_LAZY_LOAD, // fetch master data one by one in next() calls, with
relaited detail data
DDM_ALL_AT_ONCE, // fetch all data in a single query at once, and process
all at the execute() call
DDM_FORCE_ORDER // sort data using the identity of the master data to get
all related detail data in sequence
}

The "all at once" and "force order" methods could be combined. However we
should not create even more possibilities, and confain them to a meaningfull
subset.


Werner Guttmann - 16/Oct/04 03:44 AM
Let me just have a look at the way passing access modes to e.g. Database.load()
is handled. As far as I remember, there's two options with this. Either specify
an access mode in the mapping file (access="read-only") or specify one at the
method level. Bruce, correct me if I am wrong.

Martin Fuchs - 30/Oct/04 03:33 AM
      • Bug 986 has been marked as a duplicate of this bug. ***

Werner Guttmann - 13/Oct/05 09:59 AM
Martin, just wondering whether you would want to progress this any further ?

Martin Fuchs - 22/Dec/06 04:47 PM
Update patch to current (Castor 1.1) source.