FWD » Core Development » Database

Part of #10737, I discovered that BY clause index overlapping resolution is influenced by WHERE clause. This happens for any kind of query.
For presort however, the regression tests shown that FWD trunk doesn't properly find new records after OPEN QUERY, which apparently is possible. The scenario is rather limited in the sense that it happens only if the BY clauses partially overlap an index, but the WHERE clause ensures that this is enough (with equality checks on leading components).

I committed testcases/1813 with tests/query/index/ByAndWherePresortTestCases.cls. Check with tests/query/index/ByAndWherePreselectTestCases.cls for non-presort cases that work fine in trunk.

I keep finding failing testcases and fixing them, will post here when there are no issues to be solved.

OE testing is conclusive when it comes to the queries that are converted to FWD PresortQuerie. Result rows membership is additive, but never subtractive. Once resolved, nothing is removed, but new matches ahead of the cursor are picked up. That's not a preselected snapshot, and it's not a live cursor. It's a frozen-size list whose forward contents are reevaluated. The bottom line is that simply inheriting the behavior of AdaptiveQuery is not enough, I am working on matching it.

Once resolved, nothing is removed, but new matches ahead of the cursor are picked up. That's not a preselected snapshot, and it's not a live cursor.

This sounds like the behavior of a scrolling query, that uses a Cursor. This is implemented for any DynamicQuery: AdaptiveQuery and CompoundQuery.

Artur, please take a step back and do a design plan for this porting. The simple reroute of the extentds from PreselectQuery to AdaptiveQuery doesn't sound enough. There are >10 use-cases that are handled through the P2JQuery API and we should rather have them documented, before porting. I will provide some examples:

• scrolling vs non-scrolling vs forward-only behavior
• invalidation:
  • revalidation
  • cacheOnReval
  • cross-session invalidation due to foreign commits
  • dirty-share honoring including earlyInvalidation
• results delegation (there are plenty of results, results-adapter, highlevelresults instances). We need to standardize this to be able to make room for PresortQuery
  • integration with ScrollingResults, ForwardResults, PagedResults or ProgressiveResults
• FQL composition:
  • _multiplex consideration
  • breakValue consideration
  • sort order consideration and trailing recid
• caching on session commit if result-set is forward only (due to same session-commit).
• consideration of PresortCompoundQuery
• locks, triggers, buffers loading, related buffer handling (OF syntax) and embedded buffers (embedded CAN-FIND)
• projection queries vs full queries
• plenty of code has instanceof AdaptiveQuery or similar.

This task doesn't resonate as a simple porting, but as a very careful surgery that requires a comprehensive plan and skilled surgeon.
Tests are most welcome, but please make them on groups. We know before hand what testing should tests and we can drive the AI to generate very quirky tests to capture revalidation, cacheOnReval and other things like this.

OE testing is conclusive when it comes to the queries that are converted to FWD PresortQuerie. Result rows membership is additive, but never subtractive. Once resolved, nothing is removed, but new matches ahead of the cursor are picked up. That's not a preselected snapshot, and it's not a live cursor. It's a frozen-size list whose forward contents are reevaluated. The bottom line is that simply inheriting the behavior of AdaptiveQuery is not enough, I am working on matching it.

I would have qualified this as the "scrolling" behavior. If not please ignore my comment.
If it is the "scrolling" behavior, than this should be leveraged from the AdaptiveQuery and not reimplemented in PresortQuery.
I would say that most of the bugs that occur in the process is a gap in linking PresortQuery to the AdaptiveQuery parent, as each functionality discovered is most probably already implemented in the super-class, but the sub-class fails to leverage it.

First I want to record the 4GL constructs that convert to PresortQuery and their measured behaviour.

Resolution is read from NUM-RESULTS sampled immediately after OPEN QUERY, before any fetch: equal to the row count means the set was materialised at the open, 0 means resolution is incremental. Change sensitive is whether a row committed after the open, sorting past the end of the set, is delivered by the continuing walk. Both post-open rows are committed, so no cell below depends on any dirty-share setting.

OpenEdge applies one rule: can the database produce this order by walking an index? If it can, the walk is incremental and picks up rows committed ahead of the point resolution has reached. If it cannot, an unindexed break field, a computed expression, a descending sort against an ascending index, a duplicated sort field, the set is materialised at the open and nothing committed afterwards is seen.

FWD trunk diverges in both directions.

1. It materialises the two shapes OpenEdge walks lazily. That is #10806.
2. It resolves BY <field> BY <field> incrementally and change-sensitively, where OpenEdge materialises it. This is the opposite divergence.

I think OE change sensitive is matching the FOR EACH conversion decision on FWD (to AdaptiveQuery or PreselectQuery).
In other words, does it make sense to have PresortQuery still extend PreselectQuery and have a separate AdaptivePresortQuery that extends AdaptiveQuery?

Or maybe something that fits in the Composite design pattern, or a Query which uses a delegate (switch between AdaptiveQuery and PreselectQuery, setDelegate).

After reanalyzing the tests, the table above has to be corrected.

The two deleted rows: BY <field> DESCENDING converts to PreselectQuery, and BY <field> BY <field> to AdaptiveQuery. Neither is a presort query, so neither belongs in a PresortQuery table.
What the corrected table says:
- FWD trunk is never change-sensitive for any presort construct.
- OpenEdge is change-sensitive for exactly the two whose order the database can produce by walking an index.
- The other two agree already.

So the design is now simpler.

1. The design

Two classes, chosen at conversion:

• PresortQuery extends PreselectQuery — unchanged. Materialised and frozen. Already correct for BREAK BY <unindexed> and BY (computed).
• AdaptivePresortQuery extends AdaptiveQuery — new. Incremental and change sensitive. For a BREAK BY whose order an index can produce.
• Conversion picks between them with the discriminator it already computes: clientSort in index_selection.rules, plus the equality brackets that make a partial index match sound.
• AdaptivePresortQuery adds only break-group support on top of AdaptiveQuery. Scrolling, invalidation, cursor and results handling all come from the parent, untouched.

2. Why the choice belongs at conversion time

