Project

General

Profile

Bug #11027

Unflushed record causes infinite loop

Added by Andrei Plugaru 7 months ago. Updated 6 months ago.

Status:
Test
Priority:
Urgent
Assignee:
Andrei Plugaru
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
trunk/ rev.16374
production:
No
env_name:
topics:

Related issues

Related to Database - Bug #10390: FIND FIRST buffer field change does not propagate into the next FIRST FIRST statements. Internal Test

History

#1 Updated by Andrei Plugaru 7 months ago

While investigating the unit tests for #10247, I got over an infinite loop that can be reproduced with this testcase:

find first test6_8971.
test6_8971.f1 = test6_8971.f1 - 1.
def buffer b_test6_8971 for test6_8971.
for each b_test6_8971 by f1:
    message  b_test6_8971.f1.
end.

This is the schema for test6_8971 table:


In test6_8971, there should be at least 1 record.

First, it is important to understand that the change to the record from test6_8971 table(which decrements the f1 value) is not flushed before the query executes. Then the AdaptiveQuery, which is in dynamic mode, always finds the same record. At all iterations for the query, we will actually retrieve the unmodified record from the DB, but in SQLQuery.hydrateRecordImpl we will look into session cache and will get the changed record from there.
The reason why always get that record from the DB is that in RAQ.executeImpl, the placeholder is the changed record, however in DB we have the unchanged record. In other words, f1 in the record from DB is grater than f1 from the placeholder.
So this statement from the bundle from Test68971__Impl__ as bTest68971 where (coalesce(bTest68971.f1 > ?0, not (bTest68971.f1 = bTest68971.f1), coalesce(?1) is not null)) order by bTest68971.f1 asc, bTest68971.recid asc will return the record the unchanged record from DB.

If the default buffer test6_8971 would have been used instead of the b_test6_8971 it would have worked fine as the record will have been flushed in PreselectQuery.addComponent, before the query is executed.

This leads me to think that the solution is to flush the record from test6_8971. Maybe when b_test6_8971 is defined?

#3 Updated by Alexandru Lungu 7 months ago

This leads me to think that the solution is to flush the record from test6_8971. Maybe when b_test6_8971 is defined?

This is not correct. Flushing always means WRITE triggers and validation is called (+ visibility of record across all indexes). If OE doesn't call WRITE trigger for b_test6_8971, then it is not flushed.
We should account for this exact case:

On first run:
  • search in dirty database and find the changed record.
  • search in persistent database and find the unchanged record.
  • if they have the same id:
    • if the dirty one is bigger, then use the unchanged record as placeholder and try again.
    • if the persistent one is bigger, then it is fine to use the dirty image. This is what happens I suppose.
On second run (NEXT):
  • another search will find the unchanged record (which as bigger) - nothing in the dirty bigger than the placeholder now.
  • check if the found record has a dirty image. If it does (which does in your example):
    • if it is smaller, then set the found changed record as placeholder and try again (this means that the found record in database is modified and "is in the past")
    • I don't think it is possible to be bigger because it would have been found by the dirty database look-up.
  • mind that in dirty you may found another record that may be bigger and this is resoluted differently. I am talking only about same id cases.

#4 Updated by Andrei Plugaru 7 months ago

A few thoughts I want to note before going into vacation :))
A first problem I am encountering is that there is no pristine record from the persistent database in that scenario. The record that gets into the dmo variable is from session cache and therefore it contains the changes done previously(test6_8971.f1 = test6_8971.f1 - 1. in my scenario):

                     dmo = persistence.load(buffer,
                                            fql,
                                            parms,
                                            lockType,
                                            timeout,
                                            unique,
                                            updateLock,
                                            false,
                                            findByRowid,
                                            onlyId);

So, I am not sure how to really implement the algorithm described above as it relied on a pristine record from the persistent DB. However, I have been thinking about something else.
In RAQ.executeImpl, during the NEXT iterations, the dirty info variable looks like this DirtyInfo [dirtyDMO=null, comparableDMO=null, status=1]. Status 1 means the record found from the primary database has been changed. However, the other DMOs are null as the dirty DB, doesn't contain a 'bigger' record. I am wondering if we can draw the following conclusion based on the values in DirtyInfo. If the record found in the primary DB(I mean the one found with persistence.load()) has been changed(aka is dirty) and there is no record in the dirty DB to satisfy the conditions, this means the found record in the primary DB should be discarded?

