FWD » Core Development » Database

From #10000:

First and foremost, there is Eduard's change in trunk rev. 15701 where buffers from components 2 onward are not released on off-end. This is a correct fix from #9538.

• However, this exposes a limitation in AdaptiveQuery where the cursor.addResultFirst(getCurrentIds()); is called unconditionally. It should have checked whether the query actually run off-end and then call cursor.addResultFirst(null);. [...] we need a better synchronization between results and cursor: the cursor is empty when the results are empty and cursor has records when the results actually fetched something.

I am attaching my attempt.

This covers the example in #9538-69 - but scrolling variant. Debug it and check that after running off-end, the row is also added to the cursor instead of null to signal last entry.
With my attempt, FIND CURRENT becomes broken, because not updating the cursor will let is eventually empty. A FIND CURRENT on an example like #10000-34 will yield an error.

• Extend the FIND CURRENT test-case suite to cover: scrolling, non-scrolling queries + browsed queries + multi-table queries (with optimized components or not).
  • run some nexts and run FIND CURRENT.
  • run some nexts and then some prevs + run FIND CURRENT (the buffers should have been loaded by loadByValue now).
  • etc.
• Check if there are other places where the query should decide whether updating the cursor with ids or null is right. There may be other places apart from the ones in my patch.
• Fix GET CURRENT to work even if that component didn't find anything. It is possible to have a record loaded using loadByValues, but the results and cursor still be empty (and the query off-end). Maybe calling the CURRENT from CompoundQuery should by-pass such checks. The CURRENT solely relies on buffer contents, so it is irrelevant if the query is off-end.
• Extra: please check in OE what happens if buffers are loaded with something else before a CURRENT. Will the CURRENT honor whatever is already loaded or reload the queries with the last entries found?

Mind that some symptoms of this issue were seen in #10000 and fixed properly. However, this issue has the potential to generate logical mismatches which may be hard to understand with the debugger.

Branch 10026a (trunk r16766) fixes GET CURRENT / GET-CURRENT() for compound and single-table queries. All behavior was checked against OpenEdge 12.8.

• CURRENT on a scrolling query reloads the current result-list entry: warning 4108 for each buffer that holds a different record, 4114 when there is no row (off-end, between rows after REPOSITION, deleted row).
• CURRENT on a non-scrolling query refetches the records in its buffers, with 4114 only when no buffer holds a record. GET NEXT / GET PREV on a non-scrolling query stays off-end.
• 4114, 4108 and 3164 follow the 4GL error mode: shown in a plain block, recorded in ERROR-STATUS under NO-ERROR, raised under CATCH or BLOCK-/ROUTINE-LEVEL ON ERROR UNDO, THROW. GET-CURRENT() no longer swallows the raised error.
• REPOSITION on any non-scrolling query reports 3164, not only on FORWARD-ONLY ones.
• CompoundQuery component loading was refactored (r16767), and AdaptiveQuery no longer adds a cursor row when nothing was found.
• Regression fix in FQLPreprocessor: the "or is null" rewrite of ? < field and ? <= field keeps the operand order.

• Both testcases from the previous notes and the browse probe (Swing and web client) give the same output as OpenEdge.
• tests/query and tests/persistence: no regressions (2365 passed, 412 failed; the same failures as before the change).

• PRESELECT and presorted queries do not reload the result-list entry on CURRENT (OpenEdge warns 4108), and REPOSITION on a non-scrolling PRESELECT query does not report 3164.
• With several stale buffers under CATCH, OpenEdge raises one error carrying every 4108; FWD raises after the first.
• OUTER-JOIN cases will be handled in #11576.

I'm working on fixing some issues flagged by a self-review. Then I will clean-up some things and set it to review status.

I fixed some issues, and I think 10026a is ready for a proper review.

• GET CURRENT on a scrolling query between rows reports 4114 and keeps the buffers, for all query types.
• A join keeps the buffers of the components whose records still exist when one record of the current entry is gone.
• A lock conflict while reloading the entry is now reported, no longer swallowed (4108 followed by locking the wrong record).
• When the entry was fully loaded, GET CURRENT no longer reloads every buffer a second time.
• GET-CURRENT(lock) on a closed query reports "not open" again, instead of a Java NPE on an unknown lock.
• No 3164 on REPOSITION when FORWARD-ONLY was assigned FALSE, and no 4114 after a 4108.
• A non-scrolling reposition no longer recurses, and first/last no longer cache a stale row when nothing is found.