• A partial index match is sound only when the skipped leading sort components are constant — BREAK BY si2 under where si1 = ... works only because si1 is pinned by equality. If it varied, the delivered order would be wrong.
• That bracket information exists at conversion and is not exposed at runtime. QueryComponent has no accessor for it.
• The runtime already has a fork — createHighLevelResults chooses IncrementalResults or SortedResults — and it is mis-gated: sortCriteriaMatchSortPhrase compares both lists from index 0, so a criterion matching at offset 1 fails and the eager path is always taken. The runtime cannot currently make this call correctly with the information it has.

Existing coverage for PresortQuery, by subject

Or maybe something that fits in the Composite design pattern, or a Query which uses a delegate (switch between AdaptiveQuery and PreselectQuery, setDelegate).

Do you mean emitting PresortQuery like we do a QueryWrapper to use a delegate? I think it makes sense - it is indeed composition.
However, I am not sure how tightly coupled this PresortQuery shall be. Will it simply proxy the next/prev/first/last/etc. API and build the groups on its own? I think this can work, but I think we are just scratching the surface now. I am not fully confident we see all the functionality or PresortQuery: LAST-OF, FIRST-OF and maybe other aggregates (e.g. ACCUMULATE and ACCUM?).

Two classes, chosen at conversion:

I am inclined to approve this approach. The composition would make sense if the presort implementation would have to share some behavior, but IMHO PresortQuery and AdaptivePresortQuery wouldn't duplicate much of the logic. Also, there might be different logics for these after all, even if we can't find one now, so I would prefer a more pessimistic approach now in order to provide clarity to conversion and have flexibility later on to change only PresortQuery or AdaptivePresortQuery.

I committed the initial implementation of the plan above to 10806a, at this point I have only 9 failing tests, no regressions compared to trunk. Working on fixing them.

• Presorter.java 234-283 I didn't make this one, it flips the meaning. if (!criteriaMatchAt(...)) return false asks whether every offset matches; we need whether some does. BREAK BY si2 under WHERE si1 = ... on an index over (si1, si2) gives a 3-component phrase (si1, si2, PK tiebreaker) and one criterion si2, matching at offset 1 only inverted, offset 0 fails and we're back on the eager path.

But you were right that it doesn't read well. The bound is now a named lastOffset, there's a comment saying the criteria are a run to find rather than something every offset must satisfy, and criteriaMatchAt says what it walks: the sortHelpers keys in insertion order against criteria.get(offset) onward.

Working on fixing the last 'in scope' test.

I looked at 10806a/r16769.

This is basically a patch for a quirk. I remember something vaguely similar when reading a stream, if the last line does not finish with the line terminator (\n), the previous line is read twice before returning EOF, and the actual last line is never read.

• it needs to be well tested to be sure we do not affect the normal execution of the query;
• it must be well documented in the code. Luckily, the patch is localised and can be easily annotated with supplementary Java comments/javadoc.

• AdaptiveQuery.java:
  • while the other new local flags used for identification of the exceptional situation are reinitialized each time the next() method is invoked, deadEndDelivered is not reset on open(), first(), etc which should normally invalidate this flag;
  • what about the mirrored case, when the result is navigated in reverse order (using previous())? How does the quirk manifest?

You were right about deadEndDelivered: it was only cleared by an ordinary forward delivery, so anything else that moved the query left it stale and a stale guard suppresses the re-delivery next time the same dead end is reached, which is the bug it exists to prevent, inverted. Added forgetDeadEnd() and call it from first, last, previous, both reposition variants and cleanup. The field javadoc now says what the guard means and why anything else invalidates it.

New tests proc_probe_dead_end_guard.p + TestDeadEndGuardResetsAndLeavesNormalWalksAlone,
every value measured on OpenEdge:

The pass1/pass2 pair is the regression test for the guard same dead end twice, and a stale guard drops the second. The first two are the "normal execution untouched" check.

The reverse case. There isn't one. OpenEdge rejects backward navigation on a BREAK BY query outright: Cannot LAST or PREV Query when FORWARD-ONLY or -noautoreslist or BREAK BY enabled. (12393), and AdaptivePresortQuery is only emitted for BREAK BY. But FWD
allows the move, which is what makes previous() reachable for a presorted query here when it is unreachable in 4GL so the forgetDeadEnd() call in previous() is doing real work.

I split that into its own test, TestBackwardNavigationOnBreakByIsRejected, which fails. It is pre-existing and separate from this task, nothing in the presort work adds or removes that validation.

Committed the changes to rev 16770.

Indeed, the fix in r16771 should work as explained in #10806-32.

Multi tenant app testing is ok, tested conversion, harness, web ui navigation, reports and load testing.
Continuing with etf and chui.

Etf housing and chui regression testing passed. Rebased 10806a on trunk/16782.
Will ask colleagues to test the other projects.

Stefanel and Razvan will help me with further testing, thanks guys!
Please post here the results.

• I generated BREAK phrase
• I also loaded BREAK phrase (private)

Artur, please take a look and let me know if some of the documented behavior is out of place. I will review and do some tests on 10806a next.

Three items from the #10806 measurement work. Evidence: OpenEdge 12.8 single-user, plus the FWD trunk run of the query_shape_uncommitted_visibility corpus.

1. The OPEN QUERY grammar permits a combination the compiler rejects

openQueryStmt lists INDEXED-REPOSITION and BREAK as sibling options in any order, but the reference refuses that pairing at compile time. Re-measured on a temp-table, with controls both ways:

brk_ir.p    (BREAK BY + INDEXED-REPOSITION) -> error=yes
    [14297] BREAK BY only allowed if query q is not INDEXED-REPOSITION. (14297)
brk_only.p  (BREAK BY alone)                -> error=no
ir_only.p   (plain BY + INDEXED-REPOSITION) -> error=no

Pure language restriction, no database involved. Since backward navigation is also rejected (12393), a BREAK BY query's reposition surface is effectively nil.

2. Pending-change bullet — split out the third case

The third case, "an update to a sort or break field that changes the record's position or group", measured as working on FWD trunk: TestPresortOrdersByPendingSortValue asserts "CCC,ZZZ" where stored values give "ZZZ,CCC", and TestPresortGroupsByPendingBreakValue asserts one group where stored values give two. Both pass — SortedResults resolves keys from temp records set with setTempRecord, which already carry the pending values, so ordering and grouping use the new value while selection does not.