#6 Updated by Alexandru Lungu 6 months ago

  • Priority changed from Normal to Urgent
  • Status changed from New to WIP
  • Assignee set to Andrei Plugaru

#7 Updated by Alexandru Lungu 6 months ago

A first problem I am encountering is that there is no pristine record from the persistent database in that scenario. The record that gets into the dmo variable is from session cache and therefore it contains the changes done previously(test6_8971.f1 = test6_8971.f1 - 1. in my scenario):

I totally see the problem now. The dmo is the modified version instead of the pristine one in the DB. In other words, it is potentially not satisfying the FQL used. This is a bit dangerous as it feels abnormal (use load with f1 > 0 and get a record that has f1 = 0 because in the DB it is indeed 1, but the session cache shows that it is CHANGED). Anyway...

In RAQ.executeImpl, during the NEXT iterations, the dirty info variable looks like this DirtyInfo [dirtyDMO=null, comparableDMO=null, status=1]. Status 1 means the record found from the primary database has been changed. However, the other DMOs are null as the dirty DB, doesn't contain a 'bigger' record. I am wondering if we can draw the following conclusion based on the values in DirtyInfo. If the record found in the primary DB) has been changed(aka is dirty) and there is no record in the dirty DB to satisfy the conditions, this means the found record in the primary DB should be discarded?

I think it can be discarded, but what will be the next step? You still need to provide back the "next" record, which may be in the persistent database unchanged. For example, you search a record that is 1 in the DB, but 0 in the session cache. Dirty shows that it is not OK to use it, but doesn't tell whether a 2 still existing in the DB. If it exists, we still need the 1 placeholder so that running the query again will find the 2 in the DB. Also, it is possible that the dirty to find another dirty record that is bigger. I am not sure this is a problem, but DirtyInfo [dirtyDMO=null, comparableDMO=null, status=1] is a particular case.

Historically, we had dirty-share mechanism to be used with cross-session logic. There, "comparable" and "dirty" DMO were described as two different things, mapping what is leaked vs what can be found by the same session. One of them is used as a pristine snapshot, while the other it the latest version of the DMO. These principles do not map properly to inter-session dirty-share, so we hacked the DefaultDirtyShareManager to use flags (cross- vs inter-) to drive its behavior. With intra- (and without cross-), there is virtually no need of comparable vs dirty distinction because there is no difference in how the same session is meant to "see" that record whether its from its own session or leaked from ... itself (if that makes sense).

I am stressing with this occasion the need to fast-forward #8388 as this #11027 (and most probably its echo in #11096) may get hackish to fix. I mean, we will need to repurpose the "dirty" DMO (the pristine image) and rely on "comparable" DMO for other aspects, but manage its lifecycle for intra-session purposes - like you noticed, it should hold the actual image from the DB, not the one in the session cache.

I wonder how the following behaves:

// presume data is 1, 2, 3
find first test6_8971.
test6_8971.f1 = 10. // 1 -> 10
def buffer b_test6_8971 for test6_8971.
for each b_test6_8971 by f1:
    message  b_test6_8971.f1.
end.

The first record is 1, but its actual session cache value is 10, so the snapshot will be 10, right? This means that the values printed will be only (10), instead of (2, 3, 10)?

#8 Updated by Andrei Plugaru 6 months ago

Alexandru Lungu wrote:

I wonder how the following behaves:
[...]

The first record is 1, but its actual session cache value is 10, so the snapshot will be 10, right? This means that the values printed will be only (10), instead of (2, 3, 10)?

Yes, exactly, it only prints 10.

I mean, we will need to repurpose the "dirty" DMO (the pristine image) and rely on "comparable" DMO for other aspects, but manage its lifecycle for intra-session purposes - like you noticed, it should hold the actual image from the DB, not the one in the session cache.

Just to be clear, you mean that, the so called dirtyDMO from DirtyInfo will need to actually store the pristine record from the DB. Then, back in RAQ.executeImpl, we will use it as the placeholder to retrieve the NEXT records?

On another note, as Persistence.load is returning the DMO doesn't satisfy the FQL received as input, should we at least point out this flaw in the JavaDoc of the method as further usages can also have problems because of this?

