FWD » Core Development » Database

I did not found any tests that use forward-only so I created a test suite with 12 examples. From the 12 tests mentioned, 2 were missing an error but had correct results and 2 displayed the wrong error and wrong results. I am currently investigating the test scenario where doing GET-NEXT and REPOSITION-BACKWARD(1) causes the record to be unavailable, but it should be available since reposition should not happen on a non-scrolling query.

Committed 7496a/rev.14650. Reposition can be done only for non-scrolling queries. From the test suite I noticed that by setting the forward-only attribute to true a scrolling query will become a non-scrolling query and only non-scrolling queries can use reposition.

• Dynamic queries are scrolling by default, if the forward-only flag is set to false, the scrolling value should not be modified at all (omitted in the commit).
• Write test cases for reposition by rowid.
• The documentation says that the forward-only attribute can't be set while a query is open or is being browsed. In this case we can use a query and close it, then set the attribute before opening and using the query again. We might want to keep track if the query can be scrolling from the start so that we can have it as such when possible.

The documentation says that the forward-only attribute can't be set while a query is open or is being browsed. In this case we can use a query and close it, then set the attribute before opening and using the query again. We might want to keep track if the query can be scrolling from the start so that we can have it as such when possible.

Danut, please double check 4GL if it actually remembers that the query was scrolling in the first place. Therefore, check if after close / set forward-only on false / open, 4GL sets the query back on scrolling.

Alexandru Lungu wrote:

Danut, please double check 4GL if it actually remembers that the query was scrolling in the first place. Therefore, check if after close / set forward-only on false / open, 4GL sets the query back on scrolling.

It does set it back to scrolling.

Ok! Danut please take the extra mile to ensure that ResultSet.FORWARD_ONLY is used if query:forward-only is set. Therefore, please check all kind of results we have back there and make sure they are honoring ResultSet.FORWARD_ONLY.
Coordinate with the effort in #7241-113 (onward); Stefan is attempting to refactor ProgressiveResults in order to fully support ResultSet.FORWARD_ONLY. Danut, your goal is just to bring the forwardOnly flag till the results class, so it can be picked up there. Currently, Stefan is hard-coding ResultSet.FORWARD_ONLY inside ProgressiveResults to test it.

Alexandru Lungu wrote:

Ok! Danut please take the extra mile to ensure that ResultSet.FORWARD_ONLY is used if query:forward-only is set. Therefore, please check all kind of results we have back there and make sure they are honoring ResultSet.FORWARD_ONLY.
Coordinate with the effort in #7241-113 (onward); Stefan is attempting to refactor ProgressiveResults in order to fully support ResultSet.FORWARD_ONLY. Danut, your goal is just to bring the forwardOnly flag till the results class, so it can be picked up there. Currently, Stefan is hard-coding ResultSet.FORWARD_ONLY inside ProgressiveResults to test it.

Committed 7496a/rev.14651. Improved the support for the FORWARD-ONLY attribute. I managed to make it available in PreselectQuery and added an additional condition in AdaptiveQuery.executeQuery which will eventually create ProgressiveResults instead of ScrollingResults.

The only thing to do that is left is to throw an error when using BROWSE with a forward-only query and finish up the test suite. Another important thing to note a query that includes a BREAK BY phrase becomes a FORWARD-ONLY query (as specified in the documentation), but support for this is not necessary since using BREAK BY will generate a PresortQuery with ResultSet.TYPE_FORWARD_ONLY by default.

• Between 7421a/rev.14649 and 7421a/rev.14651, the following error occurs when defining a query for a browse with FORWARD-ONLY attribute set to true: BROWSE requires the QUERY to be DEFINEd SCROLLING. (3330) which is correct since a scrolling query will become non-scrolling if this flag is set to true.
• In 4GL, setting the FORWARD-ONLY attribute to either true or false will result in this error being thrown: Forward only status may not be changed while query q is open or being browsed. (12354) followed by Unable to set attribute FORWARD-ONLY in widget of type QUERY. (3131).. When eliminating the DEFINE BROWSE which uses the query, these errors do not appear. In FWD, the previous error is replaced by error 3330, but only when FORWARD-ONLY is true.

Apart from the points mentioned above, I don't think there is a huge problem. The only way for error 12354 and 3131 to be thrown in FWD is by setting the forward-only attribute when the query is open. I see that there is a QueryWrapper.browsed flag available that can be used for this case.

In FWD, FORWARD-ONLY is by default false, which makes it possible to create a query which will set the value of the attribute to false and still be able to create a BROWSE with that query. This happens because the QueryWrapper.browsed value is set later, when the query is attached to the browse, after it is opened (please correct me if I am wrong).

While testing, I stumbled upon an issue related to GET PREV and GET LAST where after creating a few records and opening a query with forward-only enabled, will cause the GET LAST to throw an error (correct) and if you try to message data from the buffer it will manage to print it. This does not happen in 4GL, once the error is thrown, no records are found. I will take a look and create a separate issue for this after checking if it was not already reported.

