FWD » Core Development » Database

This is related also to the #10833-80 scenario, where it was proved that nonEmptyTableFlags is broken in case of rollback.

• prog0.p
  

  def var h1 as handle.
  
  run prog1.p persistent set h1.
  dynamic-function("func0" in h1).
  
  do transaction:
     delete object h1.
     undo, leave.
  end.
  

• prog1.p
  

  def temp-table tt2 field f1 as int.
  
  function func0 returns int.
     def var i as int.
  
     do i = 1 to 10:
        create tt2.
        tt2.f1 = i.
        release tt2.
     end.
  end.
  

What matters is that the population of the temp-table is done outside of the transaction, and the rollback inside the transaction acts on a deleted table.

So this scenario where a rollback is done on a deleted table, must not leave records in it...

When the multiplex is closed, the DELETE stmt which clears the records with the specific multiplex ID must not be recorded for undo, as otherwise a rollback will 'revive' them, although the 4gl temp-table has been deleted.

So the title maybe should not be fwd-h2 'killedMultiplexesCleanup' must not abend if the batch is not empty but instead deleting a temp-table inside a transaction must not record undo logs or something like this.

Something else to consider: when a temp-table gets deleted, the reference for any object in some record field gets decremented; thus destructors may executed while the temp-table gets deleted. So, does an UNDO revert any created records by such destructors?

If we set a savepoint in doCloseMultiplexScope() , and after that discard it completely (not commit!), this would affect any actions done in destructors, as those actions will be lost for rollback.

I don't see another way than add a NO-UNDO option for the DELETE statement in H2, so that we can do DELETEs which are not recorded for undoable purposes.

I don't see another way than add a NO-UNDO option for the DELETE statement in H2, so that we can do DELETEs which are not recorded for undoable purposes.

This makes sense. The delete object should be aggressive enough to clear the logs so any eventual rollback won't revive records or mark the table as forcefully dropped and skip any undo log related to it. Dropping of table is transactional in H2 for FWD. By default, DROP is non-transactional so this is a bug we technically introduced. Note to self: there might be some complications with extent tables that are still in use for some customers.

• in-memory DMOs rollback
  • this should be skipped as well ... we should not revive DMOs (or reset their DELETED state).
• other state rollback (RecordNursery, dirty-share, etc.)
  • fortunately, dirty work is not capable of proper rollback, so I don't think it is a problem - but it should be tested.

So, does an UNDO revert any created records by such destructors?

Huh, interesting lead.

Alexandru Lungu wrote:

So, does an UNDO revert any created records by such destructors?

Huh, interesting lead.

• prog0.p:
  

  def var h as handle.
  run prog1.p persistent set h.
  
  dynamic-function("func0" in h).
  
  find first book.
  message book.isbn.
  do transaction:
     delete object h.
     undo, leave.
  end.
  
  find first book.
  message book.isbn.
  

• prog1.p:
  

  def temp-table tt1 field f1 as progress.lang.object.
  
  function func0 returns int.
  create tt1.
  tt1.f1 = new oo.foo().
  release tt1.
  
  end.
  

• foo.p:
  

  // block-level on error undo, throw.
  
  class oo.Foo:
  
    destructor public foo().
       for each book: delete book. end.
       create book.
       book.isbn = "abc" + string(mtime).
       message book.isbn.
       release book.
    end.
  end.
  

Looks like there is a difference when the destructor is called from a delete object (i.e. temp-table destroyed) and a delete tt1.. In the delete record case, the destructor changes are rolled back. In the temp-table destroy case, the destructor changes are not rolled back.

So I think more experimentation is needed.

In the temp-table destroy case, the destructor changes are not rolled back.

So you can create things in the persistent database that are not sensitive to rollback? Please try to create a book into the transaction from prog0.p - maybe OE thinks that the transaction block is useless considering that there are no proper DB operations and eventually makes the destructor a full transaction (?)

Or maybe the destructor is deferred till the end of the transaction and we don't see that. Maybe a message after delete would help, to see if the destructor happens before/after that message.