#9 Updated by Alexandru Lungu 6 months ago

Just to be clear, you mean that, the so called dirtyDMO from DirtyInfo will need to actually store the pristine record from the DB. Then, back in RAQ.executeImpl, we will use it as the placeholder to retrieve the NEXT records?

Don't take my word for granted; there was no such design in intra-session. In cross-session and only for new records, the dirtyDMO and comparableDMO have functional goals. I don't recall the exact quirks, but the point is that the newly leaked DMO can be updated in session A without the update to be seen by session B (until the full index is updated). So one of them is more or less the pristine state and the other is the last state. But, again, this was by intend in the OG implementation of the cross-session dirty-share for transient records.

In the meantime, we leveraged this database to support intra-session and updates. So some of that design doesn't make sense anymore - for updates the comparable always equals the dirty AFAIK.

On another note, as Persistence.load is returning the DMO doesn't satisfy the FQL received as input, should we at least point out this flaw in the JavaDoc of the method as further usages can also have problems because of this?

You can do the update. Either way, we need a way to identify this pristine image. The way it was done in cross-session was to store it in the DefaultDirtyShareManager (either in earlyInserts or the actual database). For intra-session now, earlyInserts is only a cache for the H2 database :/

Maybe a quick fix would be to gather a non-tracked DMO from the database and use it like that - mark it as COPY (state) to avoid incorrect persistence lifecycle.

#10 Updated by Andrei Plugaru 6 months ago

  • % Done changed from 0 to 30

I have committed 11027a/16342 with some initial changes to load the pristine records from DB and compare with the dirty record in order to understand which one should be used or how to set the placeholder. For now, as the original scenario was with a query iterating in ascending mode, I have only focused on that, however, I will also test and investigate the DESC mode.

#11 Updated by Andrei Plugaru 6 months ago

While investigating how the DESC mode works, I came to an example which doesn't seem to work even with trunk:

find last test6_8971.
test6_8971.f1 = test6_8971.f1 + 1.

def buffer b_test6_8971 for test6_8971.
for each b_test6_8971 by f1 desc:
    message  b_test6_8971.f1.
end.

Basically, I am incrementing f1 on the last record, then iterating over the records in DESC mode.
Unfortunately, the for each doesn't return any results.

#12 Updated by Andrei Plugaru 6 months ago

I have created #11115 for the issue exposed in the last note. In order to test the functionality needed on this task I will use a dynamic query, which seems to work fine in DESC mode.

#13 Updated by Alexandru Lungu 6 months ago

Andrei, there are some tests in xfer entitled adaptive-(no)scroll-set-*. Please adapt them to persistent database and run. I think they were translated to unit-tests (... or maybe not?).

#14 Updated by Andrei Plugaru 6 months ago

  • % Done changed from 30 to 50
  • Status changed from WIP to Review
  • reviewer Alexandru Lungu added

I have committed 11027a/rev. 16343/16344 where I have better handled the case when the query runs in inverse mode. I have tested both with queries that run forward over the index DESC, however also with a query that runs in ASC mode, but is iterated with GET-LAST/GET-PREV.

I have also tested the tests from xfer: adaptive-(no)scroll-set-*. It seems there are already in both persistent and temporary modes. I have tested the persistent ones and my branch doesn't regress anything.

Alex, I think this is ready for a review.

#15 Updated by Alexandru Lungu 6 months ago

Review of 11027a:

  • else if (id.equals(foundID) && info.isModified()) is redundant because the method is returning info anyway. You don't need this conditional to early return info. In this case, it can be removed I suppose.
  • // if the dirty has moved in the direction we are iterating -> use the pristine record as placeholder and try again: what if dirtyDMO.id = dmo.id? Doesn't this mean that the DMO is fine to be used and avoid reattempting?
    • mind that dirtyDMO != null && dirtyDMO.primaryKey().equals(dmo.primaryKey()) is OR (!info.isInserted() && !info.isLeaked()). So it is not guaranteed that dirtyDMO.id = dmo.id.
  • I think there may be a blind-spot when dirtyDMO is null. In this case, I think you attempt the look-up again with placeholder = dmo;. However, the dmo is not pristine, so you can get into an infinite loop right?
    • Something like: you have one record with f1 = 1 and you change it to 0. The query shall find records with f1 > 0. In dirty you won't find anything, but in primary database you will.
  • else if(compWithPristine < 0 && (activeBundleKey FIRST || activeBundleKey LAST)) why only for FIRST/LAST?
  • Please investigate the locking implications of these changes. I guess the initial dmo loading takes care of that.