Committed 7496a.rev/14652. Since the forwardOnly flag might be useful in the future in other queries, I added it as a member to AbstractQuery and added the required methods to use it. I also added the missing BROWSE errors mentioned above.

Now I will test the changes on a customer application and add my test suite to testcases. I will follow up with an update after I am done.

I didn't find any problem while testing a customer application.

Committed testcases/rev.2380. Added FORWARD-ONLY tests.

Committed 7496a/rev.14653. Quick changes made based on the #7496-12 review. Similar to PreselectQuery, I removed the changes and logs from AdaptiveQuery.

Alexandru Lungu wrote:

I think that we can't rely on QueryWrapper to properly do the propagation in the delegates. I mean it is fine that it does this whole state transitions from scrolling to non-scrolling / forward-only to non-forward-only and vice-versa. However, we need to replicate this behavior in the underlying queries. Consider changing PreselectQuery.isScrolling implementation (and other isScrolling implementations) to (scrolling && !typeForwardOnly) || isNativelyScrolling(). This way, we propagate TYPE_FORWARD_ONLY implicitly to the Results, so this is fine.

I made a few changes to allow the transition between scrolling states in 7496a/rev.14654. I also tested 1/4 of my test suite independently and a customer application. Everything works fine and the results of the test suite were all correct, with the exception of one which turned from a No record available error to the record hold by the buffer after creating all the records (which is already an existent problem) and I still need to look into.

Review of 7496a

