FWD » Core Development » Database

Created task branch 8593a_h2 which improves the DELETE FROM tt1 WHERE tt1._multiplex = ?. The reasoning is this:

• all remaining indexes assume the PK is the first column (I need confirmation that this will be always true), and just find that record in the index and remove it.

If you meant PK as recid, this is always the last column in an index, but the first column in the row. If you meant PK as H2 internal row key, this is kept outside the data array and I think it is not ever indexed.
This being said, you can't remove a row only based on the recid/PK from a tree index. You can however prune the tree index based on multiplex (as it is the first field in the index).
Please let me know if I misunderstood your intent. Without a full-scan of the index, you can't find a row solely on its PK.

One last important thing is that you still need to log the deletes. so you will most certainly need to "register" the row as being deleted in case of a roll-back (unless you do this only for NO-UNDO tables).

I didn't look into the changes yet.

You mean the Row.key is not the same as the internal PK?

Also, there is 'recid' index (with just this column) in H2 created for FWD temp-tables.

Constantin Asofiei wrote:

You mean the Row.key is not the same as the internal PK?

I mean Row.key to be the same as the 'recid' PK we set on position zero in the row's data array.

Alexandru, the assumption was that the MultiplexedBatchIndex$Batch holds data only for a certain multiplex. This is not correct (maybe because of re-used keys?).

Currently a batch can have rows for more than one multiplex ID.

Constantin Asofiei wrote:

Alexandru, the assumption was that the MultiplexedBatchIndex$Batch holds data only for a certain multiplex. This is not correct (maybe because of re-used keys?).

Currently a batch can have rows for more than one multiplex ID.

This is absolutely no good. The invariant shouldn't be (ever) broken. Do you have a scenario that triggers such anomaly?

• mpid1.p
  

  def temp-table tt1 field f1 as int index ix1 f1.
  def var k as int.
  
  procedure proc0.
     def var i as int.
     run mpid2.p(input-output table tt1).
     buffer tt1:empty-temp-table().
     do i = 1 to 10:
        create tt1.
        tt1.f1 = i.
        release tt1.
     end.
  end.
  
  do transaction:
     run proc0.
  end.
  
  

• mpid2.p
  

  def temp-table tt1 field f1 as int index ix1 f1.
  
  def var i as int.
  def input-output param table for tt1.
  
  do i = 1 to 10000:
     create tt1.
     tt1.f1 = i.
     release tt1.
  end.
  
  for each tt1: delete tt1. end.
  
  

For an undoable temp-table, a EMPTY-TEMP-TABLE results in a UPDATE tt1 SET _multiplex = <delete-multiplex> WHERE _multiplex = <old-multiplex>. The assumption with the batches was that the multiplex ID is read-only - this is not true for undoable temp-tables, where a delete is emulated via changing the multiplex (and thus hiding the record from the legacy temp-table).

Constantin Asofiei wrote:

For an undoable temp-table, a EMPTY-TEMP-TABLE results in a UPDATE tt1 SET _multiplex = <delete-multiplex> WHERE _multiplex = <old-multiplex>. The assumption with the batches was that the multiplex ID is read-only - this is not true for undoable temp-tables, where a delete is emulated via changing the multiplex (and thus hiding the record from the legacy temp-table).

Do you mean the INPUT-OUTPUT TABLE functionality, right? I see your point, but I thought this wasn't implemented yet (it seems it was implemented in the mean-time).

Anyway, the UPDATE in H2 is a DELETE + INSERT. All records will be first removed (doing something like a DELETE ... WHERE multiplex = <old-multiplex>) and then added (doing an insert for each row). The only issue I see is the recid. It is still the old recid from the source table. I am mostly sure this is wrong. 4GL is reindexing the records, so we shall do this the same (reserve id before insert).

1. How urgent is this? Anything breaking right now?
2. We can have a direct-access API for this (changeMultiplex). It will basically do the UPDATE + reindexing in the updating.
3. Otherwise, we can have a special SQL keyword (REINDEX) for the UPDATE, that is reindexing the records after update.

No, the empty-temp-table is the problem, while executing in a transaction, for an undoable temp-table - this does not remove the records, but changes the multiplex ID.

For MultiplexedScanIndex, the target batch in setRow(key, row) is calculated from the record's key (which is not changing!) - so a remove/add will end up in the same batch.

Urgency: the performance for #8363 is urgent, the bug appears only when introducing 8593a_h2 in ETF.

For MultiplexedScanIndex, the target batch in setRow(key, row) is calculated from the record's key (which is not changing!) - so a remove/add will end up in the same batch.

Hmm, I see. Drop my statement then - the INPUT-OUTPUT tables are not yet implemented with UPDATE.

Still, why are we updating with a <delete-multiplex> instead of actually deleting them? If they are undoable, we can rollback (with a savepoint or not). Why do we need to have such functionality of marking them as deleted?

Alexandru Lungu wrote:

Still, why are we updating with a <delete-multiplex> instead of actually deleting them? If they are undoable, we can rollback (with a savepoint or not). Why do we need to have such functionality of marking them as deleted?

This technique of updating the multiplex ID to emulate an undoable delete predates (by almost two decades) our use of savepoints and any editing of H2 source code.

Seems like a long-standing leak of records. If that built up to a large number, it might be both a memory leak and a drag on performance.

Greg Shah wrote:

Seems like a long-standing leak of records. If that built up to a large number, it might be both a memory leak and a drag on performance.

No, this "emulated delete" only happens at the subtransaction level, so that an UNDO is able to restore the records by switching the multiplex ID back. They are actually deleted at transaction commit, or restored at transaction rollback. At least, that was the original implementation; I can't speak to this new issue that Constantin detected.

Eric, even for a sub-transaction. We can simply delete and rollback to savepoint, right?

Alexandru Lungu wrote:

Eric, even for a sub-transaction. We can simply delete and rollback to savepoint, right?

Yes, we should be able to now. We just weren't using savepoints back then, and this technique has survived all these years. If you make this change, we should test thoroughly, since this has been a core design point from the beginning. I don't recall if/where we have dependent assumptions on this approach in the runtime.

Just a thing that was on my mind and wanted to have it here as well:

For NO-UNDO cases, we used to rollback (transaction or savepoint) and redo the state of the NO-UNDO tables (i.e. rollbacking the rollback). This is no longer the case as FWD-H2 has the NO-UNDO keyword in place, so rollbacks are only for UNDO tables.

Yes, we should be able to now. We just weren't using savepoints back then, and this technique has survived all these years. If you make this change, we should test thoroughly, since this has been a core design point from the beginning. I don't recall if/where we have dependent assumptions on this approach in the runtime.

Ok

Previously we used the UPDATE ... SET _MULTIPLEX = ? paired with a ReversibleBulkDelete, which was meant to restore the multiplex ID (via a reverse UPDATE), in case of rollback of an undoable temp-table.

ReversibleBulkDelete was removed I think when we introduced no-undo temp-tables directly in FWD-H2 (or the savepoints?).

I do not think we actually need the UPDATE ... SET _MULTIPLEX = ? in TemporaryBuffer.removeRecords anymore, the forceDelete flag should always be true. Currently is set to:

forceDelete = forceDelete || !isUndoable() || !bufferManager.isTransaction()

• !isUndoable() means will be true for NO-UNDO temp-tables
• !bufferManager.isTransaction() means we are not in a transaction