Please run the cross-session harness from xfer testcases - flushing and triggers, cross-session.

#16 Updated by Andrei Plugaru 6 months ago

Alexandru Lungu wrote:

Review of 11027a:

  • else if (id.equals(foundID) && info.isModified()) is redundant because the method is returning info anyway. You don't need this conditional to early return info. In this case, it can be removed I suppose.

I agree on that.

  • // if the dirty has moved in the direction we are iterating -> use the pristine record as placeholder and try again: what if dirtyDMO.id = dmo.id? Doesn't this mean that the DMO is fine to be used and avoid reattempting?
    • mind that dirtyDMO != null && dirtyDMO.primaryKey().equals(dmo.primaryKey()) is OR (!info.isInserted() && !info.isLeaked()). So it is not guaranteed that dirtyDMO.id = dmo.id.
Well, I don't think that even in the case when dirtyDMO.id = dmo.id, it is always safe to use the found DMO. Let's take this scenario. In DB we have: 1 4 7 in the column we are iterating on. Let's think we are changing the value of 1. I thought of 2 scenarios:
  1. we are modifying 1 -> 6(or any value >4, in order to change the iteration order). This means the for each should walk on this order: 4, 6(this was initially 1), 7. During the 1st iteration, dirtyDMO and dmo will have the same id(where the indexed field is 6). However, this is not the record we should visit first. We should visit 4. So, I put the placeholder on the pristine record(aka 1). So, the next record found will be 4, which is right. Then, the next dmo is rightly chosen because of dmo = processDirtyResults(persistence, dmo, info, lockType, updateLock); .
  2. we are modifying 1 -> 2. This means the for each should walk on this order: 2, 4, 7. In this case it was actually right to use the found dmo, however based on the information available, I couldn't infer if it is indeed the 1st record to be visited. So, it will perform another lookup with placeholder=1
  • I think there may be a blind-spot when dirtyDMO is null. In this case, I think you attempt the look-up again with placeholder = dmo;. However, the dmo is not pristine, so you can get into an infinite loop right?
    • Something like: you have one record with f1 = 1 and you change it to 0. The query shall find records with f1 > 0. In dirty you won't find anything, but in primary database you will.

This case is specifically handled by this:

            if ((DirtyShareSupport.isEnabledCrossSession() || DirtyShareSupport.isEnabledIntraSession()) &&
               activeBundleKey != UNIQUE && info.isModified() && dirtyDMO == null)
            {
               // this handles the case when the record found based in the persistent DB query has been 
               // modified AND there is no record in dirty DB to satisfy the query, this means the 
               // found record doesn't satisfy the query, so will move past it 

               Record pristine = persistence.getSession().loadPristineRecord(buffer.getDMOImplementationClass(),
                                                                             dmo.primaryKey());
               placeholder = pristine;
               dmo = null;
               continue;
            }

  • else if(compWithPristine < 0 && (activeBundleKey FIRST || activeBundleKey LAST)) why only for FIRST/LAST?

Hmm, that was based on some tests, but now that I look one more time, it may need some adjustments. I will think about it....

#17 Updated by Andrei Plugaru 6 months ago

else if(compWithPristine < 0 && (activeBundleKey FIRST || activeBundleKey LAST)) why only for FIRST/LAST?

Hmm, that was based on some tests, but now that I look one more time, it may need some adjustments. I will think about it....

It seems we may actually not need the logic in the else if. If the record is modified backwards, the same record will be found in dirty and our current logic will handle it.
There is however, a scenario based on which I intially added that condition:

find first test6_8971 where test6_8971.f1 = 4.
test6_8971.f1 = -4. // 4 -> -4

def buffer b2_test6_8971 for test6_8971.
find first b2_test6_8971 where b2_test6_8971.f1 = 1.
b2_test6_8971.f1 = 10. // 1 -> 10; just to have it modified

def buffer b_test6_8971 for test6_8971.
for each b_test6_8971 by b_test6_8971.f1:

message  b_test6_8971.f1.
end.

