FWD » Core Development » Database

Profiling a client's application write heavy scenario showed that we are setting and immediately releasing savepoints during validation.
In this case, a record was already inserted and its NEW/dirty state was cleared. A later validation path still entered Validation.flushImpl(), created a savepoint, called Session.save(), and immediately released the savepoint, but Session.save() emitted no SQL. This produced an empty SAVEPOINT / RELEASE SAVEPOINT pair and incorrectly treated a no-op validation pass as a real flush.

I will change Validation.flush() so it only creates the validation savepoint when there is actual DML pending.

My first question is what happens if the trigger decides to reject the record using RETURN ERROR? buffer.maybeFireWriteTrigger(); from line 634 would raise an exception without the savepoint being set.

The second is: if (!dmo.checkState(NEW) && !dmo.isDirty()) (record is unchanged from the version from SQL) do we still need to fire the trigger? Since _ record was already inserted_ the trigger should have been fired at that time. The comment at line 632 explain that the old mechanism relies on a side-effect for avoiding the double firing, but this condition seems like a better alternative.

So, from a theoretical standpoint, it seems to me that the block at 636-642 is correct, but the one at 632-634 should be moved back?

Ovidiu,
You are right, I think the fix is to first short circuit clean, already-persisted records. Then fire the trigger only for records that still have pending work. After the trigger returns, we check again, because the trigger may have flushed the record reentrantly.
Please review rev 16623.

I will change Validation.flush() so it only creates the validation savepoint when there is actual DML pending.

This was added in some of Radu's changes long ago when "database validation" was added for _temp. Prior to his changes, there was no such "micro-savepoint", but it was added for safety. I don't recall if we found an actual bug that required this, but it was added because we feared having the database in an incorrect state if the insert failed. AFAIK, it was related to H2 temporary database. Prior to this changes, there was no _temp unique checks because indexes in _temp are not unique - this allows us to have inconsistencies in volatile data and do validation server-side. Due to performance drawbacks, we decided to add "allowDBUniqueCheck" and SOFT-UNIQUE indexes. This is a H2 customization that allowed us to toggle validation on and off. Thus, unique indexes will be throwing errors only when validation is toggled.

AFAIK, a failing INSERT would automatically rollback that operation, so the savepoint may be redundant even if there is a DML or not happening.

      if (!dmo.checkState(NOUNDO))
      {
         // preemptively set lazy savepoints in the savepoint manager before we create a "micro" savepoint
         // here, because otherwise, the save here will trigger the savepoint manager to lazily create them,
         // and we do not want those savepoints nested within this one
         SavepointManager savepointManager = session.getSavepointManager();
         if (savepointManager != null)
         {
            // this is a no-op if we are not in a transaction or we are in a full transaction block
            lastBlockSetup = savepointManager.lazilySetSavepoints(true);
         }
      }

This code in flush is setting savepoint anyway, so I guess this makes flushImpl savepoints always redundant.

PS: My comment was to remind of the origin for this savepoint. Radu, do you recall the task in which this happened? The SOFT-UNIQUE implementation.

Alexandru Lungu wrote:

PS: My comment was to remind of the origin for this savepoint. Radu, do you recall the task in which this happened? The SOFT-UNIQUE implementation.

Yes yes, it was #7323. I don't really remember the particularities of that task, but I can look into it if needed.

Ovidiu, please take a look again at rev 16623.

I have reviewed 11573a, 16622..16624. I think the solution might work.

• the history numbers in Session.java (026, 028 -> 028, 029);
• remove the 029 (only the digits at start of line 81) from Validation.java history (both 028 and 029 are part of this branch);
• move static final class DmlSavepoint before private static class Block in SavepointManager.java

Constantin, I did not change the status of the task to allow you the chance to have a look over the changes.

Thank you, Ovidiu.
I will start testing while Constantin looks over the changes.

Artur, do you have a standalone test which shows how these changes are used?

No, I will come up with one.

start.p

define variable iSourceBookId as integer initial 2 no-undo.
define variable iTargetBookId as integer initial 3 no-undo.
define variable iCopied       as integer           no-undo.

define buffer b-book for Book.

for each Book exclusive-lock:
    delete Book.
end.

do transaction:
    create Book.
    assign
        Book.book-id    = iSourceBookId
        Book.isbn       = "ISBN0001" 
        Book.book-title = "title1".

    create Book.
    assign
        Book.book-id    = iSourceBookId
        Book.isbn       = "ISBN0002" 
        Book.book-title = "title2".
end.