• when we are inside a transaction
• when the temp-table is undoable

I'm testing this approach now. Please let me know if you see anything wrong.

The FWD direct-access changes are OK!
Also, getting rid of the forceDelete is good, but requires extensive testing!

• rowCount -= removed.cardinality; is something that is theoretical irrelevant. The cardinality should always be 0 when killing the multiplex. the killMultiplex API always relies that this is a batch clear-up attempt, not an actual removal of rows. If you encountered this as being "required", it means some kind of invariant was broken.
• return i SearchRow.ROWID_INDEX ? ValueLong.get(key) : data null ? null : data[i]; why is data allowed to be null in the first place?
• of course, this also requires extensive testing.

Alexandru Lungu wrote:

The FWD direct-access changes are OK!
Also, getting rid of the forceDelete is good, but requires extensive testing!

ETF testing passed with the current changes, and also was the reason why these bugs surfaced.

• rowCount -= removed.cardinality; is something that is theoretical irrelevant. The cardinality should always be 0 when killing the multiplex. the killMultiplex API always relies that this is a batch clear-up attempt, not an actual removal of rows. If you encountered this as being "required", it means some kind of invariant was broken.

• rowCount at the MultiplexedScanIndex tracks only the 'flushed' records - it does not include the 'rows allocated for primary key reservation'
• cardinality at the batch tracks all rows added to the batch.

So that was the reason I had to add 'clearPrimaryKey' for unflushed records which are deleted/rollback, as otherwise the DELETE ... WHERE _MULTIPLEX = ? statement in TemporaryBuffer.removeRecords failed the invariant.

• return i SearchRow.ROWID_INDEX ? ValueLong.get(key) : data null ? null : data[i]; why is data allowed to be null in the first place?

I think this was because of the 'template row' with null data used to allocate a row to reserve a primary key in a batch.

Constantin Asofiei wrote:

• rowCount -= removed.cardinality; is something that is theoretical irrelevant. The cardinality should always be 0 when killing the multiplex. the killMultiplex API always relies that this is a batch clear-up attempt, not an actual removal of rows. If you encountered this as being "required", it means some kind of invariant was broken.

Well, the invariant was/is already broken:

• rowCount at the MultiplexedScanIndex tracks only the 'flushed' records - it does not include the 'rows allocated for primary key reservation'
• cardinality at the batch tracks all rows added to the batch.

So that was the reason I had to add 'clearPrimaryKey' for unflushed records which are deleted/rollback, as otherwise the DELETE ... WHERE _MULTIPLEX = ? statement in TemporaryBuffer.removeRecords failed the invariant.

Do you mean that the invariant was broken and now it was fixed by clearPrimaryKey? I understood the fact that there were "leaking reservations" and these were removed by the rollback changes.

The original code always presumed that the records were deleted before the multiplex was killed, so we always presumed that the cardinality if 0 when killing the batches. I agree this was broken before your changes, but now it should be fixed, right? So rowCount -= removed.cardinality; is no longer needed.

I think this was because of the 'template row' with null data used to allocate a row to reserve a primary key in a batch.

Oh yeah, right. The code is reserving slots with a null data for the sake of performance/memory.

Alexandru Lungu wrote:

Do you mean that the invariant was broken and now it was fixed by clearPrimaryKey? I understood the fact that there were "leaking reservations" and these were removed by the rollback changes.

Is still somehow broken: a template row for the allocated primary key is not added to the other indexes. So the row count at MultiplexedScanIndex, the table and other indexes will always have the 'flushed row count', while if you sum the batches' cardinality, this will be different.

The original code always presumed that the records were deleted before the multiplex was killed, so we always presumed that the cardinality if 0 when killing the batches. I agree this was broken before your changes, but now it should be fixed, right? So rowCount -= removed.cardinality; is no longer needed.

I want to keep it, as at the moment the multiplex is deleted, we want to ensure all is stable.

Also, I think the actual DirectAccessHelper.killMultiplex is no longer needed - it tries to remove rows which were previously removed via the DELETE from TemporaryBuffer.removeRecords, plus it clears only the MultiplexedScanIndex - what about the other indexes?

Constantin Asofiei wrote:

Alexandru Lungu wrote:

Do you mean that the invariant was broken and now it was fixed by clearPrimaryKey? I understood the fact that there were "leaking reservations" and these were removed by the rollback changes.

Is still somehow broken: a template row for the allocated primary key is not added to the other indexes. So the row count at MultiplexedScanIndex, the table and other indexes will always have the 'flushed row count', while if you sum the batches' cardinality, this will be different.

Oh, the invariant I was talking about is removed.cardinality 0 when killing a multiplex. I agree that the sum of cardinalities is different from the rowCount.

Also, I think the actual DirectAccessHelper.killMultiplex is no longer needed - it tries to remove rows which were previously removed via the DELETE from TemporaryBuffer.removeRecords, plus it clears only the MultiplexedScanIndex - what about the other indexes?

The need for killMultiplex was due to the empty batches that were left behind. For UNDO tables, empty batches are left behind in case an UNDO happens. For instance, if you remove a record, the batch (even empty) will stay alive for UNDOABLE tables, waiting for an UNDO. If the UNDO never comes, the batch will leak. killMultiplex was intentionally designed to only drop the empty batches of UNDOABLE tables. That is why the removed.cardinality 0 invariant should hold when killing the multiplex and that is why we always need to clear the table before killing the multiplex.

Alexandru Lungu wrote:

Oh, the invariant I was talking about is removed.cardinality 0 when killing a multiplex. I agree that the sum of cardinalities is different from the rowCount.

When the actual killMultiplex(multiplexId) is being called from FWD, no more 'primary key allocation rows' should exist; also, all batches should be already removed because of the removeAll(session, multiplexID) call.

The need for killMultiplex was due to the empty batches that were left behind. For UNDO tables, empty batches are left behind in case an UNDO happens. For instance, if you remove a record, the batch (even empty) will stay alive for UNDOABLE tables, waiting for an UNDO. If the UNDO never comes, the batch will leak. killMultiplex was intentionally designed to only drop the empty batches of UNDOABLE tables. That is why the removed.cardinality 0 invariant should hold when killing the multiplex and that is why we always need to clear the table before killing the multiplex.

I need to double-check for undoable temp-tables. The case I'm thinking is a empty-temp-table call followed by undo, leave. This would trigger a DELETE ... FROM ... _MULTIPLEX = ?, which will call removeAll and also remove all the batches. The rollback operation should create the batches again.

Constantin Asofiei wrote:

I need to double-check for undoable temp-tables. The case I'm thinking is a empty-temp-table call followed by undo, leave. This would trigger a DELETE ... FROM ... _MULTIPLEX = ?, which will call removeAll and also remove all the batches. The rollback operation should create the batches again.

This is a huge thing. Removing a batch will make the MultiplexScanIndex quite unstable. If you want to recreate it, you will need to do some kind of linked list traversal to find its spot and "revive" it. This is the exact reason for why I was preferring to leak them and let them be cleared entirely.

I think the removeAll caused by the truncate should only clear the batches (not actually remove them). In fact, it can remove the batches only for NO-UNDO. Also, mind that the "last" batch is a bit more special as it can reclaim keys. Therefore, never remove a batch if it is the "last" for a multiplex.

Alexandru Lungu wrote:

This is a huge thing. Removing a batch will make the MultiplexScanIndex quite unstable. If you want to recreate it, you will need to do some kind of linked list traversal to find its spot and "revive" it. This is the exact reason for why I was preferring to leak them and let them be cleared entirely.

I think the removeAll caused by the truncate should only clear the batches (not actually remove them). In fact, it can remove the batches only for NO-UNDO. Also, mind that the "last" batch is a bit more special as it can reclaim keys. Therefore, never remove a batch if it is the "last" for a multiplex.

Understood, thanks.

Alexandru, please review 8593a_h2 rev 49/50 and 8593a rev 15158. I'll need to re-test these changes.

Constantin Asofiei wrote:

Alexandru, please review 8593a_h2 rev 49/50 and 8593a rev 15158. I'll need to re-test these changes.

ETF testing passed. Alexandru, if you can run a ChUI regression testing with this and 8363g, please let me know.

ETF testing passed. Alexandru, if you can run a ChUI regression testing with this and 8363g, please let me know.

ChUI takes ~2h:30mins. Do you want to test them both or separately? I would prefer to have them together as I have limited time on the bare-metal due to the intensive performance testing we are doing today.

You can test them both (8593a + 8636g). I've uploaded fwd-h2 1.46 and 8593a rev 15161 has the change in build.gradle, so you can build directly. Just patch 8593a over 8363g.

Alexandru, please let me know if you managed to run ChUI - I'd like to merge these tomorrow.

You can go ahead with the merge of 8593a and 8636g.

Code Review Task Branch 8593b Revision 15179

The LowLevelSocketImpl changes look good.

Eric/Ovidiu: Please look at the persistence changes ASAP.

Code Review Task Branch 8593b.

I do not see anything wrong with the code (r15187-).

8593b passed ETF testing.

This is a test where it gets into an infinite loop:

def temp-table tt1 no-undo field f1 as int index ix1 f1.
def temp-table tt2 like tt1.

do transaction:
  create tt1.
  tt1.f1 = 1.
  release tt1.
end.

for each tt1 where tt1.f1 > 0:
   message tt1.f1.
   run proc0 (input-output table tt1).
end.

procedure proc0.
   def input-output parameter table for tt2.
end.

This can be expanded to have more than one batch. So the 'old implementation' I don't think is OK.

Constantin Asofiei wrote:

This is a test where it gets into an infinite loop:
[...]

This can be expanded to have more than one batch. So the 'old implementation' I don't think is OK.

• If I have only one batch (1 - ~150 records), I have the following output (including the recid):
  

  2304 1
  2305 2
  2306 3
  ...
  2402 99
  2403 100