Only the eager SortedResults path was exercised; the IncrementalResults post-commit quickLoad path the private page blames was never reached. Suggested: keep cases one and two, scope case three to the path where it was observed.

3. The unindexed/computed exemption is stated as Sure but recorded as unreconciled

"BREAK BY an unindexed field, or BY a computed expression, behaves the same as the reference." The Performance entry on the same page says the set is taken in full at open "even when an index could produce the order", and the private page calls the IncrementalResults streaming question "the main reconcile point recorded in the audit".

Suggested: move to Unconfirmed. Every corpus probe uses an indexed break key behind a WHERE equality — the mis-gated eager path — so the exemption is untested.

I am committing query_shape_uncommitted_visibility now.

Artur, I found an issue, please take a look.

A PRESELECT loop with BREAK BY on an indexed field must convert to a PresortQuery, as on trunk, not to an AdaptivePresortQuery. PresortQuery extends PreselectQuery and walks a list built once at open, so a record created in the loop body does not join the walk. AdaptivePresortQuery is sensitive to changes, and the new record joins the walk. The cause is the new index_satisfiable_sort note (index_selection.rules:1167), which ignores the PRESELECT keyword; database_access.rules then emits AdaptivePresortQuery (database_access.rules:1480, 2035).

Testcase:

DEFINE TEMP-TABLE tt NO-UNDO FIELD f AS INTEGER INDEX ixf f.
DEFINE BUFFER b FOR tt.
DEFINE VARIABLE n AS INTEGER NO-UNDO.

DO n = 1 TO 3:
   CREATE tt.
   tt.f = n.
END.

n = 0.
REPEAT PRESELECT EACH tt BREAK BY tt.f:
   FIND NEXT tt NO-ERROR.
   IF NOT AVAILABLE tt THEN LEAVE.
   n = n + 1.
   IF n = 1 THEN DO TRANSACTION:
      CREATE b.
      b.f = 2.
   END.
END.
MESSAGE "visited=" n.

Note: The expected result is visited=3, with 10806a is visited=4

The new index_satisfiable_sort note (index_selection.rules:1167) is computed from indexSatisfiable and !sortDegraded and table_count == 1, with no reference to the PRESELECT keyword, so database_access.rules (:1480, :2035) emits AdaptivePresortQuery for a REPEAT PRESELECT ... BREAK BY <indexed field>. That class is change sensitive by design, so a record created in the loop body joins the walk. PRESELECT resolves the set at open however the order is produced.

Fix 1 — index_selection.rules: added !descendant(prog.kw_presel, 1) to the note. This restores trunk's query class for every PRESELECT shape and leaves the true arm, and therefore the FOR EACH behaviour the task was written for, untouched. Measured on OpenEdge 12.8, the rule's own comment is correct for FOR EACH: an index-driven sort stays change sensitive, a client-side sort is materialised and frozen. It is simply not a statement about PRESELECT.

Fix 2 — Presorter.java: a BREAK phrase cannot close a group without reading the row after the current one, so the read position of a break walk is one row ahead of the row whose body is running, and a row created into that band is never delivered (seeded BBB,CCC,DDD and paused on BBB: BBX is not delivered, CCX/DDX/ZZZ are, and dropping the BREAK delivers BBX too). IncrementalResults now passes over a supplied row that sorts strictly before the furthest row it had already read when the backing was replaced. Note that AdaptiveQuery.changedRecordSortsAfter cannot be used for this — it is reached through doubleCheckInvalidation, which returns early for a scrolling query, and a BREAK BY query is scrolling by language rule.

Result

Across the whole slice, 185 tests go from 152/33 to 162/23. The single remaining failure, testPreselectAgainstDeleteAhead, fails on trunk as well: a row deleted ahead of the cursor is still delivered from a frozen result list. It is a pre-existing PresortQuery defect on a different axis from this task — the branch's own AdaptivePresortQuery handles the same case correctly — so I propose a separate issue rather than widening this one. The test is new, so this is a newly discovered trunk defect rather than a known one.

Committed the fix to rev 16791.

Most of the findings can be reproduced, working on extending the test suite and fixes.

I introduced ~ 50 more tests in the suite, there are 3 regressing tests, compared to trunk, the failure reasons are:

Over-delivery — break by si1 (tests 1 and 2). The branch decides "behind the read position" by comparing sort keys. readAheadBand holds the resolved values of the break criteria only — just si1 = CCC. The created row is also CCC, and sortsBefore treats a full tie as "not behind", so CCC/1 is let in. The comparison needs the total order (si1, si2, rowid); the band is two levels too coarse. Result: BBB/1,CCC/5,CCC/1,DDD/1, with the row out of index order.

Truncation — break by si1 by si2 (test 3). There the band happens to cover the whole key, so the row is correctly refused — and the walk then dies, losing DDD/1. Refusing a row inside Presorter.loadRow breaks the reconstruction: the row never enters the presort cache, so cursor.peekLastLoadedResult — the anchor the re-executed backing is repositioned on — is stale, the re-execution comes back empty, and the loop reads that as end-of-results (addRow(null) + cleanupBackingResults()).

One cause underneath both. The branch has no representation of the walk's read position. It approximates it with four derived pieces of state — resupplied, backingResultsOffset and readAheadBand in Presorter.IncrementalResults, plus peekLastLoadedResult in AdaptiveQuery — which agree only while every row the replacement stream produces is either handed back or appended. OE semantics requires refusing some rows, and that breaks the invariant.

I need to mention that for establishing an OE baseline, I use a self hosted database, client/server uses the message buffer which heavily skews the results. Since we are dealing with prefetched records and invalidation, another layer of caching/buffering is highly confusing and is a matter for future improvements.

Review of 10806a / r16794

I analysed the branch focusing on the last two revisions documented in #10806-51. For some reason, the changes have multiple formatting issues. I could blame a rebase error?

• AdaptivePresortQuery.java:
  • line 125, 945: extra empty lines;
  • lines 982 and 989. Usage of integer.UNKNOWN is recommended. Doing so does not all any routine, does not create any new objects.
  • line 992: usage of max(0, sorted.getNumberOfLoadedRows() - resultListDeletions). Is it possible to delete more records than loaded and get a negative number?