for each Book where
    Book.book-id = iSourceBookId
no-lock:
    do for b-book transaction:
        create b-book.
        buffer-copy Book except book-id rowpointer dttz to b-book
        assign
            b-book.book-id = iTargetBookId
            iCopied        = iCopied + 1.
    end.
end.

message "Created Book records:" iCopied.

w_book.p

trigger procedure for write of Book old buffer ob-book.

if (if Book.rowpointer = ? then "" else Book.rowpointer) = "" then
    Book.rowpointer = guid.

Book.dttz = now.

book.df

ADD TABLE "Book" 
AREA "Employee" 
DUMP-NAME "book" 
TABLE-TRIGGER "Write" NO-OVERRIDE PROCEDURE "w_book.p" CRC "?" 

ADD FIELD "book-id" OF "Book" AS integer 
  FORMAT "->,>>>,>>9" 
  INITIAL "0" 
  POSITION 2
  MAX-WIDTH 4
  ORDER 10

ADD FIELD "book-title" OF "Book" AS character 
  FORMAT "x(8)" 
  INITIAL "" 
  POSITION 3
  MAX-WIDTH 16
  ORDER 20

ADD FIELD "isbn" OF "Book" AS character 
  FORMAT "x(8)" 
  INITIAL "" 
  POSITION 4
  MAX-WIDTH 16
  ORDER 30
  MANDATORY

ADD FIELD "new_dt" OF "Book" AS character 
  FORMAT "x(8)" 
  INITIAL "" 
  POSITION 6
  MAX-WIDTH 16
  ORDER 40

ADD FIELD "rowpointer" OF "Book" AS character
  FORMAT "x(36)" 
  INITIAL ?
  POSITION 5
  MAX-WIDTH 72
  ORDER 45

ADD FIELD "ext" OF "Book" AS integer 
  FORMAT "->,>>>,>>9" 
  INITIAL "0" 
  POSITION 7
  MAX-WIDTH 66
  EXTENT 3
  ORDER 50

ADD FIELD "dttz" OF "Book" AS datetime-tz 
  FORMAT "99/99/9999 HH:MM:SS.SSS+HH:MM" 
  INITIAL ?
  POSITION 8
  MAX-WIDTH 12
  ORDER 60

ADD FIELD "dt" OF "Book" AS date 
  FORMAT "99/99/99" 
  INITIAL ?
  POSITION 9
  MAX-WIDTH 4
  ORDER 70

ADD FIELD "dtt" OF "Book" AS datetime 
  FORMAT "99/99/9999 HH:MM:SS.SSS" 
  INITIAL ?
  POSITION 10
  MAX-WIDTH 8
  ORDER 80

ADD INDEX "idx" ON "Book" 
  AREA "Employee" 
  UNIQUE
  PRIMARY
  INDEX-FIELD "book-id" ASCENDING 
  INDEX-FIELD "isbn" ASCENDING

With trunk, a savepoint is set by track undoable in the trigger procedure, on rowpointer set, then a savepoint is set on exiting the trigger procedure which flushes the record, then one savepoint is set in flushImpl after the trigger exits.

With 11573a just one savepoint is set by track undoable, then it is used by the first DML only.

Can I continue testing?

Artur, some AI observations:

• [MAJOR] functional Validation.flush: dmlSavepoint.complete() runs after the try { flushImpl(...) } finally { nursery.remove(...) } block (Validation.java:663-678). When SavepointManager.prepareDmlSavepoint takes its activeChange false path it eagerly sets the parent savepoints via lazilySetSavepoints(true) but defers the inner active block's savepoint into the afterDml lambda held by the returned DmlSavepoint (SavepointManager.java:524-532, 585-592). If flushImpl throws (validation failure, persistence error, or a ConditionException from a nested WRITE-trigger DML), complete() is skipped, activeBlock.savepoint stays null, and a subsequent UNDO exiting the inner block rolls back to the parent block's boundary instead of one that excludes the failed DML — breaking the 4GL nested-block atomicity contract. Move complete() inside the try (right after flushImpl) or into a dedicated success-only branch of finally.
• [MAJOR] functional Session.save: in the private save(dmo, updateDmoState, dmlPrepared) method persister.insert / persister.update (Session.java:1048/1053) is invoked between prepareDmlSavepoint (line 1042) and the unconditional dmlSavepoint.complete() (line 1080) with no try/finally. When prepareDmlSavepoint took the activeChange false path, the inner block's savepoint setup is deferred into DmlSavepoint.afterDml and only realized by complete(). A persister failure — the very case the savepoint is meant to protect against — skips complete(), leaving the more-deeply-nested block without its boundary savepoint while the parent's is in place: the same asymmetric rollback-boundary defect as Validation.flush. Wrap the persister call so complete() runs on success only.
• [MINOR] functional SavepointManager.prepareDmlSavepoint: when activeBlock.tracks(dmo) is false the code unconditionally selects parentBlock as the rollback-owning block (SavepointManager.java:525). Block.tracks does not walk the block stack and trackUndoable only writes to the current activeBlock, so a DMO can legitimately be tracked in a grandparent G while both activeBlock A and parentBlock P return false from tracks(dmo) — any sub-block entered between G and the DML without itself touching the DMO produces this. lazilySetSavepoints then creates a fresh savepoint at P, so the DML is bracketed by P's savepoint instead of G's; if P's sub-transaction rolls back, the DB write is undone while G.changed still records the in-memory change, diverging in-memory state from the database. Walk the block stack to locate the true owning block (or roll the tracking forward) instead of hard-coding parentBlock.
• [MINOR] style Validation.flush: blank line before closing } of method body; style rule forbids blank lines before }.
• [MINOR] style Validation: history entry 029 is the second entry added in this diff but carries a sequence ID; per project convention only the first new entry in a diff gets the sequence number, subsequent entries in the same diff have no ID.
• [MINOR] performance Validation.flush: dmo.getDirtyProps().clone() is invoked twice in the new short-circuit logic (Validation.java:634, 644). The pre-trigger clone at line 634 is dead in the common NEW/dirty path because finishIfAlreadyFlushed (line 696) returns false without consuming the parameter. Cost is small (BitSet of ~5-50 bits = 1-2 long@s), but the clone could be deferred — inline the @NEW || isDirty() check and only clone when the snapshot is actually needed for nursery.remove.

Alex,
If you agree with those points, I will fix the issues AI found.

Sorry, that was supposed to be a question :)
Should i address those findings?

Artur Școlnic wrote:

Can I continue testing?

• what happens if you remove the trigger
• what happens if what you do in the trigger is done in an inner RUN proc0 call (without the trigger).

Should i address those findings?

Please cross-check them. I will do a separate individual review my own. If you think any of the points provided is out of place, just let me know, I will double-check that.

Validation.flush
The described failure path is not quite right, when flushImpl fails, it rolls back its own savepoint, so the missing inner savepoint does not cause the claimed parent rollback. However, complete() should still run immediately after a successful flushImpl, because the later nursery cleanup can throw and prevent the savepoint boundary from being completed.

Session.save
If insert or update fails, the deferred savepoint should not be completed because no write succeeded. The real risk is that the database write succeeds, but later code throws before complete() runs. Calling complete() immediately after a successful insert or update would avoid that problem.

SavepointManager ownership
The changed record may belong to a block higher than the immediate parent. Always selecting parentBlock can put the database savepoint at the wrong nesting level. Rolling back an intermediate block could then undo the database write while leaving the record's in-memory state unchanged. We should locate the block that actually tracks the record.

The other 3 points are trivial.

I addressed the review in rev 16626.

Constantin Asofiei wrote:

in the mean time, please test:

• what happens if you remove the trigger

Only one savepoint is set, during the flush in do transaction where the actual insert is happening

• what happens if what you do in the trigger is done in an inner RUN proc0 call (without the trigger).

The procedure cannot update the record since the buffer has a no-lock. Seems like the triggers don't need an exclusive lock or are acquiring one by default.

Artur Școlnic wrote:

The procedure cannot update the record since the buffer has a no-lock. Seems like the triggers don't need an exclusive lock or are acquiring one by default.

I do not understand. b-book is an explicit buffer and defined in the external procedure, and you should be able to modify it in a proc0.

I used an inner procedure. Will test with an external one.

Oh, I was using the implicit buffer for the assign, not b-book.
Anyway, the behavior is the same, there is only one savepoint set, before the insert in the do transaction, the procedure does not affect flushing.

Artur Școlnic wrote:

Oh, I was using the implicit buffer for the assign, not b-book.
Anyway, the behavior is the same, there is only one savepoint set, before the insert in the do transaction, the procedure does not affect flushing.

• What if you do a DO TRANSACTION (a sub-tx) in proc0?
• what if you pass this buffer to another external procedure?

Constantin Asofiei wrote:

• What if you do a DO TRANSACTION (a sub-tx) in proc0?