• for NO-UNDO tables, this bitmap will work as is - rollback has no effect on these, so nonEmptyTableFlags will be consistent
• for undoable tables, then rely on fastHasRecords

Otherwise, if it proves too difficult to solve/investigate the destructor conundrums, lets work on enhancing the UPDATE SQL statement syntax in H2 to add a NO-UNDO (Alex: any better idea on the name?) option - provide the proposed syntax and after we discuss it please implement it.

The point is that removeRecords when called from doCloseMultiplexScope, it will append this option to the DELETE statement. I don't recall if removeRecords will end up using FQL for the DELETE (when closing the multiplex scope) - in such case, we will need to update the fql.g grammar, too.

Otherwise, if it proves too difficult to solve/investigate the destructor conundrums, lets work on enhancing the UPDATE SQL statement syntax in H2 to add a NO-UNDO (Alex: any better idea on the name?) option - provide the proposed syntax and after we discuss it please implement it.

I am ok with it. Currently we have CREATE TABLE ... NO-UNDO to symbolize that all actions are no-undo on that table. But if we need to be granular, I think it is fine to do so.
Note to self: ensure reclaiming of id is still stable with this.

The point is that removeRecords when called from doCloseMultiplexScope, it will append this option to the DELETE statement. I don't recall if removeRecords will end up using FQL for the DELETE (when closing the multiplex scope) - in such case, we will need to update the fql.g grammar, too.

Mind that this does TRUNCATE AFAIK.

Alexandru Lungu wrote:

Mind that this does TRUNCATE AFAIK.

We can TRUNCATE only if there are separate SQL tables, and we get rid of the MULTIPLEX. Otherwise, we need DELETE ... NO-UNDO for the 'pseudo-truncate' to discard this multiplex.

We can TRUNCATE only if there are separate SQL tables, and we get rid of the MULTIPLEX. Otherwise, we need DELETE ... NO-UNDO for the 'pseudo-truncate' to discard this multiplex.

Yes, but I mean the PageStoreTable.truncate. But in order to communicate from FWD to H2 to actually do a no-undo operation, I agree we need a keyword - we can't infer from SQL.

Constantin,

I did additional tests and, all I could find was an NPE (which I have fixed and will be available in next commit). I tried your scenario (with both static and dynamic temp-tables) and they worked just fine. The explanation is that since killMultiplex() method is always called in the finally block after removeRecords(..., true /*noundo*/);, the effect is the same as you pointed out.

However, calling session.cleanUndoLog(t, multiplex); in killMultiplex() seems like a better solution. It would avoid tracking the additional noundo parameter over a couple of objects in H2. The downside is that, on the removeRecords() call, the UndoLogRecord will still be created and then marked as NOPs. It does not look like a big effort, but it's something we want to avoid, knowing that those records will actually only require cleanup effort.

• fwd-h2/10937a: r67
• fwd/10937a: r16319.

Ready for review / merge to 9457c.

Alex - please review fwd-h2 changes.

I am OK with the changes in fwd-h2/10937a. They are clean: on killMultiplex, replace any undo-entry regarding that table and multiplex with NOP. Cool!

for NO-UNDO tables, this bitmap will work as is - rollback has no effect on these, so nonEmptyTableFlags will be consistent

I just re-read this and I wonder if there is also a case in which we can't determine nonEmptyTableFlags: deleteRecords with WHERE clause. In this case, we don't know if the partial delete made the table empty or not, even if it is NO-UNDO. I guess we need to refresh nonEmptyTableFlags with fastHasRecords after partial delete. I need to think of other places where nonEmptyTableFlags is made inconsistent (copy-temp-table with replace/append? copy-temp-table from empty table? read empty xml into table, intake empty REST request, some things related to date-set relations? before-table related behaviors? etc.).

IMHO, keeping a consistent nonEmptyTableFlags is risky and may not be that relevant, considering that fastHasRecords is indeed fast (search in the b-tree a records with the provided multiplex). Otheriwse, I think we can change the implementation to actually look into MultiplexedScanIndex.lastBatches (map look-up). This will bypass the whole infrastructure to manage nonEmptyTableFlags and risk not keeping it in sync.