However, this is a slightly different problem than the one initially discovered on this task. During the FIRST iteration, the dirty record and the one found in the primary DB are different. In dirty, the record with f1=-4 is found, from the primary DB the record with f1=10 is found. They obviously have different recIds. So, my changes shouldn't handle this scenario.
The problem is that in the current implementation, it will get to:

               // The found record must be skipped because it was either
               // deleted or modified on the index we're walking.  Update the
               // placeholder to be the record we actually found...
               placeholder = dmo;
               dmo = null;

               // ...then try another pass.
               continue;

So, the placeholder would become the record with f1=10. We actually have logic to choose the right DMO in this situation: dmo = processDirtyResults(persistence, dmo, info, lockType, updateLock);, however it doesn't get to this because it enters this if:

            if ((DirtyShareSupport.isEnabledCrossSession() || DirtyShareSupport.isEnabledIntraSession()) &&
                activeBundleKey != UNIQUE &&
                (info.isDeleted() ||
                   (info.isModified() &&
                      ((!info.isInserted() && !info.isLeaked()) || 
                         (dirtyDMO != null && dirtyDMO.primaryKey().equals(dmo.primaryKey()))))))

From the OR clause in the end, this part is true: ((!info.isInserted() && !info.isLeaked()), the primary keys don't match.

I will commit soon the small changes based on the review, however, I would want to continue testing the current changes and solve the issue from this note in another iteration of this task.

#18 Updated by Alexandru Lungu 6 months ago

I will commit soon the small changes based on the review, however, I would want to continue testing the current changes and solve the issue from this note in another iteration of this task.

Please ensure that whatever if prepared in #11027 is not regressing. If the note in #11027-17 is a regression of your changes (works in trunk but not with your changes), then we need to fix it as well.

#19 Updated by Andrei Plugaru 6 months ago

Alexandru Lungu wrote:

I will commit soon the small changes based on the review, however, I would want to continue testing the current changes and solve the issue from this note in another iteration of this task.

Please ensure that whatever if prepared in #11027 is not regressing. If the note in #11027-17 is a regression of your changes (works in trunk but not with your changes), then we need to fix it as well.

It's not fully functional either in trunk, nor in this branch. However, we can consider it is working 'slightly' better with trunk. With trunk it finds the record with f1=10, with my changes it doesn't find any :((

#20 Updated by Andrei Plugaru 6 months ago

I have committed 11027a/ rev. 16345 with the removal of the redundant if in DirtyShareContext.java and also wrapping my main changes from RAQ inside an if to check if the dirty dmo is the same as the one found based on the persistent DB query.

With this revision I have run the following:
  1. harness tests from flushing_and_triggers/cross_session (xfer testcases)
  2. harness tests from /support/harness/multi-session-tests (xfer testcases)
  3. tests from tests/query/adaptive/persistent (xfer testcases)

Neither of them show any regressions.

With these changes the main scenario targeted by this task(explained in #11027-1) is solved. There is, however, the testcase from #11027-17 which isn't covered by my changes.

#21 Updated by Andrei Plugaru 6 months ago

  • Related to Bug #10390: FIND FIRST buffer field change does not propagate into the next FIRST FIRST statements. added

#22 Updated by Andrei Plugaru 6 months ago

  • Status changed from Review to WIP
I committed 2 more revisions:
  1. rev. 16346: here it is a simple fix to activate next/prev when entering this if ((DirtyShareSupport.isEnabledCrossSession() || DirtyShareSupport.isEnabledIntraSession()) &&
    activeBundleKey != UNIQUE && info.isModified() && dirtyDMO == null)
    This came based on #10390. Basically, without this fix, my changes would enter an infinite loop when no record was found.
  2. rev. 16347: the fix here basically solves the issue from the scenario in #11027-17. I will explain below how I got to that fix.
    So, let's assume again that in persistent DB we have some records with this values on the indexed fields: 1 4 7. 1 gets into 10. 4 gets into -4. So, this is the view and order over the records: -4(in persistent 4), 7, 10(in persistent 1). During the 1st iteration, from persistent we would get this 10(in persistent 1), from dirty -4(in persistent 4). It's pretty simple straightforward that this -4(in persistent 4) should have been used. Then, during the next iteration, we would just set the placeholder to 1 and try again. However, at this stage an interesting problem occurs. From the persistent DB, this record would be returned -4(in persistent 4), however from dirty DB, this one: 10(in persistent 1). Neither of them is good. So, i came up with this algorithm. If we are in the case when the record found based on the persistent DB has been modified AND the dirty and persistent records do not match, then compute the pristine records of the one found in persistent DB:
    • if the dirty record is bigger than the pristine, just use the pristine as placeholder and try again. The dirty one will be eventually fond, however we need to also find intermediate records.
    • if the dirty record is smaller than the pristine, return the dirty DMO.

I will redo the testing mentioned on this task, then ask again for a review.

#23 Updated by Teodor Gorghe 6 months ago

Andrei, I have checked your changes and it makes sense.
There is one change that I would do to your change.
Computing the pristine record is not necessarily needed when the cached DMO already reflects what is in database (notice condition dmo.checkState(CACHED) && dmo.checkState(CHANGED)).
This check may seem to be redundant but can be useful in the future if the Session cache implementation is being changed.
Also, if you noticed, the issue from #10390 does not occur when using testing with #8388. That is because Persistence.load looks into the dirty database and the difference is that the record is not being returned.

Don't forget about history entries and also there are some lines which exceeds maximum character limit of 110.

#24 Updated by Andrei Plugaru 6 months ago

Teodor Gorghe wrote:

Andrei, I have checked your changes and it makes sense.
There is one change that I would do to your change.
Computing the pristine record is not necessarily needed when the cached DMO already reflects what is in database (notice condition dmo.checkState(CACHED) && dmo.checkState(CHANGED)).

Thanks, Teodor.
However, I don't think I fully understand the need for dmo.checkState(CACHED) && dmo.checkState(CHANGED). My main changes are already conditioned by info.isModified(). This implies the DMO has been changed(aka doesn't reflect what is in the database).

#25 Updated by Teodor Gorghe 6 months ago

It is a safety check that we can ensure that the DMO hydrated values comes from database or from Session cache.

#26 Updated by Andrei Plugaru 6 months ago

  • Status changed from WIP to Review

I have committed 11027a/16348 with the safe check proposed by Teodor and also a missing break in a case where the dirty dmo is used.

I have redone the testing:
  1. harness tests from flushing_and_triggers/cross_session (xfer testcases)
  2. harness tests from /support/harness/multi-session-tests (xfer testcases)
  3. tests from tests/query/adaptive/persistent (xfer testcases)

Neither show any regressions. The good part is that the harness tests from flushing_and_triggers/cross_session are all passing with my branch!

I think this branch is ready for a review again!

#27 Updated by Andrei Plugaru 6 months ago

While running Chui regression tests found some regressions :((
Currently I am looking into the following error: Requested to persist DMO of type ... with id 120984993. A different DMO instance of type .. is already bound to this session with the same id
After some debugging I think I may understand the scenario which leads to this:
  • We have record A in a buffer
  • In RAQ.executeImpl, we decided to use a dirty DMO with the same Id. We have logic there to evict the non-dirty DMO from session cache:
             if (dirtyCopy && primaryDMOs != null)
             {
                // We are not going to use the primary DMO(s) we found, so make
                // sure they are evicted from the session if not in use elsewhere
                // in this context.
                for (Record pDMO : primaryDMOs)
                {
                   buffer.evictDMOIfUnused(pDMO);
                }
             }
    

    However, this doesn't evict as it is still being referenced by this buffer. Maybe buffer.evictDMOIfUnused should evict the record if it is not being referenced by any other buffer except the current one.

#28 Updated by Teodor Gorghe 6 months ago

Andrei Plugaru wrote:

While running Chui regression tests found some regressions :((
Currently I am looking into the following error: Requested to persist DMO of type ... with id 120984993. A different DMO instance of type .. is already bound to this session with the same id
After some debugging I think I may understand the scenario which leads to this:
  • We have record A in a buffer
  • In RAQ.executeImpl, we decided to use a dirty DMO with the same Id. We have logic there to evict the non-dirty DMO from session cache:
    [...]
    However, this doesn't evict as it is still being referenced by this buffer. Maybe buffer.evictDMOIfUnused should evict the record if it is not being referenced by any other buffer except the current one.

Yes, that makes sense.

#29 Updated by Andrei Plugaru 6 months ago

I may have found a change that could be the root cause(for at least) a few regressions:

=== modified file 'src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java'
--- old/src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java    2025-10-16 06:12:46 +0000
+++ new/src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java    2026-01-20 09:16:57 +0000
@@ -990,13 +990,6 @@
                      return null;
                   }
                }
-               // otherwise if we have a candidate and it exists in DirtyShare database ignore DirtyInfo,
-               // our candidate shall prevail
-               else if (foundID != null && info.isModified())
-               {
-                  return null;
-               }
- 
             }
          }
       }

This results in returning the actual DirtyInfo in more cases. In RAQ.executeImpl it has the consequence of returning the dirtyDmo more times.

I think my changes in DirtyShareContext.java may have broken this behaviour:

            // ignore validated changes that are driven by this context; they are already
            // reflected in the primary database

In the chui tests, this is exactly what happens: the dirty record seems to be an older one, and the found record in the primary database seems to have all the changes.

However, my changes there have been done exactly because the primary database didn't have all the changes. In this case, the opposite happens...

#30 Updated by Alexandru Lungu 6 months ago

In the chui tests, this is exactly what happens: the dirty record seems to be an older one, and the found record in the primary database seems to have all the changes.

This is odd. Usually dirty database has the absolute latest data. Can you find why this is not the case here? Is it due to the fact that cross-session is omitting some dirty flushes? If so, we can conditionally insert that code for cross- vs intra-.

#31 Updated by Andrei Plugaru 6 months ago

Actually, the record that comes from the dirty DB seems to be the latest. The problem is that in DefaultDirtyShareManager.getDirtyInfo that record is not used, instead it is used one from leaks map which seems to be older:

                        // then check for cross session records
                        copy = leaks.get(ident);
                        if (copy != null)
                        {
                           info = new DirtyInfo();
                           info.setLeaked();
                           info.setDirtyDMOs(copy, dmo);
                           break;
                        }

#32 Updated by Alexandru Lungu 6 months ago

There is a thing that distinguishes cross- from intra-: if the record is leaked from another session, then the first image that is leaked is visible; if updates are done on it afterwards, then these are not seen by the other session. I also thing this is valid per-index. I don't have 100% a clear algorithm for this, but AFAIK this is something to take into account.

Now, the leak should mean that the record actually comes from another session, but in your case I think leaks gets you back a record for the same session? Or not?

#33 Updated by Andrei Plugaru 6 months ago

I may have found a solution/workaround for the problem. As basically the problem was returning a wrong dirtyRecord back to RAQ.executeImpl, I have went back and adjusted my changes in DirtyShareContext.getDirtyInfo:

=== modified file 'src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java'
--- old/src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java     2026-01-21 08:49:09 +0000
+++ new/src/com/goldencode/p2j/persist/dirty/DirtyShareContext.java     2026-01-23 12:44:26 +0000
@@ -990,6 +990,10 @@
                      return null;
                   }
                }