• AdaptiveQuery.java:
  • lines 4569-4582: private method in protected area ()should be moved down the file, to private section;
  • lines 5960-5994: the javadocs are grouped together. Or rowMatches() method with its javadoc was inserted between provideNewResults() method and its javadoc;
  • line 6055, 6063: extra empty lines;
• Presorter.java:
  • line 137: the RECORD_ID is hardcoded to "recid", but the correct approach would be to use the configurable Session.PK instead;
  • line 141, 827, 1925: extra empty line;
  • line 1765: missing javadoc for furthestRowRead(). However, it looks like it can be found at line 1735. There is also another stray javadoc at line 1729 which looks like the original from backingReplaced(), but this has a new javadoc;
• Presortable.java:
  • line 114: extra empty line.

That's a first pass. I will try a more in-depth analysis tomorrow, if needed.

Ovidiu Maxiniuc wrote:

Review of 10806a / r16794

I analysed the branch focusing on the last two revisions documented in #10806-51. For some reason, the changes have multiple formatting issues. I could blame a rebase error?

The formatting was broken after removing logging statements from the code with a regex, sorry for that. I addressed them in rev 16795.
Regarding max(0, sorted.getNumberOfLoadedRows() - resultListDeletions), it is more of a defensive guard than a crucial piece of logic.

Review on latest version:

• [MAJOR] functional Presorter.IncrementalResults.loadRow: Break rows are recorded at backingResults.getRowNumber() (the delegate's own row number), but isNewBreakGroup/isLastOfBreakGroup look them up by the presorted layer's row number. After the backing is replaced (invalidate, resetResults, re-execute) the delegate row numbers restart, so FIRST-OF/LAST-OF and break accumulators are wrong for all later rows. Trigger: FOR EACH t NO-LOCK BREAK BY t.f where another session commits a change to t while the cursor is past row 0. Record at backingResultsOffset + getRowNumber() or at the layer's own index.
• [MAJOR] functional Presorter.IncrementalResults.loadRow: actualRow = backingResultsOffset + backingResults.getRowNumber() assumes every delegate row becomes a layer row, but rows dropped by behindReadAhead/noRecordLoaded advance the delegate without addRow, and consumeBackingReplaced only compensates for alreadyResupplied. The read position drifts, the one-row lookahead is lost and isLast/isLastOfGroup return true early. Trigger: walk on BBB with CCC read ahead, another session creates BBX; BBX is dropped as behind the band, then DDD1 (with DDD2 following) reports LAST-OF.
• [MAJOR] functional Presorter.IncrementalResults.sessionEvent: The first TRANSACTION_COMMITTING during an AdaptivePresortQuery walk drains the whole remaining delegate into a SimpleResults, sets resultsCached and deregisters, freezing the result set so rows created by other sessions/the body are never delivered (OpenEdge delivers them), and loading all remaining rows into memory (N+1 if the delegate is a DynamicResults). Whether this happens depends on whether the query was invalidated in the same transaction. When the delegate is a connected DynamicResults, spent.next() also walks the query's buffers without pushTempContexts. Trigger: FOR EACH t EXCLUSIVE-LOCK BREAK BY t.grp with per-iteration transaction and a body that creates a row sorting later.
• [MAJOR] performance Presorter.IncrementalResults.loadRow: Every loaded row with break groups executes furthestBand = resolveBand(), allocating a BaseDataType[] and re-resolving every sort criterion, though the value is only read in backingReplaced() when readAheadOfCursor() > 0 (only AdaptivePresortQuery). PresortQuery also uses createIncrementalResults, so every existing PresortQuery BREAK BY walk pays this on every row. Gate on backingResumesAtCursor() or resolve lazily in backingReplaced().
• [MAJOR] performance Presorter.IncrementalResults.behindReadAhead: Once readAheadBand is set it is never cleared, so every subsequent row allocates a band and resolves the sort phrase (in the resultsCached path also push/pop temp contexts and a quickLoad per component per row). Trigger: AdaptivePresortQuery BREAK BY walk where a transaction commits mid-walk. Clear readAheadBand once a row not behind it is appended.
• [MINOR] performance AdaptiveQuery.ResultsProvider.provideNewResults: The loop while (!found && resumeAnchorByScan() && adapter.next()) is an unbounded linear scan of the replacement delegate; if the anchor row was deleted or re-keyed it fetches the entire remainder and then adapter.reset() discards it. Bound by readAheadOfCursor or stop once past the anchor's band.
• [MINOR] functional Presorter.sortsBefore: Rows are dropped by comparing with DataSorter (unknown sorts high ascending), which assumes the database returns 4GL index order. MariaDbLenientDialect does not honour nulls-last, so with a ? key in the walk and a replacement backing, every non-unknown row after the unknown group is silently skipped via behindReadAhead.

PS: it is a bit tricky to follow the 5k patch ATM to be fair. That is why I prefer the minimal amount of changes to port PRESORT queries as a subclass of AQ in order to fix #11879 and don't have regressions. Any other side-fixes no necessarily due to RPESORT, but rather quirks and limitations of AQ that were not in scope, should be deferred, either in other task branches or actual tasks. Anything bigger than 1k patches fills my brain context and I need compacting :)

Q: Are the foreign-commit changes a separate subsystem, not specific to presort?
Yes. Cross-session invalidation in AdaptiveQuery is switched off by a hardcoded forceGlobalInvalidation = false, so outside this branch no AdaptiveQuery sees other sessions' commits. The foreign-commit code is Session.noteWrite, the SQLQuery hook, the Persistence counters, the cap in PagedResults and resumeOnForeignCommit in AdaptiveQuery. I'll remove it and defer it to its own task. One catch: resumeAtReferenceRow() also runs on a normal session close (SESSION_CLOSING_NORMALLY), so that call stays or falls back to invalidate(true).

Q: What is the goal of releaseRowsBehind? Is there an example where it clears the repo-cache?
It only limits memory. AdaptivePresortQuery must scroll, while trunk's non-scrolling PresortQuery kept just the previous, current and next rows. It is enabled only for a standalone, unbrowsed FOR EACH (walksForwardOnly()). Every repositionByID comes through QueryWrapper, so REPOSITION can't reach that query, and no code reads the dropped rows. So there is no example of a visible clear. Named queries keep their rows, which they must: OpenEdge allows REPOSITION backwards on BREAK BY.

Q: Can you document resumeFetcher? Is "resume" linked to foreign commits?
resumeFetcher, resumeIndexValues and resumePaged are the paged form of resumeAtReferenceRow(), which runs on a foreign commit and on a normal session close. Without them, both paths switch to dynamic mode (row by row). They can be deferred with the foreign commits, at that cost on session close.

Q: What do "walk" and leaveWalk mean? Can the quirk move into AdaptivePresortQuery?
"Walk" means a run of consecutive next() calls. leaveWalk() resets the re-delivery guard and the RETRY row on first, last, prev and reposition. Both only apply to AdaptivePresortQuery:
- redeliverRowBeforeSkippedOffEnd() is false everywhere else.
- retryRow needs a standalone scrolling query, and only AdaptivePresortQuery is one.

I'll move them into AdaptivePresortQuery.

Q: What does hydratesScrollingRows mean?
The presorted layer takes the record the result set is already on, instead of loading it again with quickLoad by id. The scrolling backing returns only ids, so that reload would be one SELECT per row. That cost comes from reading the code; I haven't measured it.

Q: Is it correct for loadByValues to call alignToRow?
Yes. All seven callers pass the row the cursor is already on: first, last, next from the cache, previous, the RETRY row, and loadCurrentRow from QueryWrapper and BrowseWidget. FIRST-OF/LAST-OF read from the layer, so it must follow the cursor. When it's already there, the call changes nothing.

Q: Can an AdaptivePresortQuery be a CompoundQuery component?
No. It is emitted only when the statement's table_count is 1 (index_selection.rules), and dynamic queries go through the same rules.

Q: Enhanced browse: is added sorting considered for BREAK?
Yes, though I haven't run it in a browse. BrowseWidget.beforeQueryOpen applies the sort before each open. Every sort criterion is a break level, so the new one becomes level 1 and shifts the others: QUERY:FIRST-OF then means the new field. PresortQuery uses the same Presorter code. The query falls back to fully sorted results unless the new order still matches the index.

Q: Is INDEXED-REPOSITION possible?
- Static OPEN QUERY: no. OpenEdge rejects it at compile time with 14297.
- Dynamic QUERY-PREPARE: yes. OpenEdge accepts it and applies the option: after REPOSITION, CURRENT-RESULT-ROW is ? and NUM-RESULTS restarts.
- The branch has a regression here. After REPOSITION-TO-ROWID to g=3/v=1, GET-NEXT returns g=1 v=3. OpenEdge and trunk return g=3 v=2. The presorted layer isn't moved to the random-access position. This needs fixing.

Side finding, not caused by the branch. FIRST-OF on a group's first row reached by REPOSITION is no in OpenEdge and yes in FWD. Trunk does the same.

Regarding the scope of the issue, I can attempt to limit it, but in the development process I kept finding regressions compared to trunk which meant new sets of changes. I can try to limit the scope without causing regressions, is it ok if I move the 'essential' changes in 10806b?

Alexandru Lungu wrote:

• Eduard, please review IndexSelectionWorker changes and index_selection.rules

I think the changes in IndexSelectionWorker@ and index_selection.rules are safe.

Artur Școlnic wrote:

there is also one in chui tests

Revision 16800 causes it.
When another session commits to the walked table, AdaptiveQuery.resumeAtReferenceRow now switches a FOR EACH walk to PagedResults, whose pages are read through a forward-only ForwardResults cursor. resumePageIndexValues only checks walksForwardOnly() and pagingShapeRejection; it does not reject a scrolling query, as the normal paging path does (AdaptiveQuery.java:4342). AdaptivePresortQuery is always scrolling, because finishInitialize calls setScrolling().
The FOR EACH is TransactionType.FULL, so each iteration commits. On every commit cachePagedResults -> PagedResults.cachePage -> cachePageCursor -> PreselectQuery.cacheResults takes the scrolling branch: it calls target.reset() and re-reads from the start. ForwardResults.reset() cannot rewind; it only logs Cannot call reset in FORWARD_ONLY mode. The cached page therefore holds only the rows after the cursor, but is positioned at the cursor's old row number, so rows are skipped. The page then looks shorter than its limit, and the walk ends early.

Yes. Cross-session invalidation in AdaptiveQuery is switched off by a hardcoded forceGlobalInvalidation = false, so outside this branch no AdaptiveQuery sees other sessions' commits. The foreign-commit code is Session.noteWrite, the SQLQuery hook, the Persistence counters, the cap in PagedResults and resumeOnForeignCommit in AdaptiveQuery. I'll remove it and defer it to its own task. One catch: resumeAtReferenceRow() also runs on a normal session close (SESSION_CLOSING_NORMALLY), so that call stays or falls back to invalidate(true).

Please do defer.

It only limits memory. AdaptivePresortQuery must scroll, while trunk's non-scrolling PresortQuery kept just the previous, current and next rows. It is enabled only for a standalone, unbrowsed FOR EACH (walksForwardOnly()). Every repositionByID comes through QueryWrapper, so REPOSITION can't reach that query, and no code reads the dropped rows. So there is no example of a visible clear. Named queries keep their rows, which they must: OpenEdge allows REPOSITION backwards on BREAK BY.

The performance improvement is marginal. There was effort to move cursors from Record[] to Long[]. Thus, there may be millions of records stored safely in such cursor without much fuss. We will consider such performance improvement in the future, but today it is not worth it. The memory problem is already there in trunk.

resumeFetcher, resumeIndexValues and resumePaged are the paged form of resumeAtReferenceRow(), which runs on a foreign commit and on a normal session close. Without them, both paths switch to dynamic mode (row by row). They can be deferred with the foreign commits, at that cost on session close.

Please defer.

I'll move them into AdaptivePresortQuery.

Perfect.

The presorted layer takes the record the result set is already on, instead of loading it again with quickLoad by id. The scrolling backing returns only ids, so that reload would be one SELECT per row.

This needs consideration. The scrolling backing returns only ids if the query is share/exclusive lock. This is because we always want the last image of the record before loading in the buffer. This is implemented in AQ already and it may be leveraged by the presort implementation.

When you say that the presorted layer takes the record the rs is already on, do you mean is re-hydrating from result-set? If so, then hydrate is first looking in the session cache providing the latest cached image. If the record was deleted in the mean-time, mind that AQ fetch is raising MissingRecordException and continues the next or whatever API was triggering it. If load by values or reposition is used, then other resolutions are employed. It is very important to avoid bringing back to life records from rs that were deleted in the mean-time, especially if the locking level was share/exclusive.

Q: Can an AdaptivePresortQuery be a CompoundQuery component?

So this means that PresortCompoundQuery is still preselecting results and not leveraging the dynamic behavior, right? I don't want to scope this topic on this task, but we should account that if we will ever make PresortCompoundQuery non-preselecting, then:

• individual components can become AdaptivePresortQuery.
• optimizer may join several AdaptivePresortQuery in a multi-table AdaptivePresortQuery, which its own invalidation/revalidation rules.

Q: Enhanced browse: is added sorting considered for BREAK?

This will need testing. Each time a sort column is added to the query, its state may change (from adaptive, to preselect; so from adaptive presort to simply presort and back on) - dependning if the BY matches an index prefix at runtime.

Q: Is INDEXED-REPOSITION possible?

This will be a PITA. Anyway, we will figure it out.

Regarding the scope of the issue, I can attempt to limit it, but in the development process I kept finding regressions compared to trunk which meant new sets of changes. I can try to limit the scope without causing regressions, is it ok if I move the 'essential' changes in 10806b?

Can you exemplify? I find it hard to understand how a test could regress that required foreignCommit, considering that neither the trunk PresortQuery not AQ doesn't honor it.

Artur, do you manage to port the changes into a 10806b only with some of the essential behavioral changes? If you encounter regressions comparing with trunk, please post them here in order to assess.
I prepared in parallel a test suite for acceptance testing 10806b. If the task branch will pass them without regressions comparing with trunk, I will approve the merge to ameliorate #11879

I managed to get the branch in a somewhat stable condition, there is only one known regression compared to trunk at this point.
The regression is an endless loop in a BREAK BY walk. It happens when the walk opens while the same session has an uncommitted change to the field the walk sorts on.

The test that shows it (TestUncommittedCreateAndOrdering.testPresortOrdersByPendingSortValue):
1. Table dsNonunique has two rows with si1 = "S1": P1 with si2 = BBB, and P2 with si2 = CCC.
2. Inside a transaction, buffer b-ds changes P1's si2 to ZZZ. Nothing is committed or flushed: the database still holds BBB, and only the session knows ZZZ.
3. A procedure then walks FOR EACH q-ds WHERE si1 = "S1" EXCLUSIVE-LOCK BREAK BY q-ds.si2.

OpenEdge returns CCC, then ZZZ, and stops. Trunk returns the same. The branch returns CCC, ZZZ, CCC, ZZZ, … forever, and the test never finishes.

Why trunk gets it right: trunk runs this walk as a preselect PresortQuery. It reads both rows once, then sorts the records in memory using the values the session sees. P1 is ZZZ in the session, so the order is CCC, ZZZ, and the list is fixed, so the walk ends.

What the branch does instead, step by step:
1. The walk is now an AdaptivePresortQuery, a kind of AdaptiveQuery. At the open, AdaptiveQuery notices that the session has uncommitted changes to this table. A preselect would read stored database values and miss them, so it switches to dynamic mode right away.
2. In dynamic mode, rows come from a RandomAccessQuery worker, one at a time, the way FIND NEXT works. It finds "the row after the one I'm on" by comparing sort keys.
3. The worker starts and finds CCC. Then it looks for the row after CCC; its search sees the session's pending value, so P1 counts as ZZZ, which sorts after CCC. It returns P1. Correct so far.
4. Next it looks for the row after P1. To do that it needs P1's sort key, and it takes it from its reference record, an internal copy (RecordState{COPY}) that holds the stored database value BBB, not the pending ZZZ. The row after BBB is CCC, so it returns CCC again.
5. From CCC it finds P1 again, from P1 it goes back to CCC, and so on. The presort layer above trusts whatever order it gets, so it delivers CCC, ZZZ, CCC, ZZZ, … and the FOR EACH never reaches its end.

Why it's a regression, and whose defect it is: the inconsistency in steps 3–4 (search with the pending value, position from the stored one) belongs to AdaptiveQuery's dynamic mode, not to the presort code. Trunk has it too: the same scenario with plain BY si2 instead of BREAK BY is an AdaptiveQuery on trunk, and it loops there (CCC, BBB, CCC, BBB, …). Trunk's BREAK BY only escaped because it was a preselect that never entered dynamic mode. The branch turns BREAK BY into an AdaptiveQuery, which is what makes it change-sensitive, so it now takes the same broken path.

Why it's a regression, and whose defect it is: the inconsistency in steps 3–4 (search with the pending value, position from the stored one) belongs to AdaptiveQuery's dynamic mode, not to the presort code. Trunk has it too: the same scenario with plain BY si2 instead of BREAK BY is an AdaptiveQuery on trunk, and it loops there (CCC, BBB, CCC, BBB, …). Trunk's BREAK BY only escaped because it was a preselect that never entered dynamic mode. The branch turns BREAK BY into an AdaptiveQuery, which is what makes it change-sensitive, so it now takes the same broken path.

This is acceptable. This is a known bug in AQ. In fact, it is a potential bug in 4GL as well if changing records inside the for each that move the record later without proper guarding. So:

• trunk has wrongly preselecting presort and bugged AQ
• branch has AQ and APQ leverage the same bugger engine. But this engine is the actual dirty-share implementation which will be rather replaced with #8388 soon enough.
  • as long as the infinite loop was not a problem yet through AQ, I think having it in PAQ as well acceptable. It is exploiting an existing issue that is under fixing now.

Eric, can you provide some feedback on #10806-68 and -69? The goal of this task is achieved to move PresortQuery to a dynamic model, but it now surfaces AQ bugs. We fix the preselecting incorrect philosophy of presort and replace it with a buggy dynamic implementation (that is already used by AQ) which uses dirty-share. The same failing test on the brand new AdaptivePresortQuery was also failing on AdaptiveQuery in trunk. The PresortQuery from trunk doesn't fail as it is (incorrectly) preselecting.

Alexandru Lungu wrote:

Eric, can you provide some feedback on #10806-68 and -69? The goal of this task is achieved to move PresortQuery to a dynamic model, but it now surfaces AQ bugs. We fix the preselecting incorrect philosophy of presort and replace it with a buggy dynamic implementation (that is already used by AQ) which uses dirty-share. The same failing test on the brand new AdaptivePresortQuery was also failing on AdaptiveQuery in trunk. The PresortQuery from trunk doesn't fail as it is (incorrectly) preselecting.

If we have the green light on this, please wrap up the changes in the b branch and let me review + test.
Please prepare cross-customer testing as far as we can ASAP.

10806b contains the new AdaptivePresortQuery and the core set of changes necessary to support it, the reposition regression fix and the chui regression fix. It causes one known regression in the single user test suite. Alex, please review.

Please start testing whatever you have in hand and ask team around to fast track this.

10806a passed the multi tenant app testing and chui had one regression which is fixed. I will start with etf and maybe find someone to test the large gui app with 10806b.

Artur Școlnic wrote:

10806a passed the multi tenant app testing and chui had one regression which is fixed. I will start with etf and maybe find someone to test the large gui app with 10806b.

Can you reuse conversion done with 10806a to test 10806b alone?
Mind that etf and other large gui app will need reconversion.

Alexandru Lungu wrote:

Can you reuse conversion done with 10806a to test 10806b alone?

For the multi tenant app and chui, yes. etf conversion + runtime takes 3.5 hours.

Alexandru Lungu wrote:

Eric, can you provide some feedback on #10806-68 and -69? The goal of this task is achieved to move PresortQuery to a dynamic model, but it now surfaces AQ bugs. We fix the preselecting incorrect philosophy of presort and replace it with a buggy dynamic implementation (that is already used by AQ) which uses dirty-share. The same failing test on the brand new AdaptivePresortQuery was also failing on AdaptiveQuery in trunk. The PresortQuery from trunk doesn't fail as it is (incorrectly) preselecting.

My main concerns are that we are:

• fixing the core problem in the customer application which was worked around by adding the buffer release;
• not introducing new regressions;
• not surfacing old latent bugs which make the runtime fail in new ways.

In other words, if we're fixing the core "missing buffer release" problem without making the overall behavior worse, I'm good with scoping back the fix as you suggest.

However, notes 68 and 69 seem to contradict each other. 68 says the new branch causes a regression not shown by trunk. 69 says it's an existing problem. If we're not exposing it in trunk (or the custom build line where the bug was reported), but we are with this branch, isn't that effectively a new regression?

AdaptivePresortQuery brings some of the issues from AdaptiveQuery to new 4gl query patterns in 10806b.

Artur Școlnic wrote:

AdaptivePresortQuery brings some of the issues from AdaptiveQuery to new 4gl query patterns in 10806b.

I'm not sure what is meant by "new" patterns. For the same 4GL code base, we don't want to see net new failures.

So, what does your statement mean in practical terms? Will users see new failures where the runtime was working correctly with those 4GL query patterns before? Or were those query patterns failing already before the changes (perhaps in a different way)? We shouldn't introduce new failures where there were none before.

The customer might see new failures in presort queries.

The multi tenant app and chui (ran only the previously failing tests) seem fine with 10806b. Alex, have you had a chance to run your suites?

Artur Școlnic wrote:

The multi tenant app and chui (ran only the previously failing tests) seem fine with 10806b. Alex, have you had a chance to run your suites?

This is WIP. Managed to confirm they pass in OE and run them with a baseline in FWD. Running them now with 10806b (4.7k files).

So, what does your statement mean in practical terms? Will users see new failures where the runtime was working correctly with those 4GL query patterns before? Or were those query patterns failing already before the changes (perhaps in a different way)? We shouldn't introduce new failures where there were none before.

A scenario that was always working in a customer application will potentially regress now under specific contexts. This is a trade-off from a trivial implementation of PresortQuery that we have in trunk vs the complex one in 10806b. We are aware of limitations on dirty-share, even intra-session. But in order to make PresortQuery work in a dynamic fashion, we have to leverage that (nasty) dirty-share implementation. This is the requirement of #11879 - it needs the partial dirty-share implementation in order to work with the risk for some cases to hit the bugs from dirty-share.

Artur's test unveil a dirty-share bug that also holds for non-PresortQuery, but which is also visible now in PresortQuery.

In order to overcome this, we need more dirty-share fixes - #8388 to start with.

However, notes 68 and 69 seem to contradict each other. 68 says the new branch causes a regression not shown by trunk. 69 says it's an existing problem. If we're not exposing it in trunk (or the custom build line where the bug was reported), but we are with this branch, isn't that effectively a new regression?

It is a new regression, yes. We trade a problem (lack of dynamism of PresortQuery) for another (the lack of completeness for dynamism in FWD). My request of feedback for you Eric was if this is acceptable. Because if it is not, #10806 is going to Pend for other dirty-share tasks - #8388 being one of them (and the only one for the moment).

Regarding regressions:

1. Record visibility: a row created ahead of the cursor is now delivered (1 test)¶

tests/persistence/sort/break_by_grouping/break_flags_under_retry_and_client_where/RetryWithCreateAhead.cls#checkRetryWithCreateAhead

• FOR EACH tt BREAK BY tt.nm. On row AAA the body creates BBX, which sorts ahead of the cursor; the first attempt does UNDO, RETRY.
• OE and the baseline: trace AAA,BBB,CCC. The walk never delivers BBX, although it is in the table afterwards (count 4).
• Task: trace AAA,BBB,BBX,CCC.

Expected: trace= AAA,BBB,CCC|count= 4|row= AAA 1|row= BBB 2|row= BBX 9|row= CCC 3|
but was:  trace= AAA,BBB,BBX,CCC|count= 4|row= AAA 1|row= BBB 2|row= BBX 9|row= CCC 3|

The new change-sensitive AdaptivePresortQuery picks up a row that the loop body created ahead of the cursor, in a create-ahead plus UNDO, RETRY shape where OE's BREAK BY walk does not. This is close to the core of #10806, so it needs a decision on which create-ahead shapes OE actually delivers.

2. Dynamic BREAK BY queries are treated as forward-only (5 tests, one probable cause)¶

Probable cause: the QueryWrapper change "An unbrowsed BREAK BY query navigates as a forward-only one: GET LAST/PREV ..." is too broad. On a dynamic, non-browsed BREAK BY query, GET-FIRST after a forward walk and GET-LAST now raise the forward-only errors. OE 11.6.3 allows both.

• The first two fail alone and in the suite. The other three pass alone and fail in the suite session in 2 of 2 runs, while the baseline passes them in the same session. So they are order-dependent, not flaky.
• ForEachBreakByLastOfControl and GetLastControlByWithoutBreak do not fail on their own logic. ERROR-STATUS carries over between tests in one session, so an earlier test in the suite most likely now leaves a 12393/12394 behind. Not proven: I did not trace which earlier test leaves it.
• t08WithoutNoErrorRaises failing only in the session suggests that the forward-only decision also depends on state left by earlier queries in the session, not only on the query's own shape.

Fixing the forward-only decision for dynamic BREAK BY queries would probably clear all five.

3. Harness BW05: visibility now matches OE (OE-red regression)¶

tests/concurrency/break_by_foreign_commit/TestBreakByForeignCommitReader.cls#BW05CommitAheadControl (two-session harness)

• Another session commits a row ahead of the reader's BREAK BY cursor.
• OE delivers it (Expected: 0 but was: 1). The baseline did not deliver it and passed. The task delivers it and fails with OE's own message, reproduced on rerun.
• FWD now behaves like OE. The test's control expectation (AHEAD_ROW_NOT_DELIVERED_PREFETCHED) is wrong and should be corrected to OE's behaviour.

Related findings that are not regressions¶

• New hang on the task build: tests/query/break_by/change_sensitivity/indexed_presort_walk/TestForwardOnlyBreakByPostOpenCreate.cls#testForwardOnlyWalkPostOpenCreateDelivery loops forever. On the baseline it failed (Expected: 4 but was: 3), so by the rules it is "still failing", but it now hangs instead of failing, and it blocked the whole target-suite session until the 20-minute cap. Its sibling testDefaultBreakByDeliversPostOpenCreate is fixed.
• Fixed by the branch: 120 tests. ps-navigation 69, fv-shape 29, fv-pending 11, bb-change-sens 8 (post-open create/move visibility in indexed BREAK BY walks), aq-progressive 2, ps-grouping 1.

Almost all known regressions solved in 10806b.

• QueryWrapper.canGetFirst: GET FIRST after a move raises 12394 only for an index-walked BREAK BY (AdaptivePresortQuery). A materialised list can restart. GET LAST/PREV still raise 12393 for every BREAK BY query (OE-verified tests in break_by_query_navigation).
• Dropped hydratesScrollingRows; ProgressiveResults is back to trunk.
• database_access.rules: second AdaptivePresortQuery emission site reverted to PresortQuery.

• Create-ahead after a batch UNDO, RETRY (runs as NEXT). Measured on OE 12.8 (TestCreateAheadRetryDepth, 14 cases): rows created by that iteration stay hidden until the walk reaches the second existing row after the undone one.
• TransactionManager.isRetriedAsNext() tells the iteration's listeners that the RETRY ran as NEXT.
• BufferManager/RecordBuffer report the keys of created records to a watching query.
• AdaptivePresortQuery/Presorter pass over those rows.

Testing

• New tests checked on OE 12.8 first, expected values from OE: 27 cases, all pass on FWD.
• FWD: break_by_query_navigation, break_by_grouping, query_shape_uncommitted_visibility plus the new classes, 950 tests, compared with a clean r16817 (trunk).

• Fixed today: RetryWithCreateAhead, TestCreateAheadVariants#betweenRetry, LinkQueryPrepareT02, Ticket7496FromReviewBreakByGetFirstAfterNext, T8100...ModifyInLoopThenRead, 3 x T6459 off-end tests. Nothing newly broken.
• #10806-68 test (TestPresortOrdersByPendingSortValue) passes.
• Still regressed (3): T08WithoutNoErrorRaises, ForEachBreakByLastOfControl, GetLastControlByWithoutBreak. FWD records a shown, uncaught dynamic 12393 in ERROR-STATUS; OE does not. This is in ErrorManager; IMO out of scope here.

Alex, I could not run your 6k tests suite, could you please run it?

Alex, I could not run your 6k tests suite, could you please run it?

Please let me know what was the blocker. I am trying to refine the testing process so that any developer can use it.

1. Test sources that are not valid 4GL

• Unexpanded generator placeholders left in the source (e.g. do n = 1 to $4:). The files do not parse.
• References to procedures or classes that do not exist in the repository (e.g. procWalk in the CoreAq* tests).
• Tests written to check a compile-time rejection (e.g. GroupSynonymForBreakRejected*). They cannot be converted, so they cannot sit in an ABLUnit run.

2. Tests using constructs the converter does not support

• ACCUM nested inside ACCUM (e.g. accum total (accum maximum by f1 f2) * 2), in break_by_accumulators.
• Embedded SQL statements in .p helpers.
• REPOSITION with a parameter type the expression converter rejects.
• About 18 tests whose generated Java does not compile.

These need either a converter fix or moving out of the ABLUnit suites into a separate conversion-gap set.

TestSuite aggregators

• The TestSuite.cls aggregators reference the broken tests above, so they fail to convert as well.
• The FWD test engine reaches sub-packages only through these aggregators. Without them, most of the suite is not discovered.
• Several aggregators also miss classes in their folder (e.g. TestCreateAheadVariants, TestCreateAheadRetryDepth), which are then never run in a package run.

Tests missing from the repository

• The targeted bb-* suites (tests/query/break_by: change sensitivity, indexed group, indexed navigation) are not committed.
• bb-foreign-commit is a two-session harness battery and does not run through unittest.sh.

In the end I got claude to run 1,225 tests, no regressions on those.