no effect, just one savepoint set in the full transaction do block.

• what if you pass this buffer to another external procedure?

only one savepoint, set by the undoable tracking and reused by the flush.

Testing done for 11573a:
Multi tenant application: harness, performance and reports
Large gui application: unit tests (me), smoke tests (Stefanel)
Another customer application: smoke tests will be done by Razvan tomorrow morning.

Artur Școlnic wrote:

Another customer application: smoke tests will be done by Razvan tomorrow morning.

This one passed too. All testing is done.

Artur Școlnic wrote:

Artur Școlnic wrote:

Another customer application: smoke tests will be done by Razvan tomorrow morning.

This one passed too. All testing is done.

Including ETF and ChUI?

Not, just the ones I mentioned. Should I continue with those?

Artur Școlnic wrote:

Not, just the ones I mentioned. Should I continue with those?

Yes, please.

Chui regression tests passed, the only one left is ETF, which is tested by Teodor.

There are some tests from ETF which has failed and these are unexpected.
Check devsrv01:/tmp/tg.20260701/11573.zip

Teodor,
Please send me in private the 4gl sources for the failing test.

I still have not found the procedure because the test suite is large.

Please check the automated code review and see if is relevant:

Modified areas: com.goldencode.p2j.persist.orm (3 files: SavepointManager.java, Session.java, Validation.java)

Domains reviewed: fwd-persistence-layer

Functional regression¶

• [CRITICAL] functional Validation.flush: The write trigger now fires before the block rollback boundary is established. In trunk, flush() called savepointManager.lazilySetSavepoints(true) at the very top — before maybeFireWriteTrigger() — with an explicit comment that block savepoints must be set up first so the flush's own savepoint is not nested inside them. In 11573a the order is inverted: maybeFireWriteTrigger() (Validation.java:641) runs first, and the block savepoint is only established later in prepareDmlSavepoint (Validation.java:658) / by the trigger's own DML. Any WRITE trigger that (a) performs its own undoable DML, or (b) raises RETURN ERROR, now runs with a different savepoint nesting than trunk, so a subsequent block UNDO rolls back a different set of operations. This is the ordering Ovidiu flagged in review; it is the most likely cause of the ETF regression.
• [MAJOR] functional Session.save / Validation.flush: The deferred deeper-block savepoints (DmlSavepoint.afterDml) are only realized by complete(), which runs after the DML but is skipped on any exception thrown between the successful DML and complete() (e.g. nursery.remove, or a persister error in the private save). When skipped, a more-deeply-nested block is left without its boundary savepoint while the parent's is in place — an asymmetric rollback boundary. (Partially addressed by moving complete() next to flushImpl, but the window between a successful DML and complete() still exists.)
• [MAJOR] functional Block.rollback / SavepointManager.rollback: Removing the txLevel > 0 && savepoint == null early-out means a block now performs in-memory rollback (dmo.rollback(txLevel), marking records STALE) even when no DB savepoint was ever set. This is required by the deferred model, but it changes rollback semantics for savepoint-less blocks and must be validated against the case where the DB write did happen under a different block's savepoint than the one tracking the record.

If this is going to be substantial effort on your part, I could setup ETF and test myself, please let me know.

I think the first finding is relevant because if I run the tests manually, it passes. I think there is something related with a test which is expected to return an error, but leaves to a state which is unexpected, which leads to these three tests to fail.

Artur Școlnic wrote:

I could setup ETF and test myself, please let me know.

You can start doing the ETF setup, if you don't have. It will be very useful for regression testing in the future.

Teodor Gorghe wrote:

• [MAJOR] functional Block.rollback / SavepointManager.rollback: Removing the txLevel > 0 && savepoint == null early-out means a block now performs in-memory rollback (dmo.rollback(txLevel), marking records STALE) even when no DB savepoint was ever set. This is required by the deferred model, but it changes rollback semantics for savepoint-less blocks and must be validated against the case where the DB write did happen under a different block's savepoint than the one tracking the record.

I have forgot to mention. I have reverted this change from 11573a and tested again with ETF. From 10 tests which are failing, I have obtained only 9 failed tests.

Artur, I committed to testcases/1847 plenty of tests in tests/persistence/flushing_validation/flush_pipeline_regression that explore this task. In trunk, I have 9 failures, while with 11573a I have 10 failures. I attached the test.

Thank you, Alex.
I will fix the test case, then retest etf, hopefully it will be enough.