Ovidiu, at this time I don't think I'll add 10937a to 9457c - 9457c already has a change which always uses this:

   protected boolean isTableDefinitelyEmpty()
   {
      Boolean has = fastHasRecords();
      return has != null && !has.booleanValue();
   }

We need to delay 10937a until we figure out if nonEmptyTableFlags can be kept as is or not.

The main fix is in 10937a_h2 - I'll create a 10937b with the fwd-h2 revision update and merge 10937a_h2 to fwd-h2 trunk.

Alexandru - there is another change I want to add to 10937a_h2. In UndoLog.cleanTable, I think we can be no-op if the table is no-undo, right?

Constantin Asofiei wrote:

Alexandru - there is another change I want to add to 10937a_h2. In UndoLog.cleanTable, I think we can be no-op if the table is no-undo, right?

This is correct. If the table is no-undo, the undo-log is not populated.

Alexandru Lungu wrote:

Constantin Asofiei wrote:

Alexandru - there is another change I want to add to 10937a_h2. In UndoLog.cleanTable, I think we can be no-op if the table is no-undo, right?

This is correct. If the table is no-undo, the undo-log is not populated.

Added to 10937a_h2 rev 69

Constantin,
although I am not against it, I do not think the new code in cleanTable() will be executed. As Alex mentioned, for NOUNDO tables, the undoLog is not initialized (it remains null because no logs are stored) and the only place where the method is invoked is cleanUndoLog(), which makes sure the undoLog was created.

I think this is my fault. With my last commit, I forgot to update the javadoc and error messages in thrown exceptions which may lead to confusion. I will update those to reflect the actual situation.

Session.cleanUndoLog usage of undoLog != null is database-wide no-undo for all temp-tables, Table.isNoUndo() is something else (per-table flag). So is needed, please leave my change intact.

10937a_h2 was merged to fwd-h2 trunk rev 64 and archived as merged.

10937b was created from trunk rev 16321 - 16322 upgrades the rev to fwd-h2 64.

Anything left to review?

Branch 10937b was merged to trunk rev 16396 and archived.

What remains is to decide how to improve the nonEmptyFlags and isTableDefinitelyEmpty - Ovidiu, please work on this.

• method TemporaryBuffer.rollback(boolean transaction). Clearly, the local.nonEmptyTableFlags set should be updated. But there are two kind of changes on undo:
  • if the original operation was a CREATE, the respective table was removed from this set. Do we add it now? We do not have the information it was there before to add it back.
  • if the original operation was a DELETE, the respective table might be deleted now. Do we remove it now?

• method TemporaryBuffer.isTableDefinitelyEmpty(). In trunk, relies solely on local.nonEmptyTableFlags to decide the result. As noted above, this is not reliable. Even with the most pessimistic approach, we need to go to the lowest level and 'ask' directly the H2 layer using fastHasRecords(). Reaching SQL is a bit costly, but is the only way to get the correct result.
• the other changes in that branch are just cosmetic: small javadoc improvements and code style updates.

Ovidiu Maxiniuc wrote:

Teodor,
Although 10937b was already merged into trunk, there are some changes in 10937a which might be useful. What you should consider:

• method TemporaryBuffer.rollback(boolean transaction). Clearly, the local.nonEmptyTableFlags set should be updated. But there are two kind of changes on undo:
  • if the original operation was a CREATE, the respective table was removed from this set. Do we add it now? We do not have the information it was there before to add it back.
  • if the original operation was a DELETE, the respective table might be deleted now. Do we remove it now?

And why do you think that the information is not enough? We can't just save local.nonEmptyTableFlags at the start of transaction and then restore it when rollback is being called?

Teodor Gorghe wrote:

I have done some performance tests with and without nonEmptyTableFlags and I can confirm that the performance is better without RoaringBitmap.

Lets confirm this with performance testing of other applications (long-running reports, performance harness/testing, etc). If it confirms that it does not regress performance, we will remove nonEmptyTableFlags.

Teodor, what testing is left here? If ETF, ChUI and unit tests for other apps passed, then is OK to merge.

They passed and the performance for the remaining apps is the same.