FWD » Core Development » Database

Alexandru Lungu wrote:

If the results are progressive, then an optimization would be to only cache the current delegate. There is logic in ProgressiveResults.sessionClosing, but I think it is called later than the PreselectQuery.cacheResults.

This requires investigation as it affect performance, especially in large queries that are scoped outside a transaction.

PreselectQuery.cacheResults() can be called before sessionClosing(), the changes are easier to made on ProgressiveResults.sessionClosing() because we are already aware of the number of records that can be retrieved based on the remaining size of the bracket. But if the caching is done in PreselectQuery.cacheResults(), we will have to know the number of records that we should expect to retrieve.

I will try to find a test case from a customer application scenario and an analysis of how often cacheResults and sessionClosing are called, from where and the number of records saved.

I will try to find a test case from a customer application scenario and an analysis of how often cacheResults and sessionClosing are called, from where and the number of records saved.

I don't think a biased decision will work here. From my experience, I saw result sets of ~5 records and result sets of 100k records.

Alexandru Lungu wrote:

I will try to find a test case from a customer application scenario and an analysis of how often cacheResults and sessionClosing are called, from where and the number of records saved.

I don't think a biased decision will work here. From my experience, I saw result sets of ~5 records and result sets of 100k records.

Well, I can go straight into action then. I would still need to run some customer tests because I could not get past creating an AdaptiveQuery that does not use DynamicResults. My idea is still to look into making a test case which can help me implement the solution easily and to get an overall idea on how PreselectQuery.cacheResults() is used (and some information on the number of records expected).

Rebased 9383a to latest trunk/15616, the branch is now at revision 15617.

• in case of a ProgressiveQuery, only the current bracket will be cached in a SimpleResults. What if you do a next beyond the last row? It should still fetch the next bracket if any. I think the implementation you have will simply state that there are no other records past the last one in the cached bracket. Let me know if I understood incorrectly.
  • we still need to have a ProgressiveResults as an adapter delegate, but have ProgressiveResults.sessionClosing logic properly kick in in the caching process (maybe renamed to transaction committing, not session closing).

PS: PreselectResults.cacheResults has:

      if (isResultSetCached())
      {
         // results are already cached
         return target;
      }

So if the ProgressiveResults will cache its bracket, then there won't be any need of a getCachableRowsCount. We need a transactionComitting method to trigger something very similar to sessionClosing (aka caching results), but before actually attempting to cache. When cacheResults is reached, the bracket should have been cached already.

Rebased 9383a to latest trunk/15651, the branch is now at revision 15652.

I managed to create examples after going over a customer unit tests and hit the breakpoints in each piece of modified code, there are issues with the 9383a implementation as mentioned in the review. Currently fixed a small one, but the big problem is that it does not go over all records if an AdaptiveQuery with ProgressingResults is used.

Rebased 9383a to latest trunk/16136, the branch is now at revision 16138.

Danut, transactionCommiting is not implemented as a no-op in all Results classes after the rebase:

[ant:javac] /media/rnc/Lexar/gcd/branches/9383a/src/com/goldencode/p2j/persist/SimpleResults.java:96: error: SimpleResults is not abstract and does not override abstract method transactionCommitting() in Results
[ant:javac] class SimpleResults
[ant:javac] ^
[ant:javac] /media/rnc/Lexar/gcd/branches/9383a/src/com/goldencode/p2j/persist/ResultsAdapter.java:97: error: ResultsAdapter is not abstract and does not override abstract method transactionCommitting() in Results
[ant:javac] final class ResultsAdapter
[ant:javac]       ^
[ant:javac] /media/rnc/Lexar/gcd/branches/9383a/src/com/goldencode/p2j/persist/ForwardResults.java:81: error: ForwardResults is not abstract and does not override abstract method transactionCommitting() in Results
[ant:javac] final class ForwardResults
[ant:javac]       ^
[ant:javac] /media/rnc/Lexar/gcd/branches/9383a/src/com/goldencode/p2j/persist/PresortCompoundQuery.java:621: error: CursorResults is not abstract and does not override abstract method transactionCommitting() in Results
[ant:javac]    private static class CursorResults
[ant:javac]                   ^
[ant:javac] /media/rnc/Lexar/gcd/branches/9383a/src/com/goldencode/p2j/persist/ScrollingResults.java:94: error: ScrollingResults is not abstract and does not override abstract method transactionCommitting() in Results
[ant:javac] final class ScrollingResults
[ant:javac]       ^
[ant:javac] Note: Some input files use unchecked or unsafe operations.
[ant:javac] Note: Recompile with -Xlint:unchecked for details.
[ant:javac] 5 errors
[ant:javac] 3 warnings

You can do this:

=== modified file 'src/com/goldencode/p2j/persist/Results.java'
--- old/src/com/goldencode/p2j/persist/Results.java    2025-01-20 10:41:54 +0000
+++ new/src/com/goldencode/p2j/persist/Results.java    2025-08-28 13:08:16 +0000
@@ -234,7 +234,10 @@
     * Invoked when the current session handles the transaction committing event. Gives the implementation an
     * opportunity to perform appropriate processing in response to this event.
     */
