FWD » Core Development » Database

This testcase is generating the following output:

FOR EACH table1:
    DELETE table1.
END.

FOR EACH table2:
    DELETE table2.
END.

DEF VAR counter AS INT NO-UNDO.

DO counter = 1 TO 5:
    CREATE table1.
    ASSIGN table1.value1 = counter.
END.

CREATE table2.
ASSIGN table2.value1 = 0.

ON FIND OF table1 DO:
    MESSAGE "LOAD table1" table1.value1.
END.

DEFINE QUERY testQuery FOR table1, table2 SCROLLING. <--COMPOUND QUERY
OPEN QUERY testQuery FOR EACH table1, FIRST table2. <--COMPOUND QUERY

DEFINE BROWSE testBrowse
        QUERY testQuery DISPLAY table1.value1
        WITH 5 down.

Output to file.txt.

MESSAGE "Before last " NUM-RESULTS("testQuery").
GET LAST testQuery.
MESSAGE "After last " NUM-RESULTS("testQuery").

Output close.
ENABLE testBrowse.

file.txt:

Before last  1
LOAD table1 2
LOAD table1 2
LOAD table1 3
LOAD table1 3
LOAD table1 4
LOAD table1 4
LOAD table1 5
LOAD table1 5
LOAD table1 5
After last  5

This behavior shows that a buffer is loaded twice when using a CompoundQuery.

Tested with trunk/15674, the output of file.txt was:

Before last  1
LOAD table1 5
After last  1

Octavian, is the test case provided in #9590-1 complete?

I followed up the first run of the test with another one and the file.txt now contains:

Before last  1
LOAD table1 5
LOAD table1 5
After last  1

I see the problem now.