• I don't think setScrolling should be called when calling setForwardOnly. This will make all queries that are forward-only scrolling - this is not good.
• When setting isForwardOnly in AbstractQuery (or overrides):
  • on true: if the query is scrolling, the cursor should be dropped.
  • on false: if the query is scrolling, the cursor should be created (but only if the query was "originally" scrolling.

Danut, please check what happens if you actually do first, prev or last when type-forward only is set (in 4GL and FWD). I want just to make sure we do it right even for these "non-reposition" cases. I see that first is simply calling delegate's first, which calls results' first (which may be incorrect due to forward only flag. Mind that Stefan has an implementation of results that logs if such attempts are made, but we should ensure they are captured earlier to throw proper 4GL exceptions.

Just as a fact: in ForwardResults (from #7241), first and last are supported as long as first can be done in special conditions (we are still before first) and last can be always done.

Alexandru Lungu wrote:

Danut, please check what happens if you actually do first, prev or last when type-forward only is set (in 4GL and FWD). I want just to make sure we do it right even for these "non-reposition" cases. I see that first is simply calling delegate's first, which calls results' first (which may be incorrect due to forward only flag. Mind that Stefan has an implementation of results that logs if such attempts are made, but we should ensure they are captured earlier to throw proper 4GL exceptions.

I tested the following cases with FORWARD-ONLY=TRUE and both 4GL and FWD showed the same results:
- GET-FIRST: retrieves the first record correctly;
- GET-NEXT > GET-FIRST: Shows error 12394;
- GET-LAST: Shows error 12393;
- GET-NEXT > GET-LAST: Shows error 12393;
- GET-PREV: Shows error 12393;
- GET-NEXT > GET-PREV: Shows error 12393;

With FORWARD-ONLY=FALSE, 4GL and FWD again showed the same results:
- GET-FIRST: retrieves the first record;
- GET-NEXT > GET-FIRST: retrieves the first record;
- GET-LAST: retrieves the last record;
- GET-NEXT > GET-LAST: retrieves the last record;
- GET-PREV: retrieves the previous record (the last record created, buffer was released);
- GET-NEXT > GET-PREV: no record available after GET-NEXT;

I am currently working on #7496-16, originalScrolling is only available in QueryWrapper, but it should be made possible for other queries to make use of it through AbstractQuery. Some changes in 7496a/rev.14654 will be reverted, especially those that modified the functionality of the setScrolling method.

That is great - please ensure that #7496-18 still holds after refactoring. Also, do some skim debugging on more complex cases:

• scrolling -> FORWARD-ONLY true -> FORWARD-ONLY false -> open (should be scrolling) -> close -> FORWARD-ONLY true -> open (should not be scrolling)
• scrolling -> FORWARD-ONLY true -> FORWARD-ONLY false -> open (should be scrolling) -> close -> FORWARD-ONLY false -> open (should be scrolling)
• scrolling -> FORWARD-ONLY true -> FORWARD-ONLY true -> open (should not be scrolling) -> close -> FORWARD-ONLY true -> open (should not be scrolling)
• scrolling -> FORWARD-ONLY true -> FORWARD-ONLY true -> open (should not be scrolling) -> close -> FORWARD-ONLY false -> open (should be scrolling)
• scrolling -> FORWARD-ONLY false -> FORWARD-ONLY false -> open (should be scrolling) -> close -> FORWARD-ONLY true -> open (should not be scrolling)
• scrolling -> FORWARD-ONLY false -> FORWARD-ONLY false -> open (should be scrolling) -> close -> FORWARD-ONLY false -> open (should be scrolling)
• scrolling -> FORWARD-ONLY false -> FORWARD-ONLY true -> open (should not be scrolling) -> close -> FORWARD-ONLY true -> open (should not be scrolling)
• scrolling -> FORWARD-ONLY false -> FORWARD-ONLY true -> open (should not be scrolling) -> close -> FORWARD-ONLY false -> open (should be scrolling)

By "should be scrolling" I mean that isScrolling should return true. scrolling variable can be true even if isScrolling return false (as it represents the "original status of the query). Also, cursor != null should always hold when the query is truly scrolling (isScrolling is true).

Danut, please do your best to get this done asap. I intent to have it merged intro trunk by the end of this week, so we can open new tasks of adapting this new mode to new use-cases: FOR EACH loops, honoring in different kind of results, etc.

Alexandru Lungu wrote:

Danut, please do your best to get this done asap. I intent to have it merged intro trunk by the end of this week, so we can open new tasks of adapting this new mode to new use-cases: FOR EACH loops, honoring in different kind of results, etc.

Committed 7496a/rev.14655. I've previously mentioned that other queries can make use of originalScrolling value through AbstractQuery but I made a mistake, it isn't necessary to use it's value at all and it should not even exist! Instead of doing a complicated process of toggling scrolling when setting FORWARD-ONLY to true/false, we can simply use an additional condition when calling isScrolling() (which was added previously and completed in 7496a/rev.14654).

• removed QueryWrapper.originalScrolling
• Always delegate forwardOnly value when opening the query
• removed the delegation process for scrolling when setting FORWARD-ONLY
• Reverted all the changes to PrelectQuery in 7496a/rev.14654, including history entries.
• Added setForwardOnly() to DynamicQuery and AdaptiveQuery in order to reset the cursor or disable it if necessary

All the tests mentioned in #7496-19 pass with these changes. I still need to test a customer application right now and check if everything works properly with the previous test suite.

Please review.

I previously tested a large customer application using the changes from 7241b/rev.14655, 7496a/rev.14655 and a few changes to make the actual call that generates ForwardResults. A total of 2686 ForwardResults instances were created and the application ran without any problems. I've experimented with the example provided in #7241-133 and created smaller tests. SessionListener class mentioned in #7241-138 is exactly what was missing from the implementation, SessionListener.Event.TRANSACTION_COMMITTING exists and it is handled but this event is never notified. By notifying this event in Persistence.Context.commit(), I got better results between FWD and 4GL (previously 2/4 showed No ... record is available, now all results match).

=== modified file 'src/com/goldencode/p2j/persist/Persistence.java'
--- old/src/com/goldencode/p2j/persist/Persistence.java    2023-05-23 11:54:21 +0000
+++ new/src/com/goldencode/p2j/persist/Persistence.java    2023-08-07 13:55:29 +0000
@@ -4778,6 +4778,11 @@

          try
          {
+            if (!sessionListeners.isEmpty())
+            {
+               notifySessionEvent(SessionListener.Event.TRANSACTION_COMMITTING);
+            }
+            
             session.commit();

             if (LOG.isLoggable(Level.FINER))

I will retest the customer application with a few changes to generate more ForwardResults instances. I did not check how many No ... record is available errors might have been caused by ForwardResults since I only looked for SEVERE log types related to it.

Danut, please find a concrete example where 7496a (together with 7241b) is failing before SessionListener.Event.TRANSACTION_COMMITTING and is fixed now. My point is that the PG server-cursor should have thrown something like "Portal ... does not exist". After the changes, it shouldn't be thrown. Also, please double check that the "implicit transaction" is created properly and commit is iterating all results till the end to fetch the remaining records and avoid PG server-cursor invalidation.

I agree that No ... record is available may have been caused by "Portal ... does not exist" in the first place. If this is the case, ignore my first comment; just double-check.

As for PreselectQuery, please use your type-forward-only tests to see if PRESELECT influences the forward-only and scrolling is any way, especially according to #7496-19. My final concern is if that FOR EACH is different from PRESELECT in regard to SCROLLING and FORWARD-ONLY behavior. If they are the same, I think we can remove isNativelyScrolling. Is a PRESELECT query that is defined as non-scrolling made automatically SCROLLING, just because it is preselect?

All queries should emit TYPE_SCROLL_INSENSITIVE (no matter if in 4GL we have SCROLLING or not). Only queries that are explicitly set with FORWARD-ONLY should emit TYPE_FORWARD_ONLY and determine PG server-cursor. Also, queries that are TYPE_FORWARD_ONLY should always use ForwardResults, while TYPE_SCROLL_INSENSITIVE should use ScrollingResults. Both wrap ScrollableResults and can be wrapped by a ProgressiveResults.

Danut, please port the changes from 7241b into 7496a, so I can dead-archive 7241b. Also, commit the changes from #7496-22 into 7496a. The goal here is to have only 7496a fully ready to fix FORWARD-ONLY (including implicit transactions, SCROLLING and ForwardResults).

Once completely finished, let Stefan test the changes as he already has a testing scenario already set-up form #7241. Stefan, consider testing 7496a when ready over the trunk (without 7241c).

1. TemporaryBuffer does not have a case for TRANSACTION_COMMITTING in sessionEvent(), but other implementations eventually call cleanup()/sessionClosing() and releaseSession() which will decrease the value of Persistence.sessionUseCount. This value is decreased once for each commit, but notifying multiple listeners can decrease it even further.
2. When the session event is processed, it will close the results too early and will throw the following error:
   

   org.h2.jdbc.JdbcSQLNonTransientException: The object is already closed [90007-200]
           at org.h2.message.DbException.getJdbcSQLException(DbException.java:505)
           at org.h2.message.DbException.getJdbcSQLException(DbException.java:429)
           at org.h2.message.DbException.get(DbException.java:205)
           at org.h2.message.DbException.get(DbException.java:181)
           at org.h2.message.DbException.get(DbException.java:170)
           at org.h2.jdbc.JdbcResultSet.checkClosed(JdbcResultSet.java:3252)
           at org.h2.jdbc.JdbcResultSet.next(JdbcResultSet.java:133)
           at com.goldencode.p2j.persist.orm.ScrollableResults.execute(ScrollableResults.java:440)
           at com.goldencode.p2j.persist.orm.ScrollableResults.next(ScrollableResults.java:167)
           at com.goldencode.p2j.persist.ScrollingResults.next(ScrollingResults.java:156)
   

Committed 7496a/rev.14656. I brought the changes made by Stefan in #7241 (7241b) to 7496a.

I've done tests with a customer application and there are still problems when using ForwardResults and TRANSACTION_COMMITTING. Initially I had problem with the results being closed to early or with record hydration, but now I get a overlapping recids when trying to navigate the application basic pages. It logs a SEVERE error which doesn't actually shows the cause of the error, so I still need to investigate it.

I've tried to set and use isNativelyScrolling with false/true and most of my test suite runs properly with either with the exception of a single test that already has a wrong output in FWD compared to 4GL.

• Trying to display a record after a failed Cannot LAST or Prev Query q when FORWARD-ONLY ... shows No record available. The expected scenario is not showing anything if GET-LAST fails.
• Not releasing the buffer after creating records will keep the last record and will be made available even if GET-LAST fails due to using FORWARD-ONLY. (I think this is a separate issue and the test suite will be modified to release after creating the records)
• Additional error being displayed when using SCROLLING, FORWARD-ONLY and GET-FIRST on a PRESELECT EACH (even if the query uses forward-only, it should be able to retrieve precedent records due to it being preselect)
• Many error such as
  

  ** No tmp1 record is available. (91)
  ** No tmp1 record is available. (91)
  ? ?
  

  which do not display the ? ?, just an empty line. (These were not included in the 15 tests I am currently talking about, but the overall number of occurrences is greater and is very noticeable)
• A few more instances of wrong output that are similar to the ones mentioned above, but on a larger test which causes them to mix and not easily identify the problem.

Committed 7496a/rev.14705. After fixing #7737 (#7737-12) and investigating TRANSACTION_COMMITTING, I managed to fix 12 out of the 15 failing tests, now I am left with 3 tests that are not caused by the the current changes, some of which are mentioned in the related issue. The problem with using the TRANSACTION_COMMITTING event was that it was possible to delete a record in a temporary table, the results would be cached (including the deleted record which wasn't marked as removed). If you tried to create a new record, it would use the reclaimed recid and would be found in the cache when flushing. This should've been solved only for persistent tables, for which the initial testing was done.

The problems that I found while testing the customer application were solved. I will update the application to fix an existent regression and retest if necessary.

The changes should also be tested in a more detailed manner, most of the TRANSACTION_COMMITTING events that I tested were handled in the AdaptiveQuery.sessionEvent() method, while I rarely got to see the event being handled in PreselectQuery or never (Presorter).

Personal tests and customer application were tested previously. I've retested them just to make sure, but I was not successful with the customer application since it required a new setup. Currently I do not need to browse the application, but I had some problems with the incremental version since the archive was outdated (old tables were removed and new ones were added with the new database). The current problem is with the incremental version which I finally made it work, but even after making sure that name_map.xml is updated with the new converted files, FileSystemOps.searchPath() still can't find the file anywhere.

The other problem with the customer application is being addressed in #7710. I previously encountered 403 Forbidden (exactly like the Swing client in #7710-12), but after following the instructions in the issue I managed to display the window, only to be redirected to the login page right after closing it.

Rebased 7496a to trunk and is now at rev. 14732.

The current problem is with the incremental version which I finally made it work, but even after making sure that name_map.xml is updated with the new converted files, FileSystemOps.searchPath() still can't find the file anywhere.

Somehow the dumping works properly now, it finds all the files. I tried converting a test file for another issue which didn't work since it couldn't find it and tested the dump out of curiosity. I am currently dumping the database of a customer application and will return with the results after it is done.

I dumped the customer application database in 9 hours and 17 minutes. While monitoring the CPU and Memory using VisualVM, there was nothing unusual. For an hour, it was using between 2-4GB for small tables and max 6.6GB for large tables. I left the process to execute overnight and a total of 940 tables were dumped with ~110 million records dumped. The largest table had 17 million records and was dumped in 35 minutes. There were no PG portal related errors and the test was done using auto commit set to false.

Dănuț Filimon wrote:

I dumped the customer application database in 9 hours and 17 minutes. While monitoring the CPU and Memory using VisualVM, there was nothing unusual. For an hour, it was using between 2-4GB for small tables and max 6.6GB for large tables. I left the process to execute overnight and a total of 940 tables were dumped with ~110 million records dumped. The largest table had 17 million records and was dumped in 35 minutes. There were no PG portal related errors and the test was done using auto commit set to false.

Danut, 9h and a bit (#7241-140, #7241-164) is the baseline time without changes. I am quite surprised that with #7496 changes, you are facing the same dumping time. I will request a double check here. AFAIK, a dump with server-cursor was done before and achieved a really low time (#7241-92, but with lots of "temporary patches" both in 4GL and FWD).

Therefore, please do some System.out and debugging kind of job to check the server-cursor is actually used. It should: set auto-commit on false, results type set on TYPE_FORWARD_ONLY and a fetch-size of 256 (you can even test with more, like 1024). Anyway, you should definitely see some kind of improvement comparing with TYPE_SCROLL_INSENSITIVE.

If the second test fails, I suspect this is due to AdaptiveQuery still being used. However, PreselectQuery can't me made forward-only ... Still need to think about it. Good news is that the dumping still works with no regression.

The dump was done using TYPE_SCROLL_INSENSITIVE and ForwardResults wasn't used, I had to modify he original dump file to use a dynamic query with FORWARD-ONLY instead of a simple FOR EACH. I will retest the dump now.

Danut, as a debugging hint, I downloaded the PG driver sources and debugged them when ResultSet.next was called. This way, I could identify in rows how many rows were actually fetched from the database. If the database cursor is used, this rows has only <256 records at once. If the server-cursor is not used, the ResultSet may have thousand of records at once.

Alexandru Lungu wrote:

Danut, as a debugging hint, I downloaded the PG driver sources and debugged them when ResultSet.next was called. This way, I could identify in rows how many rows were actually fetched from the database. If the database cursor is used, this rows has only <256 records at once. If the server-cursor is not used, the ResultSet may have thousand of records at once.

Thank you for the hint, the fetch size is 256 and the size of the rows is 256 as well when debugging. I stopped the process closely to the 3 hour mark and managed to dump 21.8GB. I took the dump.log file results and compared them to the previous ones that used TYPE_SCROLL_INSENSITIVE and there is not much of a difference, 2h and 42 minutes when using TYPE_FORWARD_ONLY and 2h and 57 minutes when using TYPE_SCROLL_INSENSITIVE. 710 tables were dumped in this time.

I am picking up work on this issue for #7991 and will be testing the changes first to see if there are any obvious problems since 7496a is pretty far behind current trunk.

First things first, there are a lot of 23/12/15 12:47:11.619+0200 | SEVERE | com.goldencode.p2j.persist.ForwardResults | ThreadName:Conversation [00000015:bogus], Session:00000021, Thread:00000010, User:bogus | Cannot call setRowNumber on empty or fully iterated results in FORWARD_ONLY mode logs while testing a customer application. During testing, I did not find anything problematic and went ahead and started a POC run. POC threw the following error:

Caused by: com.goldencode.p2j.persist.PersistenceException: Failed to populate record for class ...
Caused by: org.postgresql.util.PSQLException: ResultSet not positioned properly, perhaps you need to call next.
    at org.postgresql.jdbc.PgResultSet.checkResultSet(PgResultSet.java:2772)
    at org.postgresql.jdbc.PgResultSet.getLong(PgResultSet.java:2078)
    at com.mchange.v2.c3p0.impl.NewProxyResultSet.getLong(NewProxyResultSet.java:503)
    at com.goldencode.p2j.persist.orm.SQLQuery.hydrateRecordImpl(SQLQuery.java:899)
    at com.goldencode.p2j.persist.orm.SQLQuery.lambda$hydrateRecord$5(SQLQuery.java:762)
    at com.goldencode.p2j.jmx.NanoTimer.timer(NanoTimer.java:131)
    at com.goldencode.p2j.persist.orm.SQLQuery.hydrateRecord(SQLQuery.java:762)
    at com.goldencode.p2j.persist.orm.ScrollableResults.lambda$get$0(ScrollableResults.java:243)
    at com.goldencode.p2j.persist.orm.ScrollableResults.execute(ScrollableResults.java:440)
    at com.goldencode.p2j.persist.orm.ScrollableResults.get(ScrollableResults.java:212)
    at com.goldencode.p2j.persist.ScrollingResults.get(ScrollingResults.java:197)
    at com.goldencode.p2j.persist.PreselectQuery.cacheResults(PreselectQuery.java:5592)
    at com.goldencode.p2j.persist.AdaptiveQuery.sessionEvent(AdaptiveQuery.java:2230)
    at com.goldencode.p2j.persist.Persistence$Context.notifySessionEvent(Persistence.java:5374)
    at com.goldencode.p2j.persist.Persistence$Context.commit(Persistence.java:4768)
    at com.goldencode.p2j.persist.TxWrapper.commit(TxWrapper.java:307)
    at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:10943)
    at com.goldencode.p2j.util.TransactionManager$WorkArea.access$12200(TransactionManager.java:10389)
    at com.goldencode.p2j.util.TransactionManager.lambda$processCommit$3(TransactionManager.java:6876)
    at com.goldencode.p2j.jmx.NanoTimer.timer(NanoTimer.java:131)
    at com.goldencode.p2j.util.TransactionManager.processCommit(TransactionManager.java:6836)
    at com.goldencode.p2j.util.TransactionManager.popScope(TransactionManager.java:4659)
    at com.goldencode.p2j.util.TransactionManager.access$7200(TransactionManager.java:711)
    at com.goldencode.p2j.util.TransactionManager$TransactionHelper.popScope(TransactionManager.java:8831)
    at com.goldencode.p2j.util.BlockManager.doBlockWorker(BlockManager.java:10184)
    at com.goldencode.p2j.util.BlockManager.doBlock(BlockManager.java:1532)
    ...
    at com.goldencode.p2j.util.Block.body(Block.java:636)

Dănuț Filimon wrote:

First things first, there are a lot of 23/12/15 12:47:11.619+0200 | SEVERE | com.goldencode.p2j.persist.ForwardResults | ThreadName:Conversation [00000015:bogus], Session:00000021, Thread:00000010, User:bogus | Cannot call setRowNumber on empty or fully iterated results in FORWARD_ONLY mode logs while testing a customer application.

It seems that a row is searched in a loop and when it is not found, a new Locator is needed to find that row. The problem is that results used to find the record (which did not find it) and the results from the next bracket are the same, meaning that the Locator used will return the results from the only bracket available. The size of the bracket is 10 and the number of available records is 3, so when searching the row 4 it will be located in the same bracket (1).

This can be easily solved by comparing the Locator bracket and the ProgressiveResults bracket. When both are the same, false should be returned since the bracket was already searched.

Alexandru, I tested POC with the solution mentioned in #7496-40 after we discussed it could provide a performance improvement.
The following patch:

=== modified file 'src/com/goldencode/p2j/persist/ProgressiveResults.java'
--- old/src/com/goldencode/p2j/persist/ProgressiveResults.java    2023-10-24 08:12:12 +0000
+++ new/src/com/goldencode/p2j/persist/ProgressiveResults.java    2023-12-19 08:54:44 +0000
@@ -980,6 +980,11 @@
          // to get the proper bracket and move to the target row within it.
          Locator locator = locateRow(row);

+         if (results != null && locator.getBracket() == this.bracket)
+         {
+            return false;
+         }
+         
          // If we are caching, retrieve every result set and cache results.
          if (cache != null)
          {

I also tested a customer application with this change while looking for other bugs. I was not able to identify the issue with the POC tests that cause wrong results, ForwardResults is created and records are retrieved correctly but without the actual code behind it I am not able to find the problem. Still investigating...

• POC tests: passed;
• POC regression tests: passed (with the expected number of failing tests);
• Smoke tested a large application: passed;
• Radu helped me test another large application: passed;
• Majic regression tests: passed;
• Custom tests: passed;
• Adaptive-scrolling tests: passed.

This change is good to go. Alexandru, where should I commit the patch so that it can be merged asap?

Committed 7496b/rev.14879 Added the patch from #7496-41.

Greg, this seems like an easy performance gain. It was fully tested (except ETF). We can go ahead with the merge of 7496b.

You can merge 7496b to trunk now.

Doing it now - just 10 mins to check it out.

Branch 7496b was merged into trunk revision 14891 and archived.

This task is still open due to 7496a.

Danut, can you cut-out it even more.

1. Please check if this can be reproduced with temp-tables - it is a bit easier to follow. Remove the unnecessary fields. Is the index relevant?
2. Is this a QUERY-OFF-END problem or an AdaptiveQuery problem? Please replace the "<buffer-create and assign>" with "message" where possible. It is easier to follow and test.

• with no changes, the cursor already found the first record and running cursor.getNext() will return null. In this case the current record of the buffer is replaced with null;
• with 7496, a RandomAccessQuery is created. The current record of the RecordBuffer is not replaced with null after calling results.next(), it is cached before any statement is executed and the record is found afterwards (since it was made visible through honorRecordNursery).

I am not sure how to approach this problem. In the first place, the cursor would have no rows to show and in this case (with changes), the record is made available too soon.

Rebased 7496a with trunk/rev.14981. 7496a is now at revision 14991.

Rebased 7496a with trunk/rev.15494. 7496a is now at revision 15504.

I am currently working on replacing the old tests for this issue with unit tests.

There were no regressing tests from the adaptive set, only fixes from the original set.

This is really good stuff, Danut!

• 7156c - 289768 p2j queries were executed
• 7156c + 7496a - 289705 p2j queries were executed

There are no additional/missing p2j queries from what I could remark.

For p2j-query_<customerdb> there are 124645 p2j queries executed with 7156c and 123000 with 7156c + 7496a.

I've looked into the logging h2 database for __temp and <customerdb> databases and noted the following (note that the top fqls are the same in both cases):

For the <customerdb> I got the following results (there was a notable difference here):

• from MetaField__Impl__ as metaField where metaField.recid = ?0

If I remember correctly, this is part of the meta tables that are added in fwd.

• for p2j-query__temp there are 1385 distinct fqls logged while only 1380 are logged with 7156c+7496a
• for p2j-query_<customerdb> there are 1834 distinct fqls logged while only 1829 are logged with 7156c+7496a

It is normal to have the same number of P2J queries. In an AdaptiveQuery it either chooses to use ProgressiveResults using 1/2/3/4 or more SQL queries or ScrollingResults using 1 SQL query. As you see, there will always be one P2J query, but the number of SQL queries differ.

Anyway, if there were no regressions in your testing, please move this into Review/100%. I want to take a second look into the changes to ensure we don't miss something before testing.

• I am OK with the changes
• There are some places where ErrorManager.recordOrShowError(3159, String.format(errMsg, qname), ... is used, but it also has some boolean params. The boolean param is decided by an if statement on dynamic || dynamicDelegate. Can we embed this expression directly as an argument, to avoid the if statement?

• any harness / unit tests we have available.
• smoke tests in the GUI applications
• performance tests (2 sets)

The changes are only implementing the forward-only functionality. In trunk, all queries that are inside a for-each are already using the ForwardResults implementation. This task was mostly about the dynamic queries that are made forward only using FORWARD-ONLY syntax. This way, PreselectQuery.executeQuery will start using the ForwardResults more often.

Alexandru Lungu wrote:

• There are some places where ErrorManager.recordOrShowError(3159, String.format(errMsg, qname), ... is used, but it also has some boolean params. The boolean param is decided by an if statement on dynamic || dynamicDelegate. Can we embed this expression directly as an argument, to avoid the if statement?

I can just remove the additional boolean parameter as it will end up with the same value (true by default). The if statement is required to determine if recordOrShowError or recordOrThrowError should be called, it can't be removed.

Oh I see, it is a difference on the method called - you are right.

The following example:

@Test.
    METHOD PUBLIC VOID test4():
        DEFINE VARIABLE hq AS HANDLE NO-UNDO.
        DEFINE QUERY q FOR tt1.
        hq = QUERY q:HANDLE.
        hq:FORWARD-ONLY = TRUE.
        hq:QUERY-PREPARE("FOR EACH tt1").

        hq:REPOSITION-FORWARD (1).
        Assert:isTrue(FALSE, "Test should have ended.").

        FINALLY:
            hq:QUERY-CLOSE().
            DELETE OBJECT hq.
        END FINALLY.
    END METHOD.

   @LegacySignature(type = Type.METHOD, name = "test4")
   @Test(order = 4)
   public void test4()
   {
      QueryWrapper query4 = new QueryWrapper("q", false);
      handle hq = TypeFactory.handle();

      internalProcedure(Test7496ForwardOnlySet4.class, this, "test4", new Block((Body) () -> 
      {
         query4.addBuffer(tt1, true);
         hq.assign(query4.asHandle());
         hq.unwrapQuery().changeForwardOnly(new logical(true));
         hq.unwrapQuery().prepare("FOR EACH tt1");
         hq.unwrapQuery().queryForward(1);
         Assert.isTrue(new logical(false), new character("Test should have ended."));
      }, 
      (Fini) () -> 
      {
         hq.unwrapQuery().queryClose();
         HandleOps.delete(hq);
      }));
   }

The queryForward() method does not take into account that the query might be closed, the error Cannot REPOSITION query q unit it is opened. (7315) should be thrown.

Dănuț Filimon wrote:

The queryForward() method does not take into account that the query might be closed, the error Cannot REPOSITION query q unit it is opened. (7315) should be thrown.

In fact the exception is suppressed because the call to forward(rows) is wrapped in a try-catch.

I've started testing 7496a using the unit tests of a customer last night and it got stuck on a module (infinite loop). Currently retesting without any changes to see if the issue persists, then I'll have to test the remaining modules with 7496a to have all the results.

I finished running the unit tests of a customer and found 2 tests regressing in 2 modules, but after retesting with 7496a/15505 those were fixed (tested locally with 7156c/15502).

• Smoke test 3 GUI application
• Harness test
• 2 customer unit tests

Testing left: None

EDIT: updated status
EDIT: updated status 2

Alexandru Lungu wrote:

I am OK with the changes in rev. 15505.
Please also run performance tests - contact [icp] for the performance tests available.

Got it. I've updated #7496-76 with the latest testing status.

I applied the changes over 7156c rev. 15501. I will redo the testing today, I will update when they are done. ETA: 3h.

I updated the test plan status from #7496-76 with the latest results. The test plan was completed and there were no regressions found.

After retesting, performance is not affected. (-1% from baseline)

Rebased 7496a with trunk/rev.15523. 7496a is now at revision 15534.

7496a can be merged.

• In ProgressiveResults.moveTo(), the position is 236, it scrolls with delta = 1. The executed statement uses 110 as offset and there are 170 rows in the result set (280 rows with no offset). The fetched id of this row is 128th (no problems here).
• Same place, the position is now 237, delta = 1 (fails). The problem should not be that it can't handle the last result correctly, because it should reach record 129/170.

128 sound familiar, a number that can be found in directory.xml of the ETF application:

        <node class="container" name="persistence">
          <node class="integer" name="jdbc-fetch-size">
            <node-attribute name="value" value="128"/>
          </node>
        </node>

The strangest part now, I'd expect a rollback / the transaction to be committed (the TRANSACTION_COMMITTING event in AdaptiveQuery.sessionEvent()) but none of that happens!

Dănuț Filimon wrote:

The strangest part now, I'd expect a rollback / the transaction to be committed (the TRANSACTION_COMMITTING event in AdaptiveQuery.sessionEvent()) but none of that happens!

Correction, it does call AdaptiveQuery.sessionEvent(), but a bit earlier. I was watching out for an event between the two scroll() calls in the same result set but the ForwardResults were already cached (huh, weird?). Now here's the twist, the TRANSACTION_COMMITTING is called we have a ProgressiveResults with a SimpleResults which is cached, but then we get a getNext() call on the dynamic query and those results are replaced by ForwardResults and the resultsCached is still true. The solution is to find the right place to mark those results as not cached after replacing the SimpleResults with ForwardResults.

I am now thinking on how to notify the AdaptiveQuery that it's results (ProgressiveResults) changed the delegate.

Dănuț Filimon wrote:

I am now thinking on how to notify the AdaptiveQuery that it's results (ProgressiveResults) changed the delegate.

I implemented a solution and successfully tested ETF, the portal ... does not exist issue is solved. Cleaning up the implementation, making it a bit simpler, adding javadoc and it will be up for a review soon enough!

I've tested 7496c/15558 using the unit tests of a large customer and did not find any regressions.

Alexandru Lungu wrote:

Also please extend xfer 4GL unit test suite with your examples.

Should I commit the adaptive scrolling tests in the fwd_tests folder and the forward-only tests to tests/query?

adaptive_scrolling should clearly go into fwd_tests (maybe persist/queries folder?).

forward_only depends ... the tests that are only testing the conversion and simple run-time support should got in tests - double-check if there aren't already some of that there. The forward_only tests that are related to delegate change and transaction commit should also go into fwdtests/persist/queries folder as it tests a FWD specific functionality non-existent in 4GL.

Alexandru Lungu wrote:

adaptive_scrolling should clearly go into fwd_tests (maybe persist/queries folder?).

I was unable to create adaptive scrolling unit tests with persistent tables.

forward_only depends ... the tests that are only testing the conversion and simple run-time support should got in tests - double-check if there aren't already some of that there. The forward_only tests that are related to delegate change and transaction commit should also go into fwdtests/persist/queries folder as it tests a FWD specific functionality non-existent in 4GL.

There are only conversion/simple runtime tests.

Committed xfer testcases/1655:

• adaptive scrolling tests in fwd_tests/persist/queries/
• forward-only tests in tests/query/

Tests only use temporary tables.

• large customer application harness and run some reports. (Harness passed, tested 4 reports and those also passed)
• ChUI regression testing. PASSED (tested by Andrei [ai])
• large customer application unit-tests that runs with trunk. PASSED (tested by Andrei [ai])

Rebased 7496c to latest trunk/15578 and the branch is now at revision 7496c/15579.

Danut, what is the status of 7496c? What testing is left?

Alexandru Lungu wrote:

Danut, what is the status of 7496c? What testing is left?

7496c has be queued for ChUI regression testing, I've yet to get an update from Andrei (ai) who is helping me with the test.

ChUI regression tests passed. l.e. updated #7496-102

7496c can be merged.