Bug #10285
Static compound query returns no result
100%
History
#1 Updated by Andrei Plugaru about 1 year ago
While investigating #8279, regarding some CQ instances, I found an example that doesn't seem to work properly in FWD:
def VAR hQuery as handle.
CREATE QUERY hQuery.
hQuery:SET-BUFFERS(BUFFER table1:HANDLE, BUFFER table2:HANDLE).
hQuery:QUERY-PREPARE('FOR EACH table1, each table2 where table2.id = 1').
hQuery:QUERY-OPEN().
message 'dyanmic query:'.
do while hQuery:GET-NEXT():
message table1.id table1.c table2.id. // FWD and OE: 1 'a' 1
end.
release table1.
release table2.
message 'static query:'.
FOR EACH table1, each table2 where table2.id = 1:
message table1.id table1.c table2.id. // OE: 1 'a' 1; FWD: nothing
end.
Here I have the same query, however in the 1st case it is dynamic and in the 2nd case it is static. The problem occurs only in the static case. Another important aspect is that the issue is only for persistent tables, for temporary tables I got the right behaviour in both cases.
#2 Updated by Andrei Plugaru about 1 year ago
I have found out that the issue is only reproducible when using the MariaDB driver. I have tracked the difference to ScrollableResults.getRowNumber. There, we are calling getRow() on the result set. The exact result set implementation when MariaDB is used is StreamingResult. However, it seems that that method always returns 0 if the result set type is TYPE_FORWARD_ONLY as it is the case in my example.
#3 Updated by Andrei Plugaru 12 months ago
- Status changed from New to WIP
- Assignee set to Andrei Plugaru
I think I understand the root cause of the issue.
In FWD, we already have a wrapper for the forward only result sets: ForwardResults, however it is not used in this case. The reason seems to be that in PreselectQuery, the conditions for creating a forward only result sets differ from the conditions of using a ForwardResults vs ScrollingResults. In PreselectQuery, the only condition for creating a forward only result set is that the query is not scrolling. However, the condition for creating a ForwardResults is more complex:
!isScrolling() && isIterating() && persistence.getDialect().supportsJdbcApi(Dialect.API_LAST_ON_FORWARD_ONLY)
In my case isIterating() returns false, so a ScrollingResults is created instead of ForwardResults. However, the backing JDBC result set is forward only as the query is not scrolling.
I think the conclusion is to get in sync the conditions for creating a ForwardResults and a forward only result set.
#4 Updated by Andrei Plugaru 12 months ago
- Status changed from WIP to Review
- reviewer Ovidiu Maxiniuc added
I have committed 10285a/16085. My changes make that FORWARD-ONLY result sets are created only if this entire condition is positive: !isScrolling() && isIterating() && persistence.getDialect().supportsJdbcApi(Dialect.API_LAST_ON_FORWARD_ONLY). Basically, now it will match the condition for creating a ScrollableResults. Up until now, we could have had a FORWARD-ONLY result set, but with ScrollableResults. IMHO, this is wrong from a very simple reason:
in ScrollableResults we can call previous, which shouldn't be allowed for a FORWARD_ONLY result set.
With my change, I have run the test case from #10285-1 and #8971-1 with MariaDB, and no longer got the error caused by the getRowNumber method. I have also run the fwd tests of a large customer application which uses MariaDB and got no regressions.
Ovidiu, I saw you recently also had changes in this area, so I think you are the best to review my changes.
#5 Updated by Ovidiu Maxiniuc 12 months ago
- Status changed from Review to Internal Test
Yes, I think you are right. Your reasoning seems sound to me.
Let's get this into tests with the other testsets. Please update the %Done
#6 Updated by Andrei Plugaru 12 months ago
- % Done changed from 0 to 100
This is the testing plan I am currently running:
- unit tests + fwd tests of a large customer application (ap); ✅
- unit tests of a large gui application (ls); ✅
- harness + reports of a large customer application (ls); ✅
Ovidiu, feel free to add anything if you think it is necessary.
#10 Updated by Ovidiu Maxiniuc 12 months ago
These should have covered the affected code.
I think the branch can be merged into trunk.
#11 Updated by Alexandru Lungu 11 months ago
- Status changed from Internal Test to Merge Pending
Please merge 10285a to trunk now.