+               else if(id.equals(foundID) && info.isModified())
+               {
+                  info.setDirtyDMOs(found, found);
+               }
             }
          }
       }

If we get to execute that piece of code, we would want the changes from that context. So, with these changes, it will just set back the dirty dmos as the one found in current session cache, overriding what it found in leaks.
I am running the chui tests with this patch and didn't get any unexpected failures in 60% of the tests(I will of course run all of them).
I will also redo the other testing mentioned on this task.

#34 Updated by Andrei Plugaru 6 months ago

I have committed 11027a/16349 with the fix from #11027-33.

Apart from the testing from #11027-26, the ChUi tests don't show any regressions. It may also solve some tests as only gso_282 and tc_ap_rpts_002 fail. Based on #10615-75, there are also other tests that usually fail. I have started the unit tests for a large customer application and the results are ok so far.

#35 Updated by Andrei Plugaru 6 months ago

While testing the unit tests of a large GUI applications, I found some more issues, regarding the choice of the dirty record. I was just returning the dirty record instead of using processDirtyResults which better handles some edge cases. However, after using it, I was started getting some other issues. More exactly, because of this case:

         else if (dirtyDMO != null)
         {
            // Just use the primary key of the record found in the dirty
            // database, but reload the record from the primary database.
            // Note:  it might not be there, if the record existed only in
            // another context, and was never committed.
            // in that case return primaryDMO as there is no dirtyDMO to replace it
            Long id = dirtyDMO.primaryKey();
            try
            {
               long timeout = persistence.getTimeOut();
               dmo = persistence.load(buffer.getDMOImplementationClass(), id, lockType, timeout, updateLock);
            }
            catch (MissingRecordException exc)
            {
               dmo = primaryDMO;
            }
            if (dmo == null)
            {
               dmo = primaryDMO;
            }
         }