Yes, the test case is complete. I got the result using 9509a/15650 which has some changes for CompoundQuery. The problem is that the result is the same if using AdaptiveQuery and there are no changes regarding this class in 9509a. I expect the same output using trunk with AdaptiveQuery. (see #9509-25).

Octavian Adrian Gavril wrote:

Yes, the test case is complete. I got the result using 9509a/15650 which has some changes for CompoundQuery. The problem is that the result is the same if using AdaptiveQuery and there are no changes regarding this class in 9509a. I expect the same output using trunk with AdaptiveQuery. (see #9509-25).

Thanks for the input.

Using an AdaptiveQuery, the problem is this:

         if (isBrowsed() && !cursor.isFullSet())
         {
            Results results = getResults();

            while (results.next()) // <-- this will trigger the first find for: LOAD table1 [the_value]
            {
               try
               {
                  fetch(true, lockType, false); // <-- this will trigger the second find for: LOAD table1 [the_value]
               }
               catch (MissingRecordException e)
               {
                  continue;
               }

               cursor.addResultNext(getCurrentIds());
            }
            // Mark that the set is full.
            cursor.addResultNext(null);
            // current row is null after results().next and we need to go back to the last row
            results.previous(); // <-- this will trigger the third find for: LOAD table1 [the_value]

            return true;
         }

There's an issue with #9590-9, especially with how triggers are executed for certain result sets. In the AdaptiveQuery scenario, the converted query will use an AdaptiveQuery with ScrollingResults (because we do not have an invalid component yet) and in this case results.next() does not trigger the FIND. But in the second run of the test, the AdaptiveQuery will use DynamicResults which will trigger the FIND on @results.next().

I discovered this after I attempted a simple fix that does not use previous() by saving the row number and setting it after the loop and running a fetch. This also explains why the first run of the example is correct, while any other follow up run outputs the wrong results.

I've managed to fix the AdaptiveQuery scenario, currently looking into fixing the CompoundQuery scenario.

The CompoundQuery ends up triggering the FIND once in the CompoundQuery.last(), Object[] ids = retrieve(LAST, lockType, iterating); line and the second time after reaching PreselectQuery.previous() fetch(avail, lockType, false); line. A similar situation regarding the retrieve call can be found in #9509-32.

Dănuț Filimon wrote:

The CompoundQuery ends up triggering the FIND once in the CompoundQuery.last(), Object[] ids = retrieve(LAST, lockType, iterating); line and the second time after reaching PreselectQuery.previous() fetch(avail, lockType, false); line. A similar situation regarding the retrieve call can be found in #9509-32.

Both calls have something in common, both end up triggering in PreselectQuery.previous(), once for the fetch() call and the other for results.last()/results.previous(). This is a similar situation that I've met for the AdaptiveQuery where I conditioned the fetch() call to be done when there is no invalid component. However, I am not sure of the impact of the fetch call as this will also have to be done for the PreselectQuery.next calls.

Alexandru, you mentioned in the meeting the Results.isAutoFetch() method which is used here:

            boolean alreadyFetched = available && 
                                     results.isAutoFetch() &&
                                     !shouldUpdateAnyLock(lockType);

The problem is that !shouldUpdateAnyLock(lockType) will be false because the query component will have a dynamic predicate (so the LockType of the component will be SHARE).

This seems to be the problem indeed. This was added a while ago and I think it was a quick fix back then - you may need a blame for this. I recall that the dynamic query would have fetched the record with a lock (maybe NO-LOCK) and AdaptiveQuery should have promoted that lock, so it would do a second fetch to update locking. I suppose the underlying query should have done the loading with the right lock though ... this needs investigation.

Alexandru Lungu wrote:

The problem is that !shouldUpdateAnyLock(lockType) will be false because the query component will have a dynamic predicate (so the LockType of the component will be SHARE).

This seems to be the problem indeed. This was added a while ago and I think it was a quick fix back then - you may need a blame for this. I recall that the dynamic query would have fetched the record with a lock (maybe NO-LOCK) and AdaptiveQuery should have promoted that lock, so it would do a second fetch to update locking. I suppose the underlying query should have done the loading with the right lock though ... this needs investigation.

I've come up with this change:

=== modified file 'src/com/goldencode/p2j/persist/AdaptiveQuery.java'
--- old/src/com/goldencode/p2j/persist/AdaptiveQuery.java    2025-02-03 07:59:18 +0000
+++ new/src/com/goldencode/p2j/persist/AdaptiveQuery.java    2025-02-04 08:31:27 +0000
@@ -1376,13 +1376,16 @@

             while (results.next())
             {
-               try
-               {
-                  fetch(true, lockType, false);
-               }
-               catch (MissingRecordException e)
-               {
-                  continue;
+               if (!results.isAutoFetch())
+               {
+                  try
+                  {
+                     fetch(true, lockType, false);
+                  }
+                  catch (MissingRecordException e)
+                  {
+                     continue;
+                  }
                }

                cursor.addResultNext(getCurrentIds());
@@ -1832,7 +1835,11 @@
             }

             offEnd = (avail ? OffEnd.NONE : OffEnd.BACK);
-            fetch(avail, lockType, false);
+            
+            if (!results.isAutoFetch())
+            {
+               fetch(avail, lockType, false);
+            }

             if (isScrolling())
             {

Why is this solution working? fetch(true, lockType, false); was going to call super.fetch(available, lockType, errorIfNull); that in turn calls:

            boolean alreadyFetched = available && 
                                     results.isAutoFetch() &&
                                     !shouldUpdateAnyLock(lockType);
            if (!alreadyFetched)
            {
               Object[] data = available ? results.get(forceOnlyId) : null;
               coreFetch(data, lockType, errorIfNull, false);
            }

• accumulate();
• currentRowDeleted = false;
• foundFirst = true;

Alexandru Lungu wrote:

Why is this solution working? fetch(true, lockType, false); was going to call super.fetch(available, lockType, errorIfNull); that in turn calls:
[...]

Thus, the execution path considers results.isAutoFetch(). Your solution seems to move results.isAutoFetch() conditional upstream, but it doesn't account for record availability and locking. Also, you are skipping other relevant code like:

• accumulate();
• currentRowDeleted = false;
• foundFirst = true;

I see, the scenario uses a RandomAccessQuery and uses the LockType.NONE created in AdaptiveQuery.createSimpleQuery(), so just a normal next() call. But in the end, you are right, I am missing the above important steps.

Alexandru Lungu wrote:

The problem is that !shouldUpdateAnyLock(lockType) will be false because the query component will have a dynamic predicate (so the LockType of the component will be SHARE).

This seems to be the problem indeed. This was added a while ago and I think it was a quick fix back then - you may need a blame for this. I recall that the dynamic query would have fetched the record with a lock (maybe NO-LOCK) and AdaptiveQuery should have promoted that lock, so it would do a second fetch to update locking. I suppose the underlying query should have done the loading with the right lock though ... this needs investigation.

This was added in trunk/12101 for #5136.

Dănuț Filimon wrote:

This was added in trunk/12101 for #5136.

Correction, the actual code from #9590-17 was added in trunk/14523 alongside other performance improvements. The main issue was #5306, it introduced a regression which was fixed later but I could not find which issue (probably #7062?).

My new approach was to take into consideration results.isAutoFetch() when firing the trigger in @RecordBuffer.setRecord(), this is similar to #9590-16 with the difference being that this is not done by using the condition upstream, but downstream. This ensures we call the important methods mentioned.

Dănuț Filimon wrote:

[...] but I am still struggling with the final results.previous() call which triggers the additional FIND trigger (only at the last record). The idea is that we do not do the fetch when it is auto fetched, because the query used by the DynamicResults handles the record search and updating the buffer which triggers the FIND.

Including what I mentioned in #9590-21, I managed to fix this problem in the following way:

=== modified file 'src/com/goldencode/p2j/persist/AdaptiveQuery.java'
--- old/src/com/goldencode/p2j/persist/AdaptiveQuery.java    2025-02-03 07:59:18 +0000
+++ new/src/com/goldencode/p2j/persist/AdaptiveQuery.java    2025-02-05 12:21:43 +0000
@@ -1389,8 +1389,11 @@
             }
             // Mark that the set is full.
             cursor.addResultNext(null);
-            // current row is null after results().next and we need to go back to the last row
-            results.previous();
+
+            // current row is null after results.next() and we need to go back to the last row
+            Object[] data = cursor.getLast();
+            loadByValues(data, lockType);
+            offEnd = OffEnd.BACK;

             return true;
          }

The fix from #9590-22 is good for DynamicResults, but not ScrollingResults. We rely on the already parsed results and we can get the data we need using the cursor. This fixed the FIND trigger for DynamicResults, but does not do so for ScrollingResults. My previous idea was to make RecordBuffer.setRecord() aware of the isAutoFetch status of the results, but this is not ok as it does not fix the ScrollingResults case (the trigger is still fired).

• Original scenario 1:
  • First run:
    • AdaptiveQuery.last() is called, the results used are ScrollingResults
    • results.next() will not result in a FIND trigger
    • fetch will result in a FIND trigger
    • result.previous() will not result in a FIND trigger. (Expected result - favorable)
  • Second Run:
    • AdaptiveQuery.last() is called, the results used are DynamicResults
    • results.next() will result in a FIND trigger
    • fetch will result in a FIND trigger
    • results.previous() will result in a FIND trigger.

The problem here is that DynamicResults relies on a query which will update the buffer (and will fire the FIND trigger when did so) each time we do next()/previous() while it should fire the trigger in the fetch().

Rebased 9590a to latest trunk trunk/15713, the branch is now at revision 15714.