• If I have more than one batch (I tested with 300 records, I have the following output:
  

  2304 1
  6400 1
  4353 2
  2306 3
  6402 3
  4355 4
  2308 5
  6404 5
  4357 6
  ....
  2667 299
  6763 299
  4716 300

So as said, there is a difference between copying 1 batch and copying more than 1 batch. Apparently it doesn't enter an infinite loop.

• The original rows are in page 1.
• When going into proc0, the rows are moved in page 2.
• When returning, if there are more than one batch, the records go on page 3.
• This way record 1 is iterated again
• proc0 is called and rows are moved to page 1. (see how multiplex 2 is reusing the page 1 that was used by multiplex 1 at first). This is happenning just because the table is NO-UNDO and we know that multiplex 1 can't reuse items from page 1.
• proc0 returns and rows are moved back to page 2. At this point, the for each goes to the element 2.

This approves my idea from #8593-46. It is OK to move records to a new page (higher PK) if more than 1 batch.
The only missing puzzle piece is the page implementation in FWD. Because there are no pages, we always assign higher batches and don't recycle the empty pages.

Pages are always 2048 in size and can contain (apparently) only 8 batches. If all batches are empty, the page can be reused cross-multiplex.

Lastly: For UNDO, the situation is different and requires way more investigation overall (it increases the PK several times until the end of the pages; at that point it seems it forces reclaiming to avoid leaking out the page).

Ovidiu/Eric: Please review 8593c.

This is quite Urgent since it is to resolve a regression in a customer application that has been present for over a week.

I committed now 8593c/rev. 15204. This includes a requirement to use in-DB PK mapping if the table has normalized extent fields. This was found during regression testing.
With this final version, the unit tests full suite of a large customer doesn't regress.

Eric, I am starting the review process in the next minutes. Please let me know if you are already in the process.

I committed 8593c_h2/rev. 50: Allowing forced reclaiming only for NO-UNDO tables. Otherwise, on a rollback, we may have conflicting PK.
This means that COPY-TEMP-TABLE will never reclaim keys for UNDO table. This is accurate to the findings in 4GL.

I am also having a hard time tracking down an issue with the truncate now (not related to my changes in 8593c/8593c_h2). It seems like, at the moment of truncate, there are two records in the table. This first one has data, but the second one is just a reserved PK.

The root issue is that dataset fill may sometimes reach BufferImpl.FillWorker.fillRow with the following code:

if (!validated)
            {
               if (isMerge)
               {
                  // merge mode: ignore it, just skip to next record
                  TransactionManager.disableLoopProtection(null);
                  hasMore.assign(fillQuery._getNext());
                  // we need to release the newly created record as if undone. We cannot call
                  // BlockManager.undoNext(lbl) here as it will affect the buffer(s) from the query
                  // driving the fill operation.
                  recBuf.release(true, false); // I.e. drop() - this form avoids a buffer() lookup
                  return false;

Before reaching this, it always does selfBuf.create and it eventually releases the buffer due to recBuf.release. However, this is quite a special release as it doesn't flush, due to the first parameter true which stands for undo. Avoiding the flush, this DMO leaks its reserved PK.

Constantin: this is part of your effort to support clearPrimaryKey. Currently, it takes effect in ChangeSet.rollback and Session.delete, but this case is a bit more special. It may be registered so that ChangeSet.rollback will clear its PK, but it would be too late. If we hurry the process, we may end up clearing the same PK twice, which is again bad. I am not quite sure how to table the PK clear on release with undo.

I wonder if we can't be more lenient with the reserved PK and simply ignore them when truncating. On the other end, I identified this exact scenario (hopefully the last one) so I can work around it.

Alexandru, I don't like leaving 'allocated records' with no meaning in H2. Can we explicitly clear the primary key for this fill case, too?

These are the changes that unblocked me:

=== modified file 'src/com/goldencode/p2j/persist/RecordBuffer.java'
--- old/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-04-23 07:57:16 +0000
+++ new/src/com/goldencode/p2j/persist/RecordBuffer.java    2024-05-13 18:47:14 +0000
@@ -11862,6 +11862,19 @@
                                                                dbTrigManager);
             }

+            if (isTemporary && oldRecord.checkState(DmoState.NEW) && undo)
+            {
+               try
+               {
+                  // this is important to dealloc the reserved PK
+                  persistence.getSession().delete(oldRecord);
+               }
+               catch (PersistenceException e)
+               {
+                  LOG.log(Level.SEVERE, "Failed deleting the transient record on undo", e);
+               }
+            }
+            
             updateNoUndoState();

This is basically removing the DMO entirely from the session if isTemporary, NEW and undo. The delete from the session is also removing the reservation. The undo release is used only by this fill and BufferImpl.drop (in case exceptions occur). The changes are mostly in the safe side as they are hit in very specific cases.

But this requires testing.

Greg Shah wrote:

Ovidiu/Eric: Please review 8593c.

• TemporaryBuffer.java
  • nextPrimaryKey() is now public and @Override annotation dropped. It is never used to require public visibility and it still overrides the method in RecordBuffer;
  • getSequenceName() doesn't seem to be used any more. More than that Context.sequenceName is used to create the sequence and delete it, but no other uses, since FastCopyHelper relies in H2 now. I think these occurrences can be dropped as well.
• FastCopyHelper.java:
  • you marked FastCopyBundlePKElement as @Deprecated, but I think it can be dropped completely since it is not in use any more.

I have reviewed the changes from 8593c/rev. 15205. Good job!

Is there a reason for downgrading the percentage done to 80?

I've uploaded 1.48 to projsrv01 and upgraded the version in rev 15206.

I'll run ETF and test with another customer app tomorrow.

ETF testing passed, the original #7143-903 is fixed, and another app works OK with this.

You can merge to trunk now.

Greg Shah wrote:

You can merge to trunk now.

Starting.

Branch 8593c was merged to trunk revision 15212 and archived.

Branch 8593c_h2 was merged to fwd-h2 trunk revision 48 and archived.

This resolves the regression documented in #7143-900?

Greg Shah wrote:

This resolves the regression documented in #7143-900?

Yes

Committed 8593d/rev. 15206 to fix #8593-54. It contains the patch from #8593-56.
Recap: there was an issue on dataset fill where a transient record is undo-ed using a "special" release that doesn't flush. In this case, the DMO should be deleted to drop the reserved PK.

Is there any further review needed for this or do we just need testing?

8593d needs a review first.

Ovidiu: Please review.

I am picking this up for a customer unit tests.
As #8593 is broadly about performance in FWD-H2, Constantin, can you pick this up for another customer application that drove the effort in #8593 and ETF?

I am talking about 8593d.

Alexandru Lungu wrote:

Constantin, can you pick this up for another customer application that drove the effort in #8593 and ETF?

Testing is OK.

Actually I get this in the server log:

Failed deleting the transient record on undo com.goldencode.p2j.persist.PersistenceException: Attempting DML which cannot be committed!

Rebased 8593d/rev. 15263.

Tested this with a customer unit tests and everything went fine. Although, I think the testing is quite slim right now.
The project requires the latest H2 + heavy work with datasets to properly test - this blocks me on testing other suites which are more light-weight.

Thus, Constantin, can you do some smoke tests on another app with 8593d? This is the best way to confirm the changes won't regress right now.
I intend to have this merged today by EOD as it is the last missing piece of 7156c. After, I will rebase 7156c to start a conversion with it this weekend.

The change is not OK - I get this:

Caused by: java.lang.NullPointerException: row not found
        at org.h2.pagestore.db.MultiplexedScanIndex.clearPrimaryKey(MultiplexedScanIndex.java:415)
        at org.h2.embedded.FWDDirectAccessDriver.clearPrimaryKey(FWDDirectAccessDriver.java:132)
        at com.goldencode.p2j.persist.orm.DirectAccessHelper.clearPrimaryKey(DirectAccessHelper.java:171)
        at com.goldencode.p2j.persist.RecordBuffer.setCurrentRecord(RecordBuffer.java:11919)
        at com.goldencode.p2j.persist.RecordBuffer.release(RecordBuffer.java:9345)
        at com.goldencode.p2j.persist.RecordBuffer.release(RecordBuffer.java:9307)
        at com.goldencode.p2j.persist.BufferImpl$FillWorker.fillRow(BufferImpl.java:11987)
        at com.goldencode.p2j.persist.BufferImpl$FillWorker.lambda$processDataSource$1(BufferImpl.java:11791)
        at com.goldencode.p2j.persist.RecordBuffer.batch(RecordBuffer.java:3276)
        at com.goldencode.p2j.persist.BufferImpl$FillWorker.lambda$processDataSource$2(BufferImpl.java:11796)
        at com.goldencode.p2j.util.ErrorManager.silentWorker(ErrorManager.java:4134)

• during FILL will always register the record as 'batch dirty', as direct access will not go through the RecordBuffer handler
• a bulk delete will delete records from the buffers first (as a NEW record will have the PK in the database, and this needs to be removed before the temp-table truncate in H2).
• changed 'clearPrimaryKey' to return false instead of throwing a NPE - see also 8593d_h2 rev 49.

I'll test this with ETF.

8593d passed ETF testing.

Alexandru: please review.

Review of 8593d/rev. 15314:

• BufferImpl: The addDirtyBatchBuffer addition is a nice catch, but I would like to have some kind of guarantee this will not leak. I've seen other placed that do a startBatchAssignMode / endBatchAssignMode pair before/after fully filling the row.
• good change in RecordBuffer
• for TemporaryBuffer;
  • I don't get clearBuffers. The record of this buffer will be released before deleting, so it will be flushed to the database. The transient record is persisted and then deleted afterwards using SQL. How is the explicit call to RecordBuffer.delete helping the process here? It may be a good change; I just don't understand it. Do you have a case that is fixed by this?
  • you clear the buffers only if there is no where. What if the where is not-null, but still covering the transient record in the buffer?
• DirectAccessHelper changes require FWD_H2 changes. Are they in a FWD-H2 branch?

Alexandru Lungu wrote:

Review of 8593d/rev. 15314:

• BufferImpl: The addDirtyBatchBuffer addition is a nice catch, but I would like to have some kind of guarantee this will not leak. I've seen other placed that do a startBatchAssignMode / endBatchAssignMode pair before/after fully filling the row.

This is always bracketed in a batch:

            Runnable update = () -> RecordBuffer.batch(batchFill);

• for TemporaryBuffer;
  • I don't get clearBuffers. The record of this buffer will be released before deleting, so it will be flushed to the database. The transient record is persisted and then deleted afterwards using SQL. How is the explicit call to RecordBuffer.delete helping the process here? It may be a good change; I just don't understand it. Do you have a case that is fixed by this?

See this test:

def temp-table tt1 field f1 as int index ix1 is unique f1.
def dataset ds1 for tt1.

do transaction:
   create tt1.
   tt1.f1 = 1.
   release tt1.
   def buffer btt1 for tt1.
   create btt1.
   btt1.f1 = 2.

   dataset ds1:empty-dataset().
end.

• you clear the buffers only if there is no where. What if the where is not-null, but still covering the transient record in the buffer?

Then the H2 table truncate is not being called.

• DirectAccessHelper changes require FWD_H2 changes. Are they in a FWD-H2 branch?

See 8593d_h2 branch.

Got it!

For the example provided: I checked your test in 4GL and indeed, EMPTY TEMP-TABLE (similar to empty dataset) is flushing records from other buffers before deleting. So the change is reasonable.
However, this means that this was not working before truncate implementation (I think). Was this basically a latent bug that is now causing more problems with bulk delete?

H2 changes are OK.

Other testing you suggest? I am planning to take these to other application unit tests that I do with 7156c/6667g.

Alexandru Lungu wrote:

For the example provided: I checked your test in 4GL and indeed, EMPTY TEMP-TABLE (similar to empty dataset) is flushing records from other buffers before deleting. So the change is reasonable.
However, this means that this was not working before truncate implementation (I think). Was this basically a latent bug that is now causing more problems with bulk delete?

Yes, this is a latent bug, but I think it may apply for a bulk delete (via DELETE FROM) when a WHERE clause exists, too. We may want to move to a loop delete if there is a WHERE clause and records loaded in any buffer.

Other testing you suggest? I am planning to take these to other application unit tests that I do with 7156c/6667g.

ChUI regression testing.

ChUI regression testing.

I have a stable system with 6667i done just recently. I suppose I can move with 6667g so I have the FWD-H2 upgrade. I am making a baseline with 6667g now.

Please merge and upload the FWD-H2 changes so it can ease testing of 8593d. After, I will proceed with testing 6667g with unit tests of another customer.

Done, 8593d_h2 was merged to rev 49 (and uploaded to artifacts) and 8593d rev 15315 updates build.gradle to 1.49

• After 3 unit test modules that went smooth, the server entered (from the looks of it) an infinite loop that caused OutOfMemoryException
• After that, the OOME caused a bad exception unwinding that unbalanced the reserved primary keys. At that point, the H2 connection went rogue.
• All unit tests in that module failed due to closed connection exception.
• Moving on, some modules went ok, but some failed again. This time, not with OOME, but random rowCount expected 2 got 1. However, this seems to always be caused by INSERT INTO SELECT FROM statements.
• The server got stuck for several hours on a module. I think it met an infinite loop again (but one that wasn't leaking memory, so OOME was not thrown).

• I will attempt to find a small test-case by debugging the unit test.

I found another issue with this test:

def temp-table tt1 field f1 as int index ix1 is unique f1.
def dataset ds1 for tt1.

do transaction:
   create tt1.
   tt1.f1 = 1.
   release tt1.
   def buffer btt1 for tt1.
   create btt1.
   assign btt1.f1 = 1 no-error.

   dataset ds1:empty-dataset(). // this fails the truncate
end.

The ASSIGN ... NO-ERROR marks the record as invalid, but the empty-dataset see this record as invalid and does not proceed with 'clearPrimaryKey'. The fix is this in orm.Session.delete

Index: src/com/goldencode/p2j/persist/orm/Session.java
===================================================================
--- workspace/trunk/src/com/goldencode/p2j/persist/orm/Session.java
+++ workspace/trunk/src/com/goldencode/p2j/persist/orm/Session.java
@@ -900,7 +900,7 @@
       {  
          success = persister.delete(dmo);
       }
-      else if (dmo instanceof TempRecord && !dmo.checkState(INVALID))
+      else if (dmo instanceof TempRecord)
       {
          try
          {

But this may not be what you've seen in those unittests.

Constantin, the exception is triggered by a:
You may not delete BEFORE-TABLE <name> record. Use ACCEPT-ROW-CHANGES instead.
exception.

When calling TemporaryBuffer.removeRecord, one of the slave buffers is a before buffer. In this case, you can't delete it by demand. See TemporaryBuffer.delete(Supplier<logical> valexp, Supplier<character> valmsg):

      if (buf.isBeforeBuffer() && buf.hasBeforeBufferConstraints())
      {
         ErrorManager.recordOrThrowError(12374, buf.doGetName(), "");
         // You may not delete BEFORE-TABLE <name> record.  Use ACCEPT-ROW-CHANGES instead.
         return false;
      }

Do you have a FWD stacktrace when this is happening?

doCloseMultiplexScope:7689, TemporaryBuffer (com.goldencode.p2j.persist)
closeMultiplexScope:6848, TemporaryBuffer (com.goldencode.p2j.persist)
resourceDeleted:6494, TemporaryBuffer (com.goldencode.p2j.persist)
resourceDeleteImpl:9914, BufferImpl (com.goldencode.p2j.persist)
lambda$resourceDelete$7:9871, BufferImpl (com.goldencode.p2j.persist)
exec:-1, BufferImpl$$Lambda$939/0x00007f1d3ccb2288 (com.goldencode.p2j.persist)
timer:132, NanoTimer (com.goldencode.p2j.jmx)
resourceDelete:9871, BufferImpl (com.goldencode.p2j.persist)
delete:475, HandleChain (com.goldencode.p2j.util)
delete:5225, BufferImpl (com.goldencode.p2j.persist)
deleteResource:3495, ProcedureManager$ProcedureHelper (com.goldencode.p2j.util)
lambda$deleteAllBuffers$2:2726, AbstractTempTable (com.goldencode.p2j.persist)
accept:-1, AbstractTempTable$$Lambda$1381/0x00007f1d3d0fbfb0 (com.goldencode.p2j.persist)
deleteAllBuffers:2740, AbstractTempTable (com.goldencode.p2j.persist)
forceClear:3980, TempTableBuilder (com.goldencode.p2j.persist)
forceDelete:3227, TempTableBuilder (com.goldencode.p2j.persist)
resourceDelete:3216, TempTableBuilder (com.goldencode.p2j.persist)
delete:475, HandleChain (com.goldencode.p2j.util)
forceDelete:3232, TempTableBuilder (com.goldencode.p2j.persist)
resourceDelete:3216, TempTableBuilder (com.goldencode.p2j.persist)
delete:475, HandleChain (com.goldencode.p2j.util)
clearImpl:1311, DataSet (com.goldencode.p2j.persist)
resourceDelete:5653, DataSet (com.goldencode.p2j.persist)
delete:475, HandleChain (com.goldencode.p2j.util)
delete:491, HandleOps (com.goldencode.p2j.util)
deleteResources:1360, WidgetPool$WidgetPoolData (com.goldencode.p2j.util)
deletePool:796, WidgetPool (com.goldencode.p2j.util)
delete:361, WidgetPool (com.goldencode.p2j.util)

In that point, TemporaryBuffer.removeRecords is called. Where clause is null, so clearBuffers is called. A slave buffer is found and deleted. The code in #8593-94 is reached.

TBD: also done ChUI regression testing and there are 14 regressions, while only 1 was expected. Can't tell the reason yet.

Also, to clarify things, due to this error regarding before buffers, DirectAccessHelper.killMultiplex is called to clear the multiplexed index (in a finally block). Thus, the table has rowCount 1 (because the delete failed), but the multiplexed scan index is empty, as the multiplex was killed anyway. The fatal error occurs later when the table is used again, noticing the invariant break.

Thanks, the recreate is this:

def temp-table tt1 before-table brt1 field f1 as int index ix1 is unique f1.
def dataset ds1 for tt1.

def var hds as handle.
def var htt as handle.
def var hbb as handle.

create dataset hds.
hds:create-like(dataset ds1:handle).
hbb = hds::tt1.
htt = hbb:table-handle.

do transaction:
   hbb:buffer-create().
   hbb::f1 = 1.
   htt:Tracking-changes = true.
   hbb:buffer-delete().

   delete object hds. 

   undo, leave.
end.

The fix for this and the one mentioned in #8593-93 are in 8593d rev 15316.

Review of 8593d/rev. 15316.

• I am OK with the changes in Session
• For TemporaryBuffer: isn't the flag you are changing a guard to avoid spoiling the after-before table relationships? I was thinking of something similar, but I was thinking that this may have an impact the peer relationship. What if you delete the before buffer (quite forcefully now) and the after buffer is still referencing it at that time? I think the after buffer will have a dangling reference to a deleted record.

Alexandru Lungu wrote:

• For TemporaryBuffer: isn't the flag you are changing a guard to avoid spoiling the after-before table relationships?

Yes, but that is used only in cases when we are no trying to empty the entire temp-table (which includes deleting the both after and before tables).

What if you delete the before buffer (quite forcefully now) and the after buffer is still referencing it at that time? I think the after buffer will have a dangling reference to a deleted record.

The 'after-table' buffers have their records deleted first; if there are referenced records in these after buffers, then any equivalent before-table records will get deleted.

After that, you go through the 'before-table' buffers: in this case, you can have 'ROW-DELETED' records loaded as NEW in the before-buffer, and these don't have any equivalent in the after-table.

The 'after-table' buffers have their records deleted first; if there are referenced records in these after buffers, then any equivalent before-table records will get deleted.

This makes sense only in "bulk delete" kind of scenarios. So I agree with you if the following point is clarified.

Yes, but that is used only in cases when we are no trying to empty the entire temp-table (which includes deleting the both after and before tables).

Is this always true? Can't the 4GL code simply clear the before table like EMPTY TEMP-TABLE btt? Or will this also clear the after table implicitly? Or is this forbidden?

Alexandru Lungu wrote:

Is this always true? Can't the 4GL code simply clear the before table like EMPTY TEMP-TABLE btt? Or will this also clear the after table implicitly? Or is this forbidden?

4GL does not allow EMPTY-TEMP-TABLE on a before-buffer.

Constantin Asofiei wrote:

Alexandru Lungu wrote:

Is this always true? Can't the 4GL code simply clear the before table like EMPTY TEMP-TABLE btt? Or will this also clear the after table implicitly? Or is this forbidden?

4GL does not allow EMPTY-TEMP-TABLE on a before-buffer.

Got it! Picking up the changes and restart testing.

• some modules have 1-5 unit tests failing (these may be false negatives, but I will retest)
• one module is regressed hard (<10% success rate) - this is what I am investigating now
• one module is stuck in an infinite loop

I don't have the very exact scenario this is causing this, but I am trying to find one. To post some insights:

• The first buffer contains a record: NOUNDO DELETING DELETED with recid 2560 and some data and a datasource-rowid
• The second buffer contains the cached record: NOUNDO CACHED with recid 2560 and some different data and no datasource-rowid

The most suspect fact is that the first buffer is not CACHED, so it shouldn't be searched in the cache anyway.

By absolute best guess here is that the code in TemporaryByuffer.clearBuffers should skip the buffers that have a DELETED record anyway. Otherwise, we attempt double-delete that may (incorrectly) hit the session cache sanity check.
My lack of understanding: why is a deleted record still referenced by a buffer? is this a valid state anyway? I see that RecordBuffer.stateChanged is releasing the buffer if its current record was deleted.

Progress:

The culprit buffer has its record deleted by an emptyDataset - clearBuffers. However, the buffer is not registered with the ChangeBroker (why? this is still to be investigated). Therefore, the record remains in the buffer (not CACHED, but DELETED).
As H2 is reclaiming ids, that id will be given to another record.

When attempting to do a second emptyDataset - clearBuffers on the same table, the deleted record (although not cached) will find an "alter ego" (i.e. with the same id) inside the session cache and fail.

The obvious fix still remains: skip delete record buffers that contain a deleted record, but I am afraid this will patch a logic problem we already have in place.

Alexandru Lungu wrote:

However, the buffer is not registered with the ChangeBroker (why? this is still to be investigated). Therefore, the record remains in the buffer (not CACHED, but DELETED).

Hm... I wonder if this is not the reason for the A different DMO instance of type is already bound errors we see in some other unit tests.

Alexandru Lungu wrote:

Progress:

The culprit buffer has its record deleted by an emptyDataset - clearBuffers. However, the buffer is not registered with the ChangeBroker (why? this is still to be investigated). Therefore, the record remains in the buffer (not CACHED, but DELETED).

A buffer (let's say the culprit buffer is Buffer A) should never contain a deleted record.

• Scenario 1: if the record is deleted from Buffer A, it should be removed as part of the delete operation. The ChangeBroker should notify all other registered buffers, so they can remove the deleted record, if they currently hold it.
• Scenario 2: (the flip-side of Scenario 1) if the record is deleted from another buffer (Buffer B) while it is simultaneously contained in Buffer A, the ChangeBroker should notify Buffer A of the delete, and Buffer A's response to the delete event should be to remove the deleted record. If this is the case, it is key to understand why Buffer A is not registered with the ChangeBroker. Was it never registered, or was the registration lost too early? The listener algorithm in ChangeBroker is pretty complex, as I recall.
• Scenario 3: if the record is not contained in Buffer A at the time it is deleted from some other buffer, it should never be allowed to be loaded later into Buffer A thereafter. The DELETED flag should prevent any use of the record, including loading it into a buffer or caching it.

Does emptyDataset - clearBuffers fall under any of these scenarios, or is there another scenario I have missed?

In any case, a DMO should be removed from the session cache and marked with the DELETED flag at the time it is deleted. It seems the latter is happening in this case. Is it also removed from the session cache?

As H2 is reclaiming ids, that id will be given to another record.

When attempting to do a second emptyDataset - clearBuffers on the same table, the deleted record (although not cached) will find an "alter ego" (i.e. with the same id) inside the session cache and fail.

The obvious fix still remains: skip delete record buffers that contain a deleted record, but I am afraid this will patch a logic problem we already have in place.

Yes, this is not really a fix because, as noted above, if things are working as expected, there should not be any buffers which contain a deleted record.

Does emptyDataset - clearBuffers fall under any of these scenarios, or is there another scenario I have missed?

The emptyDataset - clearBuffers is applying delete to all buffers for a specific table. So the normal flow from a buffer.delete operation happens (including ChangeBroker work). From the looks of it, I incline to think it is about Scenario 1: the buffer that triggered delete is not registered in ChangeBroker, so it is not "auto-released". However, the buffer is not deleted from converted code, but by clearBuffer.

In any case, a DMO should be removed from the session cache and marked with the DELETED flag at the time it is deleted. It seems the latter is happening in this case. Is it also removed from the session cache?

It is removed from session cached and it is not flagged as CACHED. The record state is right; the buffer state is not right (still referencing a deleted record).

Yes, this is not really a fix because, as noted above, if things are working as expected, there should not be any buffers which contain a deleted record.

What if the buffer is in a more special state? Something like a deferred delete, unregistered from ChangeBroker, but still unreleased.
It looks more like a buffer which is no longer available to the converted code / was out-of-scope / deleted (so removed from ChangeBroker), but still inside getSlaveBuffers with a record loaded. The problem here would be that the buffer wasn't released at the right time.

I am still investigating.

Found the issue:

define temp-table tt no-undo field f1 as int. // no-undo to force reclaiming

procedure first_step:
    define buffer btt for tt. // leaking buffer
    message "inner".
    finally:
        find first btt no-error. // the buffer is loaded; BUT ITS SCOPE WAS CLOSED !!
        if avail(btt) then do:
            message "avail".
        end.
    end.
end. //  at this point, btt is still loaded, but its scope is closed

do transaction:
    create tt.
    tt.f1 = 1.
    message recid(tt). // 2304
end.
message "record created".

def var i as int.
// the buffers may be in a non-deterministic order in slave set
// do this scenario several times to force a empty temp-table
// to delete a non-cached deleted record first
repeat i = 1 to 10:
    run first_step.
end.

do transaction:
    empty temp-table tt. // this will mark the loaded record in btt as deleted
    message "done empty".
end.

do transaction:
    create tt. // reclaim
    tt.f1 = 1.
    message recid(tt).
end.

do transaction:
    empty temp-table tt. // error; btt is still a slave buffer with a deleted record loaded
    message "done empty".
end.

Converted as:

   @LegacySignature(type = Type.MAIN, name = "start.p")
   public void execute()
   {
      externalProcedure(Start.this, new Block((Body) () -> 
      {
         RecordBuffer.openScope(tt);

         doBlock(TransactionType.FULL, "blockLabel0", new Block((Body) () -> 
         {
            tt.create();
            tt.setF1(new integer(1));
            message(tt.recordID());
         }));

         message("record created");

         ToClause toClause0 = new ToClause(i, 1, 10);

         repeatTo("loopLabel0", toClause0, new Block((Body) () -> 
         {
            RUN_CALL_SITE_1.clone().run();
         }));

         doBlock(TransactionType.FULL, "blockLabel1", new Block((Body) () -> 
         {
            tt.deleteAll();            //  this will mark the loaded record in btt as deleted
            message("done empty");
         }));

         doBlock(TransactionType.FULL, "blockLabel2", new Block((Body) () -> 
         {
            tt.create();            //  reclaim
            tt.setF1(new integer(1));
            message(tt.recordID());
         }));

         doBlock(TransactionType.FULL, "blockLabel3", new Block((Body) () -> 
         {
            tt.deleteAll();            // error
            message("done empty");
         }));
      }));
   }

   @LegacySignature(type = Type.PROCEDURE, name = "first_step")
   public void firstStep()
   {
      Tt_1_1.Buf btt = TemporaryBuffer.define(tt, "btt", "btt");

      internalProcedure(Start.this, "first_step", new Block((Body) () -> 
      {
         RecordBuffer.openScope(btt);
         message("inner");
      }, 
      (Fini) () -> 
      {
         silent(() -> new FindQuery(btt, (String) null, null, "btt.recid asc").first());

         if (btt._available())
         {
            message("avail");
         }
      }));
   }

I think this is related to finally block, as the scope of the buffer is not managed properly. If the finally block would have created/updated the buffer, everything would have been OK. But this is just a read-only operation, so it leaks. I added Danut as a watcher because he has faced a similar issue with buffer scoping and finally blocks. From my understanding, this is a problem in trunk that is now more visible with 8593b. Theoretically, that buffer is leaking until the table scope is closed. As nobody was touching it by now, everything worked correctly. But now, with clearBuffers we touch the leaking buffer and cause a crash.

Constantin, please assess.

Alexandru Lungu wrote:

I think this is related to finally block, as the scope of the buffer is not managed properly. If the finally block would have created/updated the buffer, everything would have been OK. But this is just a read-only operation, so it leaks. I added Danut as a watcher because he has faced a similar issue with buffer scoping and finally blocks. From my understanding, this is a problem in trunk that is now more visible with 8593b. Theoretically, that buffer is leaking until the table scope is closed. As nobody was touching it by now, everything worked correctly. But now, with clearBuffers we touch the leaking buffer and cause a crash.

I had the same issue, the problem was a unit tests that would fail an assertion before is able to activate a buffer and the FINALLY block would require that buffer to delete the records, failing as it was inactive. As far as I remember, I investigated the failing assertion as it was the cause and found it to be fixed by 6667g so I did not give any attention to the inactive buffer problem after that. I also tried to replicate it in a smaller test case in the beginning, but did not have any luck with that.

The fix is not obvious. The major issue is that the scope of the buffer is closed so that it release, commits, etc. and is not aware that it is going to be used by the finally block. So any resource that will reach the finally block will have its finished method called (and its scope opened), but it may be still reused. I tried obvious fail-safes, but none were satisfying. The most obvious was to reinitialize the buffer if first used in the finally block, but the lifecycle of the buffer was not that tolerant with that. I need a more robust way to handle this.

My plan is to introduce a new Finalizable.finished(boolean beforeFinally). All Finalizable.finished from FWD should be better understood in regard to the finally block. I can incrementally have this handled starting with RecordBuffer and TemporaryBuffer.

Any suggestions? Please let me know if you have done work with finally if you can advice on this matter. My idea implies quite a large architecture refactoring, reanalyzing the usages of Finalizable.finished.

As a smaller example:

define temp-table tt field f1 as int.  

do transaction:
    create tt.
    tt.f1 = 1.
end.

for each tt:
    define buffer btt2 for tt.
    find first btt2 no-error.
    btt2.f1 = 10.

    finally:
        find first btt2. // without this line, 4GL throws and exception on next line
        btt2.f1 = 20.
        message "abc".
    end.
end.

My idea here for buffers is that the "before finally finalizable" is just releasing the record (triggering commit eventually), but nothing more! The finalizable logic is going to happen after finally.
For other resources, I can let it like now: ignore beforeFinally.

Alex, the TemporaryBuffer$Context.domains never gets cleaned, also - the salve buffers for a DMO do not get removed when that explicit buffer gets out of scope.

The buffer's scope is OK in FWD, but I think, as you mention, we close it too early - this should be done after any CATCH or FINALLY executes.

Constantin Asofiei wrote:

Alexa, the TemporaryBuffer$Context.domains never gets cleaned, also - the salve buffers for a DMO do not get removed when that explicit buffer gets out of scope.

It is cleared when the table is destroyed.
I agree that the slave buffers are not removed from that collection eagerly. We can do that, but it felt like "patching" the actual issue that the buffer is leaking to the finally block in an invalid state.

The buffer's scope is OK in FWD, but I think, as you mention, we close it too early - this should be done after any CATCH or FINALLY executes.

The buffer' scope was closed before CATCH and FINALLY to trigger validation and flush. This is the right thing to do in terms of functionality, but the scope should stay open.

I can go ahead with a bit more reactive TemporaryBuffer$Context.domains, but I fear that this is just patching the buffer leaking issue.
Do you prefer to have it fixed this way and move ahead (deferring the finally problem), or handle this now? My personal preference is to have 8593d fixed and merged asap (so go with a straight-forward fix), but at the same time I want to hurry the problem with finally (don't let it starve).

The investigation to find this took a couple of days - mainly because everything looks right internally, it was just that out-of-nowhere I spotted that a buffer from TemporaryBuffer$Context.domains is loaded although its open scope is 0 - where?, why?, etc were harder to answer. If we are going to patch TemporaryBuffer$Context.domains, anybody else facing the finally issue will have a way harder experience to track it down again.

I'm not saying that we don't need to fix the finalizable problem. The non-default buffers need to be cleaned from domains when they are destroyed, otherwise this will be a leak which will accumulate over the lifetime of some persistent procedure.

I think the finally should be a sub-block of the block it guards, and execute as part of that, not sequentially, after its 'master' block ends. In fact this is the semantic which the syntax enforces. In this case, BlockManager.processForBody() should work correctly (very simplified code):

try {
   block.body();
} finally {
   doTerminationBlockWorker(new Block((Body) block::fini))
}

Alexandru Lungu wrote:

Committed 8593d/rev. 15317 including changes to remove the out-of-scope buffer from the slave collection. This is enough to fix another bulk of regressions over unit tests with 8593d. The testing is still ongoing on my side, but I got past the problem from #8593-111. Please review.

I'm OK with the changes.

I am still facing an infinite loop, but this is due to #8945 which seems unrelated.
The issue from #8593-120 is to be discussed separately in #8948.

• re-run all unit tests with 8593d/rev. 15317 - beside the known unrelated problems, the changes are the same as the baseline.

Constantin, anything left to be tested?

Rebased 8593d from trunk rev 15330 - new rev 15336. I need to run ETF and some other app testing.

Constantin Asofiei wrote:

Rebased 8593d from trunk rev 15330 - new rev 15336. I need to run ETF and some other app testing.

ETF testing is OK and also other scenarios I was looking at.

Is this ready/safe to merge?

Greg Shah wrote:

Is this ready/safe to merge?

Yes.

Branch 8593d was merged to trunk rev 15331 and archived.

I found the reason of (some of) the A different DMO instance of type ... is already bound at least in a scenario where ACCEPT-CHANGES is involved. In this case, afterBuf.pruneSessionCache(); is being called, which clears the entire cache for this temp-table, without any consideration if there are records (for this temp-table) still loaded in buffers or not. This code in BufferImpl.acceptChanges:

     Object[] args = new Object[] {afterBuf.getMultiplexID()};
      try
      {
         boolean noUndo = !beforeBuf.isUndoable();
         int cnt = persistence.executeSQL(buf.toString(), args, noUndo);
         afterBuf.pruneSessionCache();
         if (LOG.isLoggable(Level.FINE))
         {
            LOG.log(Level.FINE, "ACCEPT-CHANGES accepted " + cnt + " changes from " + beforeBuf.getDMOName());
         }
      }
      catch (PersistenceException e)
      {
         return logical.of(false);
      }

update tt473 set _rowState=0, _originRowid=NULL, _peerRowid=NULL where _multiplex = ?

This is not OK, records are left loaded in buffers, and another query (via some other buffer) will re-load it in the ORM session cache. Next, if any operation is being done on the buffer with the original record (which was evicted from the cache but still in the buffer), FWD will raise A different DMO instance of type ... is already bound, as the cache is no longer in sync with the records loaded into the buffers.

This may be the root cause for the intermittent DMO association errors we see in other scenarios.

The solution I see is to iterate all cached records for this temp-table (DMO and multiplex) and set the _rowState=0, _originRowid=NULL, _peerRowid=NULL for each record, and after that still execute the SQL on the temp-table; this way, pruneSessionCache is avoided and the records are in sync with the database.

Alexandru, about the other issue related to leaking slave buffers. The issue is that a buffer can be defined, but without its scope opened. This code in finishedImpl needs to call cleanup instead of onCloseOutermostScope - this change solves my issue.

   protected void finishedImpl()
   {
      if (openScopeCount == 0)
      {
         if (isActive())
         {
            // Notify connection manager that this buffer is done with the connection.
            cleanup();
         }

         return;
      }

I also have the changes for #8593-129, I'll put the in a branch tomorrow, these I think are worth testing with 7156c/6667g.

• ACCEPT-CHANGES must not prune the session cache, but update the before-fields for the cached records directly.
• Fixed a leak of slave temp buffers: cleanup must be done even if their scope has never been opened.

Alexandru, please review.

Hmm, I think this is an edge case for the case when the buffer is used with CAN-FIND. Its scope is not opened, but yet it is loaded. However, I don't think a "full" cleanup should be done. This may incorrectly release and validate, flush or fire trigger for the current record. See #8871-7 for this example being converted.

if not can-find(first pt where f1 = 2) then do:
    for first pt where f1 = 3 no-lock:
        message "abc".
    end.
end.

If the if-block is not entered, the pt remains loaded, but without its scope opened. If you will apply the change in #8593-130, I suppose the loaded record in pt will suffer aggressive flushing. Can you have a more specific test-case for #8593-130? I simply presume that is related to CAN-FIND, but I may be wrong.

I incline to think the change make sense and we should continue with it, but I am not 100% sure that cleanup doesn't have unwanted side-effects.

PS: I was writing this before seeing #8593-131 (assessing now).

Review of 8493e:

• I think the solution for ACCEPT-CHANGES is elegant, resolving back to a Consumer and updating the cached DMOs on demand. However, I think it is the first time place where we start updating records that were found in the cache (and not through a buffer). Can we find records that don't have an active buffer in the cache - if so, when will these records be flushed? Also, is there a chance to update records with unwanted state (INVALID / DELETED), etc.? I reckon I am afraid of having this new path of updating records, bypassing the buffer layer. I am not against it, but I am looking forward for some extensive testing in this area.
• For "leak of slave temp buffers", see my comment in #8493-132.

Alexandru Lungu wrote:

Also, is there a chance to update records with unwanted state (INVALID / DELETED), etc.?

Hm... you are right, the record can't become 'dirty'; I think direct access to these fields is required, so the record's state doesn't change. Also, skip any invalid/deleted/etc records.

• For "leak of slave temp buffers", see my comment in #8493-132.

Will do, thanks.

Hm... you are right, the record can't become 'dirty'; I think direct access to these fields is required, so the record's state doesn't change. Also, skip any invalid/deleted/etc records.

Quick idea: maybe we can simply make the DMOs invalid. I think this will force the buffers to reload the DMO from the database. In general, I think pruning the session cache should mark the DMOs as stale (INVALID), so in the end FWD ORM will do its magic.

Alexandru Lungu wrote:

Hm... you are right, the record can't become 'dirty'; I think direct access to these fields is required, so the record's state doesn't change. Also, skip any invalid/deleted/etc records.

Quick idea: maybe we can simply make the DMOs invalid. I think this will force the buffers to reload the DMO from the database.

Is not that simple: a DMO can be in any state, even DIRTY.

In general, I think pruning the session cache should mark the DMOs as stale (INVALID), so in the end FWD ORM will do its magic.

pruneSessionCache is meant to be called in conjunction with a removeRecords (i.e. the temp-table is always emptied), so there are no more records in this temp-table, and the cache can be cleared safely.

I also looked over 8593e/15351.

I don't see anything wrong with the new code.

I remember we had previously another synchronization issue between the SQL state and the cached state. In that case the records had to be deleted so this new solution was not easily applicable. I like the approach because it opens a door for almost any processing on cached records. But, as Alexandru said, we need to be careful with its usage, as it might become a Trojan horse.

Ovidiu, please see 8593e 15354 - this fixes better ACCEPT-CHANGES.

Alexandru, I've backed out the 'cleanup' change in RecordBuffer.finishedImpl; I don't have a recreate yet, but I'll still look for it. In the mean time, can you test this in the context of the #7156 app? I understand there were some regressions there which would be fixed by 8593e.

Review of 8593e/15379 (I assume this is your 15354 after the rebase).

I think the code is OK. I like the direct access optimisations for temp-table specific reserved properties.

Alexandru, I've backed out the 'cleanup' change in RecordBuffer.finishedImpl; I don't have a recreate yet, but I'll still look for it. In the mean time, can you test this in the context of the #7156 app? I understand there were some regressions there which would be fixed by 8593e.

This was my exact plan for today :)

This passed ETF and also solves #9048. We need to merge it, even if it does not address issues in 7156c.

I agree, I reckon 7156c fails because a StopConditionException. The SQL is wrong as a column is not found.
So, I will need to look for a mean to make FWD-H2 safer.

If this is ready to merge, it can go after 8233a. Constantin/Alexandru make the call.

Greg Shah wrote:

If this is ready to merge, it can go after 8233a. Constantin/Alexandru make the call.

Queued for merge.

Branch 8593e was merged to trunk rev 15379 and archived.