Basically, here, it loads the DMO with the dirty's id from the primary DB, however, it is possible the loaded one to not match the FQL anymore. I tried to fix this by checking if the loaded DMO is smaller than the dirty one. If the loaded one is smaller, I assume it is not valid. If it were valid it would have been found in the 1st place.

This change is in 11027a/16351.

With this changes, this testing has been done:
  • harness tests from flushing_and_triggers/cross_session (xfer testcases)
  • harness tests from /support/harness/multi-session-tests (xfer testcases)
  • tests from tests/query/adaptive/persistent (xfer testcases)
  • chui tests(85% done - no problems so far)
  • unit tests of large GUI application
  • harness tests of large application (as)

#36 Updated by Teodor Gorghe 6 months ago

There is a regression on a large customer application caused by the latest revision.
I am attaching the stack trace here and I can provide more details on email:

Caused by: java.lang.NullPointerException
    at com.goldencode.p2j.persist.RandomAccessQuery.processDirtyResults(RandomAccessQuery.java:6486)
    at com.goldencode.p2j.persist.RandomAccessQuery.executeImpl(RandomAccessQuery.java:6187)
    at com.goldencode.p2j.persist.orm.P2JQueryExecutor.executeImpl(P2JQueryExecutor.java:455)
    at com.goldencode.p2j.persist.orm.P2JQueryExecutor.execute(P2JQueryExecutor.java:351)
    at com.goldencode.p2j.persist.orm.P2JQueryExecutor.execute(P2JQueryExecutor.java:205)
    at com.goldencode.p2j.persist.RandomAccessQuery.execute(RandomAccessQuery.java:4629)
    at com.goldencode.p2j.persist.RandomAccessQuery.findNext(RandomAccessQuery.java:5338)
    at com.goldencode.p2j.persist.RandomAccessQuery.next(RandomAccessQuery.java:2308)
    at com.goldencode.p2j.persist.RandomAccessQuery.next(RandomAccessQuery.java:2146)
    at com.goldencode.p2j.persist.AdaptiveQuery$DynamicResults.next(AdaptiveQuery.java:4300)
    at com.goldencode.p2j.persist.ResultsAdapter.next(ResultsAdapter.java:162)
    at com.goldencode.p2j.persist.AdaptiveQuery.next(AdaptiveQuery.java:1878)
    at com.goldencode.p2j.persist.PreselectQuery.next(PreselectQuery.java:2864)
    at com.goldencode.p2j.util.BlockManager.forEachWorker(BlockManager.java:12080)
    at com.goldencode.p2j.util.BlockManager.forEach(BlockManager.java:4812)

#37 Updated by Andrei Plugaru 6 months ago

I think a simple null check there will solve the issue.

I will redo some of the testing I am handling as I don't think it should regress the actual fix, then ask again to check your application.

#38 Updated by Alexandru Lungu 6 months ago

  • Status changed from Review to Merge Pending

11027a passed ETF and POC testing without any observable performance degradation. Please merge 11027a to trunk now.

#39 Updated by Andrei Plugaru 6 months ago

Before merging I realized the changed file didn't have history entries. I added that, and also changed the copyright year in 11027a/ rev.16353. Of course, no other changes added!
I will do the merge now.

#40 Updated by Andrei Plugaru 6 months ago

Branch 11027a was merged into trunk as rev. 16374.

#41 Updated by Greg Shah 6 months ago

  • Status changed from Merge Pending to WIP

#42 Updated by Andrei Plugaru 6 months ago

  • % Done changed from 50 to 100

Greg, at this moment, all the testing done for this task passed and also, didn't found any testcases related to this task that have wrong behaviour.

#43 Updated by Andrei Plugaru 6 months ago

  • version_resolved set to trunk/ rev.16374

#44 Updated by Greg Shah 6 months ago

  • Status changed from WIP to Test

Also available in: Atom PDF