Constantin,
The regression in ETF comes from the second set of changes, but rev 16622-16623 addresses one of the issues discovered in the customer application and Eduard tested these changes separately with etf, they passed. I propose we deliver them now in the existing branch and make a 'b' branch for the second set of changes, I still have to work on those.

Artur Școlnic wrote:

Constantin,
The regression in ETF comes from the second set of changes, but rev 16622-16623 addresses one of the issues discovered in the customer application and Eduard tested these changes separately with etf, they passed. I propose we deliver them now in the existing branch and make a 'b' branch for the second set of changes, I still have to work on those.

Please update a and b branches and I'll look at them.

Done. Now 11573b contains all the changes and I will continue working on it, but 11573a contains only the trigger related changes that are fully tested and safe.

Artur Școlnic wrote:

Done. Now 11573b contains all the changes and I will continue working on it, but 11573a contains only the trigger related changes that are fully tested and safe.

Please confirm what was tested with 11573a.

I tested the multi tenant project, large gui application unit tests and etf, all passed.

Eduard, I think this is the recreate for the regression in 11573b:

do transaction:
   for each book: delete book. end.
end.

def var hb as handle.
create buffer hb for table "book".

procedure procCreate.
   hb:buffer-create().

   run procCreate2. // this doesn't seem to be important
end.

procedure procCreate2.
end.

procedure procUpdate.
   run procUpdate2.  // this call is important, this is what leaks the savepoint
end.

procedure procUpdate2.
   hb::isbn = "abcdef".
   hb::book-id = 1234.
end.

procedure raiseError.
   def var h as handle.
   h = h:next-sibling.
end.

do transaction:
   create book.
   book.isbn = "zdef".
   book.book-id = 9876.

   do transaction:
      run procCreate.

      run raiseError.  // this does a rollback

      run procUpdate.

      run raiseError. // this messes up the 'hb' record
   end.
end.

for each book:
   message book.book-id book.book-title.
end.

Yes, this is regressed by 11573b, thank you very much, Constantin.

What additional testing does this need, once it is reviewed?

I can do another 2 projects besides etf. I will also ask for help with chui regression testing. If the review is positive, testing can be done by tomorrow EOD.

Ovidiu: please review also.

Actually, I'm in progress of doing it.

Review of 11573b / r16646

I was interested in balancing of the savepoints because of another task and I had a look over 11573b.
The branch is very nicely documented, the javadocs are explaining local code. I like that, it makes the reader's job easier.

• SavepointManager.java
  • at lines 538/540: I would add a small comment here. It looks like previous can be inlined. However, this would not be correct because the call to lazilySetSavepoints() may and probably will have as side-effect the change of changeBlock.savepoint;
  • line 761: I think DmlSavepoint can be static;

• Session.java
  • lines 1002, 1048-1052: dmlSavepoint is received as parameter and updated, but not used for anything else. Something like the block at 1176-1179 is missing.

Ovidiu, thank you for catching these, I addressed the issues in rev 16647, please take a look again.

Artur, about the "flush before delete" - I think this may be problematic. The change exists only in memory until flushing. But flushing may result in an unique index collision - and delete will fail because this. Please test this,

You are right, if the flush fails, the delete fails also. I will look for another solution.

I checked and this does not make much sense in 4gl also

do transaction:
   find first Book exclusive-lock.
    Book.book-id = 1. // assume another record exists with id 1, this will raise an error and the block below will not execute.
   do on error undo, leave:
      delete Book.
      undo, leave. 
   end.
end.

Still, flushing the changes during a delete maybe is not the best approach, I will check if there are other side effects, maybe triggers.

Artur Școlnic wrote:

I fixed the regressions and some issues in trunk, details in #11625. I did a bit of a rework so the changes are better reviewed as a diff from trunk and not rev 16647.
Constantin or Ovidiu, please review rev 16648.

I see. The rework is quite extensive. I did not see anything that might look incorrect. Just an observation related to newly added stateSet from ChangeSet.java. If I understand it well, the same effect could have be obtained by changing int[] states to Integer[] states, with the following conversion semantics: stateSet[i] = (states[i] != null).

TBO, as I described the problem I encountered, I though the problem was simpler, just to fix the nested blocks counter for lazy savepoints setup. However, the solution looks much more complex.

I reverted some of the fixes for tests failing in trunk as they are incomplete and out of scope, Ovidiu, please review rev 16649.

I don't think so, as Alex said in #11625, only keeping track of the dmo state is not enough, there are RecordNursery and dirty tracking implications I did not consider, as I understand this issue is worked in #6923.
Will start testing with 11573b.