Review of 10026a:

-               else
-               {
-                  try
-                  {
-                     available = super.first(lockType);
-                  }
-                  catch (QueryOffEndException e)
-                  {
-                     cursor.addResultFirst(null);
-                     throw e;
-                  }
-               }

-               cursor.addResultFirst(getCurrentIds());
+               available = cacheFirst(lockType);

• This doesn't look right as the logic that is now in cacheFirst is no longer wrapped in the else block and is run unconditionally. All other places that compress the super.first logic ported to cacheFirst are not within else blocks.
• Make noQueryRecord return false after ErrorManager work. The users should return noQueryRecord this will allow us to easier short-circuit work after noQueryRecord by simply running return noQueryRecord. Also, the method naming should be a verb, for example throwNoRecordAvailable or failWithNoRecordAvailable or other suitable verb name.
• Please use {@code true} instead of .

+      if (entry == null)
+      {
+         return 0;
+      }

• Is this a legal state or a safe check? If it is a safe check, please add a log. Same applies to if (!isScrolling() || cursor null || _isOffEnd() || cursor.isBetweenRows()). In some circumstances, calling reloadResultListEntry is wrong rather then expecting 0. So I prefer to know if we have non-scrolling queries that call reloadResultListEntry instead of silently fallbacking to 0.
  • IMHO, this hides a potential problem from logs or diagnosis data.

- super.reposition(row);
+ super.reposition(row, internal);

• I still need to assess this
• Please try to compress history entries to 1/2 lines. If they are too big, it means that information stored there is verbose enough to be included in javadocs instead.

+      else if (!iterating && OffEnd.BACK.equals(getOffEnd()))
+      {
+         // a non-scrolling query stays off-end instead of restarting
+         releaseBuffers();
+         
+         return false;
+      }

• Can you point me to a test for this logic. I am inclined to say the logic is right, but from my experience, CQ is not always realasing all buffers under all circumstances. If I recall correctly, in an INDEXED-REPOSITION multi-table query with both EACH and FIRST, only EACH buffers are released. There may be other cases (maybe after REPOSITION and then going off-end using NEXT).
• staleBuffers I think it makes more sense to return 0 if there is no stale buffer and a positive number instead. I find it hard to associate -1 buffers being stale as "no buffers" being stale. This confusion manifested in:

+      int stale = staleBuffers(entry, buffers);
+      if (stale < 0)
+      {
+         return 0;
+      }

• Please double-check if the lockType of the reloadResultListEntry governs the locking system instead of the "per-query-components" locking. Also check if lockType is missing, the query component locking is honored instead.
• I don't think the changes in FQLPreprocessor are in scope.
• public default int reloadResultListEntry(LockType lockType) is made default to return 0. Shouldn't we consider implementations instead to ensure the fix is complete: PreselectQuery, PresortQuery, etc. same for isBetweenRows. I usually prefer to force queries to have this even with a default value, but stress the developer to think about it. A new query won't be forced to think about these 2 new entries.
• CURRENT_REFETCH, CURRENT_NONE and CURRENT_LOADED seem reasonable for an enum. This also forces beforeCurrent to return a well scoped enum value (e.g. not -1).
• Why is forwardOnlyAssigned needed; isn't forwardOnly enough? AFAIK, a query wrapper can be neither scrolling nor forwardOnly. It can be forwardOnly only if assigned. Correct me if I am wrong.
• beforeCurrent should be renamed to a verb (e.g. isBeforeCurrent). In fact, I think this method actually check the disposition in case of a CURRENT, so the name shall be getDispositionForCurrent which returns the enum in question.
• currentRecords to canGetCurrentRecords.
• empty = (((BufferReference) buffers.get(i)).buffer().getCurrentRecord() == null); this is the available check. Use available API. empty should be in fact allBuffersLoaded which seems quite opposite to empty. It is initialized with true and becomes false when encountering at least one non available buffer.
  • Check the behavior for outer-join. A buffer can be "semi-"unavailable, aka it was last used in an outer-join query and it has no record, but it is in unknownMode