-   public void transactionCommitting();
+   public default void transactionCommitting()
+   {
+      // no-op
+   }

    /**
     * Clean up and release any resources which this object is holding, such as open result sets.

Committed #9383-17 to 9383a/16139. Made transactionCommitting() default.

With 9383a I get around 30k calls for the scenario from #10172 comparative with 34-35k calls with trunk. The reason for such a small reduction relies in the capacity of the session cache. It fills up pretty quickly (2-3 records inserted in the cache from the called sub-procedure in addition to the record from the main FOR EACH loop) which leads to continuous cache misses, forcing the session to load the record.

I've modified the limit to be 10k instead of 1024, but around row 1.1k / 7k, the cache is already full and we are back at loading records one by one, so I don't see how increasing the cache size would be a viable solution.

• having a DMO in the session cache => very fast
• not having a DMO in the session cache, but its columns are still selected so the values can be hydrated => fast
• not having a DMO in the session cache and the DMO was fetched using a projection query (only id is available); requiring extra Loader.load => slow

It fills up pretty quickly (2-3 records inserted in the cache from the called sub-procedure in addition to the record from the main FOR EACH loop) which leads to continuous cache misses, forcing the session to load the record.

To be noted that the session cache is LENIENT, so it can grow more than its limit as long as DMOs are still in use. It will evict only DMOs that are no longer used.

I've modified the limit to be 10k instead of 1024, but around row 1.1k / 7k, the cache is already full and we are back at loading records one by one, so I don't see how increasing the cache size would be a viable solution.

Can you try to increase the lenient threshold of caching results from 64 to 1.000.000? If this fixes the issue, it means that we simply do not cache enough data. #9383 was meant to fix this my caching only the current delegate of the ProgressiveResults, while next batches will be fetched from the database in bulk

Increasing the lenient threshold does not solve #10172. Although all record properties are now cached, the problem still seems to be at least related to the session's cache.

By catching all the rows from the current bracket + what the called sub-procedure catches, the next record from the main FOR EACH ends up being evicted before the sub-procedure finishes in order to ensure capacity. As such, session.get() will not find the record in the cache and will be forced to load it independently.

By catching all the rows from the current bracket + what the called sub-procedure catches, the next record from the main FOR EACH ends up being evicted before the sub-procedure finishes in order to ensure capacity. As such, session.get() will not find the record in the cache and will be forced to load it independently.

I see. Well, the goal of the session.cache is to store some DMO instances and this has an obvious limit. Wanting to cache more will eventually break the memory-safe guard imposed by the session cache.

The OG issue is that we COMMIT and the results we have a dropped - too bad. We try to save the day by caching 64 rows, but the others are simply cached by id. This is reasonable as they quite fit in the session cache.
But still, we need the data for honoring the query. Originally, we extracted it using ProgressiveResults (fetching incrementally larger batches) to avoid spiking the memory. When we try now to cache, we should also avoid spiking the memory by *fetching incrementally larger batches). That is why #9383 was created in the first place.

With 9383a I get around 30k calls for the scenario from #10172 comparative with 34-35k calls with trunk. The reason for such a small reduction relies in the capacity of the session cache. It fills up pretty quickly (2-3 records inserted in the cache from the called sub-procedure in addition to the record from the main FOR EACH loop) which leads to continuous cache misses, forcing the session to load the record.

What are the sizes of brackets to be cached when COMMIT occurs? I am wondering if we reach brackets of size 1.000 (or more) and we try to cache all these 1.000 (or more) at once. This should indeed hit the session cache limit.

I've modified the limit to be 10k instead of 1024, but around row 1.1k / 7k, the cache is already full and we are back at loading records one by one, so I don't see how increasing the cache size would be a viable solution.

Can you go for 100k just for experimentation? This is not viable indeed because it represents too much of a memory hit.

We can do some shenanigans to bypass the LENIENT limit of the session cache by making isInUse return true for these DMOs that are cached on COMMIT, but we really need to understand if this is safe to do (from the POV of memory).
Though, there is an interesting thing: when committing there is SavepointManager.commit that should have set the DMO state to TRACKED. In that case, isInUse should return true. Can you double-check this? Maybe the commit of the SavepointManager executes after the caching, making the DMO evict before having the chance to mark them as TRACKED.

PS, the code is:

dmo.updateState(null, TRACKED, false);

So after committing, the DMOs are no longer tracked, making them evictable :) This is exactly what we want to avoid ... but still it is technically correct.
I don't think there is a clear way of keeping the DMOs cached, mostly because the session cache limit is enforced.

What are the sizes of brackets to be cached when COMMIT occurs? I am wondering if we reach brackets of size 1.000 (or more) and we try to cache all these 1.000 (or more) at once. This should indeed hit the session cache limit.

10, 100 and 1000. After caching the third bracket, the session cache fills up. Later it goes up to 6.5k records.

Can you go for 100k just for experimentation? This is not viable indeed because it represents too much of a memory hit.

There are no Loader.load() calls targeting the main FOR EACH buffer with a 100k limit.

I was wondering, if we save so many records on a transaction commit, couldn't we pick it up from results (as we do now) and check if it's alright to use (not STALE, DELETING or DELETED)? We basically already have the DMO in hands from the results, but we prefer to check the session cache. Is there any particular reason for that?

I was wondering, if we save so many records on a transaction commit, couldn't we pick it up from results (as we do now) and check if it's alright to use (not STALE, DELETING or DELETED)? We basically already have the DMO in hands from the results, but we prefer to check the session cache. Is there any particular reason for that?

Anything that is not in the session cache is technically stale. For trunk: if the same session will pick up some of that records from the database (using other statements like FIND) and change them, then the changes won't be reflected in he post-commit cached results of the query. The point of session.cache is to have one single object per pow so the changes on it will be visible in all buffers (and other places) instantly.

If another session is changing this record or the record is DELETED, then the state is updated for the DMO in the session.cache - not the scattered instances through-out the persistence layer (including the post-commit cache of AdaptiveQuery).

Think of session.cache as a container of all DMOs that are in use by the current session. Any DMO that is not inside this cache is by default stale. Now, are the post-commit cached records considered as "in use"? I don't think so; they are simply nice to have in the cache, but we can live without them at the price of performance.

PS: there is a limit of 1024 for session.cache because in systems with >100 users, this will pile up to 100k DMOs in the whole server, which can represent a large memory footprint.
So, technically, it is right to cache records after commit only if it fits in this 1024 limit. Otherwise, with higher limit, all sessions can start caching result sets of 10k rows, resulting in 100 session * 10k rows = 1M rows being cached in the server. This is too much.

So, technically, it is right to cache records after commit only if it fits in this 1024 limit. Otherwise, with higher limit, all sessions can start caching result sets of 10k rows, resulting in 100 session * 10k rows = 1M rows being cached in the server. This is too much.

Do you want to address this in a new commit? (i.e. put a maximum threshold on the number of cached records on transaction committing, dictated by the limit of the session's cache).

Also, since messing up with the size of the session cache is not possible, what if we would detect this one-by-one record loading and force the progressive results to load + hydrate (in order to have the dmos in the session cache) a bracket of lets say, 100 records? DMOs needed for next() will most probably not be evicted, as opposite to caching a bracket of 1000.

Also, since messing up with the size of the session cache is not possible, what if we would detect this one-by-one record loading and force the progressive results to load + hydrate (in order to have the dmos in the session cache) a bracket of lets say, 100 records? DMOs needed for next() will most probably not be evicted, as opposite to caching a bracket of 1000.

I think this is nice. We can make the ProgressiveResults less progressive if they ever get cached. Mind that there is also a nice functionality of "estimating" the memory size of a bracket (i.e. analyzeRow(Object[])). FALLBACK_MAX_EXPONENT is used to set the default maximum exponent to which the progressive can go. But the actual one is returned by getMaxExponent. Maybe you can make getMaxExponent simply return 2 after caching, so that we do not ever attempt to prefetch so many records at once.

Also commited rev. 16141 to remove some unnecessary history entries.

Review of 9383a:

• bracket == forcedMaxExponent + 1 I don't think this is a hard constraint. I think bracket > forcedMaxExponent is reasonable, because you can in fact request for row 1.000.000.000, even if current bracket has only the first 10 records :) and this way you bypass the forcedMaxExponent by a lot. For 1e9 - 1, if the forcedMaxExponent is 2, then the right bracket is around (1e9 - 100) and offset is 99 (size is still 100). In your implementation, bracket is always 2, which is not correct.
  • I would prefer to abstract this logic behind getMaxExponent - something like if (forcedMaxExponent != -1) return forcedMaxExponent;. The for look will do its job accordingly.
    • In other words, the locator won't get any special, it will simply have an enforced limit (just like it happens when approximating the maximum size possible). When caching, you can dictate that directly.
  • BTW, you already have actualMaxExponent. You can use this instead of a new forcedMaxExponent. This is normally computed lazily based on the size of the results. But in your case (transaction ending), you can simply enforce it to 2.
  • This will also avoid doing the extra isBracketDowngraded work downstream. Technically, the Locator is holding the bracket, offset and size - so it is a virtual view on the result-set. Adding isBracketDowngraded makes the locator less of a virtual view (including only positional logic).

For 1e9 - 1, if the forcedMaxExponent is 2, then the right bracket is around (1e9 - 100) and offset is 99 (size is still 100). In your implementation, bracket is always 2, which is not correct.

I thought the bracket always maps to the size, hence why I insisted with the 2 :). For example bracket 1 is always 10 in size, 2 is 100, 3 is 1000 and so on.

If as in your example, the requested row is 1.000.000 and we iterate with brackets of 100 in size, it will take some time to reach that 1B. A faster approach would be to detect when we hit the desired size (100), and then use one single set of divisions instead of possibly millions of additions.

If as in your example, the requested row is 1.000.000 and we iterate with brackets of 100 in size, it will take some time to reach that 1B. A faster approach would be to detect when we hit the desired size (100), and then use one single set of divisions instead of possibly millions of additions.

I do not oppose to a faster way of identifying the bracket and offset. But lets first think about the functionality:

I am concerned that you can get to row X inside bracket 5 without any problem. After committing, you set the actualMaxExponent to 2, which means that locateRow won't locate it to bracket 5, but to bracket 100 maybe. This kind of spontaneous algorithm change is scary.

 Locator locator = locateRow(row);

         if (results != null && locator.getBracket() == this.bracket)
         {
            return false;
         }

This code will check if the row to move to is inside the current bracket. It will technically not if you change the logic in the mean-time.
Also, ProgressiveResults.bracket will be inconsistent if you "downgrade" the bracket exponent.

When committing, we need to think of a better way to set the maximum exponent without altering the existing bracket indexing.

If the concern is on the bracket calculation, maybe when commiting a transaction we can convert the current bracket to this new mode.

Example for a row in bracket 5:
Before it would be calculated as 1, 10, 100, 1000, 10000 and now as 1, 10, 100, 100, 100, ...., 100. For the conversion to work, the row upon which bracket and lastValidBracket were previously calculated are needed, so there is a small overhead here. But now, if (results != null && locator.getBracket() == this.bracket) will see if the bracket actually changed or not when locating the row.

Moreover, the results can have a boolean variable which dictates in what mode we are currently in. convertBracket() will only do the conversion one time, and then fast-exit on any further transaction commits, if the mode hasn't changed. With this implementation, in the future we can revert back to the old mode in case we detect it is faster.

What do you think? Would that solve the problem you posed?

Moreover, the results can have a boolean variable which dictates in what mode we are currently in. convertBracket() will only do the conversion one time, and then fast-exit on any further transaction commits, if the mode hasn't changed. With this implementation, in the future we can revert back to the old mode in case we detect it is faster.

I wonder if we can cut corners here to avoid introducing some functionality that will get harder to maintain. My concern is that this functionality is rare and rather specific to #10172. This whole new mode may be an overkill for such a particular functionality. ProgressiveResults is historically a place of failures due to its existing complexity. I don't want to introduce more of it.

• save position and set it to -1
• set bracket to -1, set delegate to null, reset lastValidPosition and lastValidBracket to -1
• set actualMaxExponent to 2
• call moveTo with save position

With this state, everything (first/next/last/prev/etc.) should work as before. ProgressiveResults should simply recover based on position, without enforcing a new mode. Of course, this attempt will basically drop existing results, but it will be for a good cause. Also, this should happen only if this.bracket is bigger than 3. Otherwise, for 0, 1, 2 you are already in the right state. Nevertheless, if (bracket < FALLBACK_MAX_EXPONENT) conditionals should be rewritten to if (bracket < getMaxExponent()) to honor the new enforced exponent of 2 in case of caching.

The remaining work is to understand what happens on multiple commits. I guess you should indeed save a flag like "avoid resetting on commit" to avoid invalidating on each commit.

Alex, please review rev. 16142 when possible.

Note that #10172 sees only a reduction of 7.5k load() calls out of 33k. If the review is OK, maybe we can go the extra mile and investigate why there are still so many calls left.

Alexandru Lungu wrote:

• Also, shouldn't it be > 2 instead of >= 2? If it is already equal to 2, the progressive results may have a bracket of 100 rows, which is fine when actualMaxExponent equals 2.

Note the reset + moveTo logic is after hydrating the records. If it would be > 2, then by the time we reach the new piece of code, 1000 rows would've been already hydrated (i.e. bracket 3 has been reached), defeating the whole purpose.

A question might arise: why are reset + moveTo executed after hydration? The reason is that fetching from SimpleResults is faster than from ScrollingResults, so we perform moveTo just to align ProgressiveResults to the new state, but completely ignore the fetched results, knowing we already have them.

so we perform moveTo just to align ProgressiveResults to the new state, but completely ignore the fetched results, knowing we already have them.

Is this happening in 9383a? I see that delegate = null and the moveTo which basically emits a new query in the database to gather the same records.

Yes. Executing moveTo may be safer than manually updating the state at the cost of re-executing the query. The good thing is that there will be at most 100 rows fetched and this happens only when switching between modes.

But there is also a trailing:

delegate = new SimpleResults(rows);
       delegate.setRowNumber(0);

That sets that delegate again to a new SimpleResults.

I've just now realized that we don't actually need neither to call moveTo because we already have the rows in cache + the delegate rows, nor to modify position and lastValidPosition because they are absolute numbers, and not relative to the current bracket.

Alex: What do you think about only changing the bracket and removing moveTo?

if (!transactionCommited && bracket >= 2)
{
   transactionCommited = true;
   actualMaxExponent = 2;

   bracket = lastValidBracket = locateRow(position).getBracket();
}

delegate = new SimpleResults(rows);
delegate.setRowNumber(0);

bracket = lastValidBracket = locateRow(position).getBracket();

This will return the bracket index N considering a formula where actualMaxExponent is set on 2.

delegate = new SimpleResults(rows);

Is a delegate with maybe 1M rows, but this is not the N-th bracket.

If bracket >= 2 holds, it means that the rows are not OK because they are too many and represent a bracket that does not correspond to locateRow(position).getBracket(). Ideally, the moveTo was setting the delegate to the right bracket based on the new formula.

Alexandru Lungu wrote:

delegate = new SimpleResults(rows);

Is a delegate with maybe 1M rows, but this is not the N-th bracket.

You are correct, but what if we just shrink rows to size 100 (i.e. Math.pow(BASE, actualMaxExponent))? Wouldn't that correctly be considered the N-th bracket? Remember that this delegate already holds the correct rows + position internally.

You are correct, but what if we just shrink rows to size 100 (i.e. Math.pow(BASE, actualMaxExponent))? Wouldn't that correctly be considered the N-th bracket? Remember that this delegate already holds the correct rows + position internally.

That will be nice. If the bracket you are looking at is the 100th one generated with exponent 5, then technically you can shrink it to size 100, but you need some formulae for it. If you find it and it is well tested, I am OK with this approach. In fact, it is ideal :)

PS: I think there are cases in which you can't, because the offsets of the records may be 1111, 1112, 1113, ..., 2110. If you shrink, maybe not all 100 records are captured in this list (e.g. .... 1109, 1110, 1111, 1112, ....).

• Old bracket = 3 (from 111 to 1010)
• new bracket = 10 (from 811 to 910)
• rows is holding the records from position 830 to 1010

In this case, we should copy the rows from 830 to 910. We just need to calculate what is the starting row of the next bracket in the current mode - 1. Am I understanding something wrong and it's more complicated than that?

In this case, we should copy the rows from 830 to 910. We just need to calculate what is the starting row of the next bracket in the current mode - 1. Am I understanding something wrong and it's more complicated than that?

You are right, but also mind more complex cases:

• old bracket = 10 (sizes of previous brackets were 1, 10, 100, 1000, 10000, 10000, 10000 10000, 10000, 10000). This means that first record is 71111 in bracket 10 and the last is 81110.
• current position is 81100
• new bracket = 812 starting at 81011 to 81110.

This sounds right

Alexandru Lungu wrote:

If the bracket you are looking at is the 100th one generated with exponent 5, then technically you can shrink it to size 100, but you need some formulae for it. If you find it and it is well tested, I am OK with this approach. In fact, it is ideal :)

I think the formulae you refer to can be replaced with locateBracket(bracket + 1).getRow(), where 'bracket' is already updated to the new mode.

I think transactionCommited = true; and actualMaxExponent = 2 should hold for any bracket (not only >= 2). The >=2 is relevant only for the reset + moveTo logic

transactionCommited isn't a flag which is true if at least one transaction was commited. I will change the name into something more appropriate, like isBracketThresholdActive. This is meant to indicate what mode is currently active.

As for actualMaxExponent = 2, setting it when bracket < 2 will force ProgressiveResults to fetch in brackets of maximum 100 records. In the scenario where there was a commit on bracket 0 and 1M rows, wouldn't this bring a performance overhead? My idea was to enable this mode in a lazy manner.

As for actualMaxExponent = 2, setting it when bracket < 2 will force ProgressiveResults to fetch in brackets of maximum 100 records. In the scenario where there was a commit on bracket 0 and 1M rows, wouldn't this bring a performance overhead? My idea was to enable this mode in a lazy manner.

This is correct.

In #9383-46, position 830 is actually rows.get(0). The first record from rows will always match the record under position upon transaction commiting. This is true because of:

do
{
   rows.add(delegate.get());
} while (delegate.next());

This also explains delegate.setRowNumber(0) because we want to sync the delegate with the results.

Moreover, why the total size of reduction should always be 100? In your example from #9383-47, rows should end up with records from 81100 to 81110, which would be a size of 11.

Moreover, why the total size of reduction should always be 100? In your example from #9383-47, rows should end up with records from 81100 to 81110, which would be a size of 11.

The bracket should contain records from 81011 to 81110, but the current position inside it should be 90.

To clarify, after forcing the maximum exponent to 2, the brackets should look like the following. If you relocate to the Nth bracket after changing the exponent, than it means that the first row in the bracket is the sum of sizes of the first N - 1 brackets.

 0: [1] 1: [10] 2: [100] 3: [100] 4: [100] ... N: [100] N+1: [100]

 0: [1] 1: [10] 2: [100] 3: [100] 4: [100] ... N-1: [100] N: [X < 100] N+1: [100]

In #9383-46, position 830 is actually rows.get(0). The first record from rows will always match the record under position upon transaction commiting. This is true because of:

Ah, I understand now. The rows contain only the remaining rows, not the whole bracket. Please disregard my comment then. Let me re-review :)

I can't deny that I am a bit stressed with such "partial" bracket. If you do a previous, then Locator locator = locateRow(row); will give you the same bracket as the one you already have. Because of if (results != null && locator.getBracket() == this.bracket) return false;, this will apparently state that there is no previous (which is wrong).

In other words, I was always keen on holding upon the invariant from #9383-53, because the locate methods would easily identify the bracket and offset. With such bracket that is partially filled, this invariant is broken. Ideally, the delegate should match the entire bracket size.

I see. In this case, we need to change the whole transactionCommiting() because it is already constructed to yield delegates with partial brackets.

Razvan-Nicolae Chichirau wrote:

I see. In this case, we need to change the whole transactionCommiting() because it is already constructed to yield delegates with partial brackets.

I suppose so. Mind that #9383 OG title is about integrating this feature. So if you continued on top of Danut's 9383a, then you are now executing new code (or at least activated some dead-code).

Alex: Can we move to the next phase of testing?

Please do some investigation for memory spikes. What is the minimum amount of memory to actually run these?
I am concerned about this aspect as the initial implementation for this session caching was breaking the memory limit for a customer (so we decided to do projection queries after 64 fetched rows).
We can test the scenario (#9138).
If everything is OK from the memory POV, I guess we can move this into customer application automatic testing.

Is this a real concern?

I can't tell, but I think it is fine.
Please proceed with customer application testing.

Alexandru Lungu wrote:

We can test the scenario (#9138).

I tested the scenario from #9138 with 9383a and didn't see anything strange in terms of memory consumption.

The testing was halted temporary due to a regression found in ChUI regression tests. ATM I don't have an exact testcase, but I managed to get this stack trace:

Caused by: java.lang.IndexOutOfBoundsException: toIndex = 100
    at java.base/java.util.AbstractList.subListRangeCheck(AbstractList.java:507)
    at java.base/java.util.ArrayList.subList(ArrayList.java:1108)
    at com.goldencode.p2j.persist.ProgressiveResults.transactionCommitting(ProgressiveResults.java:875)
    at com.goldencode.p2j.persist.AdaptiveQuery.sessionEvent(AdaptiveQuery.java:2360)
    at com.goldencode.p2j.persist.Persistence$Context.notifySessionEvent(Persistence.java:6237)
    at com.goldencode.p2j.persist.Persistence$Context.commit(Persistence.java:5610)
    at com.goldencode.p2j.persist.TxWrapper.commit(TxWrapper.java:320)
    at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:11429)
    at com.goldencode.p2j.util.TransactionManager.processCommitImpl(TransactionManager.java:7133)
    at com.goldencode.p2j.util.TransactionManager.processCommit(TransactionManager.java:7068)
    at com.goldencode.p2j.util.TransactionManager.iterateWorker(TransactionManager.java:6748)

caused by:

if (position == bracketStart)
{
   // trim the collected rows to the new size as they represent a full bracket
   int newRowSize = Math.min(locator.getSize(), rowSize);
   rows = new ArrayList<>(rows.subList(0, newRowSize)); <----------
}

Razvan-Nicolae Chichirau wrote:

I'll do the following:

• smoke-tests, performance tests and unit tests for some customer apps
• ChUI regression testing
• analyze performance for scrolling / nonscrolling queries in the unit tests dedicated project
• retest #10172

The testing plan passed with no regressions. Can we merge this?

I am working on a scenario to test and check what can be done.

I documented the issue in #11154.

From my POV, this is a customer specific pattern that breaks #9383 idea. I don't think we would be able to generate a test to catch this without more feedback from customer use-case. See my POV from the end of #11154.

I've created 9383b / rev 16369 which reverts the changes that were introduced in trunk / 16217.

Lorian Sandu wrote:

I've created 9383b / rev 16369 which reverts the changes that were introduced in trunk / 16217.

ETF testing passed.

Constantin, do you think we should test other apps besides ETF and #10614?

Lorian Sandu wrote:

Constantin, do you think we should test other apps besides ETF and #10614?

Yes. Both large GUI apps need to be included, plus ChUI.

The smoke test passed on the app I'm managing.

• CHui tests passed
• Large GUI app unit-tests passed

Lorian, what else is left?

Constantin Asofiei wrote:

Lorian, what else is left?

I don't think there is anything left. The branch can be added to merge queue.

Lorian Sandu wrote:

I don't think there is anything left. The branch can be added to merge queue.

Please double-check in the docker container for the #10614 app, the heap set for the ClientDriver process, in classic mode.

Constantin Asofiei wrote:

Please double-check in the docker container for the #10614 app, the heap set for the ClientDriver process, in classic mode.

512 mb. I think it's fine.

9383b was merged into trunk / 16369

A question about Alexandru's note from #9972-167:

if you commit a transaction, the progressive results most probably get redundant, as each and one loop iteration will be going to drop the ongoing result-set (i.e. result-set gets unconnected on COMMIT). So, we can't simply re-emit a query after a COMMIT, because this may happen for each and one record. The solution is to cache whatever we have in memory and use that as much as we can.

Why would a query for a table which is not involved in the commit (i.e. no records have been changed in that table, and the query is NO-LOCK) would require dropping the result-set? Is this a JDBC problem because the query needs to be re-executed after the connection commit?

Alexandru Lungu wrote:

• if you commit a transaction, the progressive results most probably get redundant, as each and one loop iteration will be going to drop the ongoing result-set (i.e. result-set gets unconnected on COMMIT). So, we can't simply re-emit a query after a COMMIT, because this may happen for each and one record. The solution is to cache whatever we have in memory and use that as much as we can.

There is a concept of Holdable Result-set which does not close the result set after COMMIT. I don't know which JDBC driver ("dialect") does not support this feature. The default behavior for JDBC is to close the cursor at commit.

Is this a JDBC problem because the query needs to be re-executed after the connection commit?

Yes. See #10172-31. This is a limitation of fetch-size that brings in data in buckets from the database.

Committed revision 16418 on task branch 9383c:
- When ResultSet is invalided, fetch the full record using batches.

The performance for #10172 & #9972 seems to be similar with the initial work of #9383.
I am planning to find more edge cases tomorrow. I will also try the full batch caching approach.

Committed revision 16420 on task branch 9383c:
- BatchResults invalidates the query when PK is different.
- Fixed an regression when the old Results is not positioned at -1 (added an offset parameter).

What we have discussed in a daily today, to keep track of:

• the batch size should be calculated based on the database fetchsize. This is configurable in directory.xml and we should make sure the batch is smaller than the fetch-size, but not bigger than session cache-size / 10, but not smaller than 1.
• We discussed about normalized extent fields. Is this implemented? If so, are these also batched?
• We discussed about NO-LOCK and locking in general. If we batch records in, we risk to get stale data and when we get to the records to not have the right view of it.
  • Eric suggested that rereadnolock flag is (very) used and the records are usually reloaded if loaded with no-lock and are stale.
  • you need to understand if this batch may cause issues with stale data.

Alexandru Lungu wrote:

What we have discussed in a daily today, to keep track of:

• the batch size should be calculated based on the database fetchsize. This is configurable in directory.xml and we should make sure the batch is smaller than the fetch-size, but not bigger than session cache-size / 10, but not smaller than 1

Ok, I have set it by default to 100.

• We discussed about normalized extent fields. Is this implemented? If so, are these also batched?

I don't currently know what normalized means on the extent fields, but what I know is that all the fields which are queried from SQL are saved into an array. These fields are fetched using a single SQL query, except for the extends, which are queried 1 by 1, field by field. This is how persistence.scroll currently works.

• We discussed about NO-LOCK and locking in general. If we batch records in, we risk to get stale data and when we get to the records to not have the right view of it.
  • Eric suggested that rereadnolock flag is (very) used and the records are usually reloaded if loaded with no-lock and are stale.
  • you need to understand if this batch may cause issues with stale data.

There is an issue which I have figured out. In the latest changes, on PreselectQuery.coreFetch, I have removed isResultSetCached on doFetch. I think this was used when the full record has been fully fetched from database and the result set no longer reflects the latest version of record. I removed that because the batched records are not currently in the session cache.
I will try to find a solution there.

I think it can be no worse then a ProgressiveResults regarding stale DMO because it uses a similar concept, fetching records by bracket.

Teodor Gorghe wrote:

I don't currently know what normalized means on the extent fields, but what I know is that all the fields which are queried from SQL are saved into an array.

A normalized schema has the extent fields persisted in dedicated secondary tables. Those records have a PK as reference used for locating the parent record from the primary database. Therefore, to access the extent values, individual trips to SQL or join queries are necessary.
The alternative is to get rid of the secondary tables and store the full extent inside the primary table. We call this 'expanded' mode. This will allow the full 4GL record to be fetched in a single/simple query, but has the downside that the SQL record gets bigger (we encountered cases of reaching hard limits of SQL for the number of columns in a record).
In the past the normalized was the standard, but now we actively go for expanded schemas. The only projects using normalized are constrained by the above limitation.

I assume the array you mention is the BaseRecord.data. Or you mean the cursor data array?

These fields are fetched using a single SQL query, except for the extends, which are queried 1 by 1, field by field. This is how persistence.scroll currently works.

This does not sound right. To fully populate a 100 extent field would require 100 individual queries beside the one for the non-extent columns.

Ovidiu Maxiniuc wrote:

I assume the array you mention is the BaseRecord.data. Or you mean the cursor data array?

Result which is returned by ScrollableResults.getImpl. This is used from the new BatchResults. The customer is using normalized extents.

I removed that because the batched records are not currently in the session cache.

But they end up in the session cache when retrieved, right? My point is that anything retrieved from the DB should end up in the session cache INSTANTLY, otherwise we risk getting them stale (e.g. keep them in memory, run some for each loops and then hydrate them from result-set / attach them to session cache.

PS: there is also a bullet that requires looking into the dirty-share for each record, like loader.load does. In other words, you need to post-process the rows to understand if the dirty-share manages them with other changes.

Also, pleas make some tests with deleted DMOs (i.e. you batch 100 rows up-front, but in a loop you delete the 50th - you shouldn't find it anymore). This is done by checking the DMO state - if it is deleted, just skip it.

Ok.
In BatchResults, I am thinking to change persistence.scroll to persistence.list.
One difference is that persistence.list returns the records in an array instead as an open result set, but also adds the DMO into session cache.
The doFetch change is no longer required.

Committed revision 16421 on task branch 9383c:
- Use persistence.list instead of persistence.scroll to store records in session cache.

I have started doing some tests, but so far, I have didn't encountered issues with stale records. I will make more tests on monday.

I've started the conv-launch script a few times and unfortunately it gets stuck (or works very slow) at a step that usually completed in ~30s - 1 min.

I'll need to make further investigations and debug to see what is broken.

Lorian Sandu wrote:

I've started the ... script a few times and unfortunately it gets stuck (or works very slow) at a step that usually completed in ~30s - 1 min.

I'll need to make further investigations and debug to see what is broken.

Ok, thanks!

Do you think that caching 1M+ pks into the list is the culprit. Please not that this also happens with trunk.

Lorian, do you have any status update?

Some of the problems i saw were because i was using an older conversion. I've fixed my setup and will retest.

Ok, thanks!

I have managed to get the #11154 up and running, but I see that there is a regression also with 9383c (baseline time is 4 minutes).
I will profile the application and check what is wrong.

There is no dynamic mode, which I have thought.
The issue from #11154 comes from the fact that the Session cache is getting full when iterating through records.

I can confirm that #11154 is caused by fetching smaller batches instead of one with bigger batch size.

Suppose you have a SQL query, which walks by an B-Tree index. When you use LIMIT n OFFSET i, the PSQL will walk the index from the beginning, skips the first i records and returns the n records.
When you have the offset set to 0, the query execution is almost instant, but when you go towards the end (in case of 10M records), the execution of each query will take more than 1 second.

I think the easiest solution for this is to fetch the records in batches, by PK (eg. IN syntax from SQL). This will require more research for different dialects.

The alternative solution I have discussed with Alex yesterday, is about adding additional condition on WHERE clause, which will make that the PSQL to go directly through the last record. This is much harder to implement and the benefits are similar to fetching by PK in batches.

I have great news.
I have tested #9383 with PK fetching in batches, and I see very noticible changes in performance. For instance, for an scenario which took 700ms, now it takes 100ms.
I will test the #11154 and check if the issue was fixed (I am expecting performance improvement vs trunk).

I have tested #11154 and now it takes 2 minutes (from 4 minutes).

Awesome!!!

Committed revision 16422 in task branch 9383c:
- When possible, fetch the records in batch by PK.

I will do some additional testing when the query has more than 2 components. Currently, outer-join falls to classical LIMIT OFFSET approach.

Status from yesterday:
- I have tested different queries, outer-join, with 2 or more components, query with only one component, query with fields, etc.
- The query with one component works great, but the ones with multiple components needs a little bit more work.

For the current approach for queries with multiple components, the SQL execution plan gets a little bit complex and I am thinking of doing server-side joining.

Committed revision 1832 on xfer testcases:
- Added tests for BatchResults (#9383).

Committed revision 16423 on task branch 9383c:
- Refactored BatchResults for doing the server-side join when having multiple components.

Review of 9383c / r16422..16425

I focused only to latest changes. Let me know if I should review the full change-set from the branch.

There are no severe issues. The code should work as expected. Goof job!
The below notes are just about code style and small local optimisations.

• BatchResults.java
  • lines 2-62: extra spaces at beginning of each line;
  • line 153/154: this method has a lot of parameters. Although the code style specifies they should be listed one per line, aligned, I guess we can accept this style here. Otherwise, it will look odd as a vertical list;
  • line 490: I think the new pkAt() method can be used here?
  • line 520: pks.length is batchTuples.size() used above twice (maybe even end - start ?). This line should be moved before 515 and use m for array construction and for iteration;
  • line 521/585: I noticed the aliases array is constructed to have the . at the end of each element. That requires a StringBuilder to be used silently by the compiler in PreselectQuery:6197. Why not passing unprocessed buf.getDMOAlias() there and add the . here, where the StringBuilder is also injected? Of course, if the alias is not empty string because of isOmitAlias.
  • lines 507, 552, 764: The use of UTF extended characters is not encouraged (…, ×). Let's avoid unexpected errors by replacing them with standard ASCII when possible.

Addressed the code review #9383-127 in 9383c / r16426.

• there is a missing * at line 36;
• lines 519/520 and 583/584: by moving the concatenation from one place to another, the number of StringBuilder objects remains the same. You can either:
  • explicitly construct the StringBuilder and manually invoke append() for each of the terms to be concatenated;
  • force construction of a single StringBuilder by inlining the intermediate String to obtain a flat concatenation expression. This will complicate a bit the expression so I my recommendation is to use the explicit solution here.

Note:
While in JDK8 the compiler was injecting the StringBuilder constructor and the append() calls directly in generated class, with JDK17 things have changed. Instead, the makeConcatWithConstants() method of java.lang.invoke.StringConcatFactory is injected with all concatenation terms as parameters. This will analyze them and pick a concatenation strategy. In this case, it will ultimately use a StringBuilder constructor and the required append() methods, as well.

Committed revision 16427 on task branch 9383c:
- Changes to address #9383-129 observation (no functional difference).

There is actually a regression from 9383c on SQLServer.
The issue comes from the fact that you can't use OFFSET LIMIT when your query does not have an ORDER BY clause.

Ovidiu, when the offset parameter and limit is 0, there is a way to avoid including the OFFSET clause in SQL?

I have found where the LIMIT is inserted, and that is because the maxLimit is set. I will set it to 0 since is not required.

Committed revision 16428 on task branch 9383c:
- Fixed SQLServer issue (set the limit to be 0).

On ChUI regression tests, there is one unexpected test which fails now. I am investigating this once the ChUI regression testing for #11228 is done.

About the regression from ChUI Regression tests, I have debugged the test, but unfortunate, I was unable to see the issue and I can't see how the issue relates to my changes.
When running the scenario by hand, I get the expecting passing screen.
I will rerun the tests and check the logs if there is something obvious.

I have found the issue.
I have didn't looked that much when the query is PreselectQuery from conversion.
The issue comes from the firing the results changed listener (returning false on next, prev, first and last calls).
In this case, the query iteration should continue. I will fix this issue and retest with ChUI Regression testing. That is also why the lenient caching threshold makes sense.

Committed revision 16429 on task branch 9383c:
- Fixed regressions from ChUI regression tests.

Now, the ChUI regression tests has passed.

Stefanel, when you have time, please test this branch on your main application (I have asked you last two weeks ago).

Teodor Gorghe wrote:

Stefanel, when you have time, please test this branch on your main application (I have asked you last two weeks ago).

Yes, sorry about that. My main application setup was quite outdated and needed a refresh. That said, it would have been better if I had been reminded sooner. I’ll complete the testing today.

No problem.
I would have been tested by myself, but I am struggling with the disk space.

Tests passed successfully, nothing unusual.

Ok, thanks!
All testing was done and it can be scheduled for merging.

All testing was done and it can be scheduled for merging.

#11154 is the number one priority for testing this. We need to strive to avoid regressing that. Can you ask Artur/Ovidiu to test that?

I have already tested that and this is why I have moved from OFFSET approach to fetch by pk.
Every change that I have done, I have tested this. The performance for that scenario is better than with the trunk right now.

Do you also mean #10275?

Yes, I have made the customer application setup just for that.

Please tell when to port these changes to custom customer build line.

Updated documentation to include how to adjust the batchResultsSize parameter:
https://proj.goldencode.com/projects/p2j/wiki/Database_Configuration#Batch-Results-size