FWD » Core Development » Database

Much of the lines form output is not relevant, but uses to locate the points where the WRITE events are emitted.

• The 1st WRITE is emitted very late, leaving 2 blocks end their scope (the DO TRANSACTION, at line 17 and the procedure executed at line 28). Instead the buffer is flushed to database only when the space is needed for storing new data, ie. just before the CREATE of the external DO block.
• The 2nd WRITE is also not emitted when the logical block ends (line 26, be it simple DO or DO TRANSACTION, there is no difference) and the data is not written to db until the buffer is needed for storing the data retrieved by the FIND statement.

The current implementation of P2J fires the trigger at the latest moment possible, ie. when the content of the buffer is about to be flushed to database (RecordBuffer.flush()). In both cases, P2J will do the respective flushes as soon as the transaction blocks end their scopes, for the cases above being much earlier than P4GL.

I was not able to create a test-case with iterative statements as the buffer is automatically refreshed with the next value and the old is flushed automatically between iterations.

To match the P4GL behavior, we need to change the moment when flush() is called in P2J, the important place here being processValidate() of TransactionManager. I am not aware at this moment of the implications of removing the flush at that point and use only the indirect calls when the buffer is needed to store new info (create(), delete(), setCurrentRecord()).
Maybe the only block to trigger the flush could be the EXTERNAL_PROCEDURE ?

• the previous copy of the flushed record is kept so that it can be available as the optional OLD buffer parameter in the WRITE trigger.
• there is some kind of touched flag. If it is not set, there is no need for a flush operation.
  When the persist operation is performed, the flag is invalidated.

All these look fine and changing flush events from end of blocks was easy to implement. The problems appear when there are UNDO statements. Which are block-related. Beside the fact that they rollback the buffer to the value from the start of the block, this is the moment when I need to set back the saved copy for old parameter of WRITE trigger and, in some cases, a silent save is needed to undo buffers that were clean. In other cases only the dirty/touched flag will suffice to force the flush at a future moment.

Code from om_upd20140213a.zip merged with bzr rev. 10493 uploaded.

The simple UNDO issue is solved in the update.
However, I discovered an UNDO, NEXT issue, not related to current task; I also think cannot be fixed by implementing a new Reversible. The following behavior occurs with existing p2 code from bzr:

for each book:
    display isbn.
    isbn = "abc".
    undo, next.
end.

  at com.goldencode.p2j.persist.ProgressiveResults.cleanup(ProgressiveResults.java:565)
  at com.goldencode.p2j.persist.ResultsAdapter.cleanup(ResultsAdapter.java:296)
  at com.goldencode.p2j.persist.PreselectQuery.resetResults(PreselectQuery.java:2698)
  at com.goldencode.p2j.persist.AdaptiveQuery.resetResults(AdaptiveQuery.java:2274)
  at com.goldencode.p2j.persist.AdaptiveQuery.sessionEvent(AdaptiveQuery.java:1408)
  at com.goldencode.p2j.persist.Persistence$Context.notifySessionEvent(Persistence.java:5321)
  at com.goldencode.p2j.persist.Persistence$Context.closeSessionImpl(Persistence.java:5260)
  at com.goldencode.p2j.persist.Persistence$Context.closeSession(Persistence.java:5144)
  at com.goldencode.p2j.persist.Persistence$Context.endTransaction(Persistence.java:4776)
  at com.goldencode.p2j.persist.Persistence$Context.rollback(Persistence.java:4662)
  at com.goldencode.p2j.persist.Persistence.rollback(Persistence.java:3550)
  at com.goldencode.p2j.persist.BufferManager$DBTxWrapper.rollback(BufferManager.java:2192)
  at com.goldencode.p2j.util.TransactionManager$WorkArea.notifyMasterCommit(TransactionManager.java:5520)
  at com.goldencode.p2j.util.TransactionManager$WorkArea.access$47(TransactionManager.java:5497)
  at com.goldencode.p2j.util.TransactionManager.processRollback(TransactionManager.java:4718)
  at com.goldencode.p2j.util.TransactionManager.rollbackWorker(TransactionManager.java:1702)
  at com.goldencode.p2j.util.TransactionManager.rollback(TransactionManager.java:1589)
  at com.goldencode.p2j.util.BlockManager.undoNext(BlockManager.java:5914)
  at com.goldencode.p2j.util.BlockManager.undoNext(BlockManager.java:5892)
  at com.goldencode.testcases.write_delay.TestRoot$1$1.body(TestRoot.java:46)
  at com.goldencode.p2j.util.BlockManager.processForBody(BlockManager.java:7095)
  at com.goldencode.p2j.util.BlockManager.forEachWorker(BlockManager.java:8640)
  at com.goldencode.p2j.util.BlockManager.forEach(BlockManager.java:3717)
  at com.goldencode.testcases.write_delay.TestRoot$1.body(TestRoot.java:29)

Regarding the regression test for om_upd20140213a.zip.
The gso_ctrlc_tests were OK and the gso_ctrlc_3way_tests had some irrelevant failings.
On the other hand, gso_tests had 100+ failures, gso_rfq_tests 4 and tc_tests 50+.
Some examples of found issues:
- gso_29/step 53: Records selected: 502 instead of 475
- gso_108/step 72: Records selected: none instead of 1
- gso_114/step 44: Client reset to login screen (lost track inside the job/job63-r.p, job63-r.p/j63hmain.p)
However, I have some exceptions logged in the server log:

at com.goldencode.p2j.persist.Persistence.commit(Persistence.java:3531)
at com.goldencode.p2j.persist.RecordBuffer.flush(RecordBuffer.java:7701)
at com.goldencode.p2j.persist.RecordBuffer.setCurrentRecord(RecordBuffer.java:8420)

Attached the update form this morning that generated the also attached server log.

The sessions are still 'synchronized' with blocks and all Commitables are committed or roll-backed when a block ends
I don't think this s correct. The undo / rollback operations are normal at this point. Apparently the commit part must not execute or should let the session alive.

On the other hand, IIRC, Hibernate Sessions are intended as a short-lived objects: use-and-release, Hibernate is responsible for their long-term management.

I'm preparing a new update with a dmo-counter fix that was damaged in this update and put it to a preliminary ctrl+c test.

I attached the last version of implementation. The files contain extra logging code.
Also the current set of testcases. I did not re-check all of them, but the latest I used were successful. At the end of the main files, they contain the expected output from P4GL. Some of them also contain a header with intended cases to test.

The attached update fixes a defect which enabled write triggers to corrupt the BlockManager's context-local state.

Greg, please review.

Ovidiu, would you please review the attached update? It goes way beyond the write trigger fix to deal with the incorrect timing of our implicit validation as well.

#2371 describes the trigger behavior I am trying to match.

• upon changes to an index (this means upon a change to any field in an index for an existing record, but upon a change to all fields in an index for a new record);
• upon the release of a record from a buffer;
• upon explicit validation (VALIDATE statement);
• upon the earlier of the end of a full transaction or the end of a buffer's scope.

The last one is new, and dragged in a lot of other changes.

In my testing of the 4GL behavior, I found that our dirty database implementation was wrong in several ways. That implementation is designed to leak changes to indexes across transactions, to emulate Progress' leaky transaction isolation. I found that I needed to share awareness of changes in certain cases across buffers in the same session as well. This led me to add a new record to the dirty database immediately upon its creation (we used to wait until it was fully validated and flushed to the database).

Previously, we used the BufferFlushQueue to flush all buffers backed by the same table before any kind of query, so that the query could find a record newly created by another buffer. This was too aggressive in some cases and had to be removed, as it was triggering validation errors at the wrong times. The creating buffer is now flushed according to the validation rules listed above.

• I found cases where a query which does not use an index to sort its results can trigger the validation/flush of a new record in a separate buffer, earlier than it normally would happen (presumably to load the fields on which it needs to sort). I plan to address this in a future update.
• In certain cases (like a for each loop with a full transaction scope), a find trigger will be fired inside a transaction, whereas Progress reports the same trigger to be outside the transaction. This happens because of our implementation of these queries, which places the next method within the body of the block, instead of outside it.
• Errors raised in find triggers are not handled correctly yet, because we are launching them like UI triggers, but the launching code needs to be specific for database triggers. Actually, this probably applies to all database triggers, but I found it with a find trigger.
• Deleting a buffer object by its handle causes the buffer to flush prematurely in our implementation. This needs to be deferred.
• The removal of the BufferFlushQueue and the fact the all dirty database operations are no-ops when using temp-tables means we have no means at the moment to share awareness of new records in temp-table buffers with other buffers in the same session. Fortunately, it is much less likely to find code which relies on this, because the technique of creating a record in one buffer and then loading it in another before it is fully validated is more prevalent with persistent tables, where the developer was trying to avoid concurrency issues. Nevertheless, I intend to address this in a future update.

Various incarnations of the update have gone through many rounds (over 25) of regression testing. This version of the update currently regresses only one test scenario consistently (down from hundreds a few weeks ago). It is a particularly painful one to debug, as it takes several hours to set up, and apparently will only occur when there are some (as yet unknown) conditions in a separate session -- I spent all day debugging this one today, and in the end it worked normally, so there's some concurrency dependency there.

Anyway, I am continuing to look for the cause, but hopefully it is unlikely to result in a substantial change in the end, so I'd like you to begin your review now, even though it's not quite finished. The review may take a while -- the changes are in a particularly complicated area of the persistence framework.

Let me know if you have any questions or concerns.

Ovidiu Maxiniuc wrote:

I have not finished yet reviewing your update 20140903a, only a side-by-side compare with current bzr. At first impression is looks good. Some impressions and questions:

• I like the new TriggerTracker class that elegantly centralize information about when the trigger availability that was scattered in RecordBuffer. IIRC, I needed a stack of oldDMO to keep those 'waypoints' in UNDO events. Is it implemented some other way, or is it not really necessary ?

I don't think it is necessary, because we are copying the state of the current record at the moment before the first change is made to it. Subsequent sub-transaction commits and rollbacks may affect the current record, but shouldn't affect the old state. If the scope in which the copy was made is itself rolled back, the copy is dropped and the write trigger won't fire, because at that point any changes made at the current or more deeply nested scopes will have been undone.

• I observed the 'prototype' solution of deferring the change events, validation and flushing to be processed to the initial RecordBuffer that created the record. I went for a document-view implementation but this is a simpler faster solution.

I'm not sure what you mean. Are you talking about the code that uses creatingBuffer, or something else?

• the dirty package I must redo, I did not fully comprehend all changes.

Sorry, this is one of the most complicated areas of the persistence runtime, because it has to emulate a very bizarre quirk of Progress without the same backing infrastructure that Progress uses. The Progress database leaks uncommitted changes to indexes across transactions, because it is essentially an index engine with no notion of MVCC. When a record is inserted, removed, or moved within an index in one transaction, other transactions can see this change immediately, though they can't necessarily access other, non-indexed information until the first transaction is committed. These uncommitted changes can affect the content and sorting of search results and checking against unique constraints in unrelated transactions.

In our implementation using an MVCC database like PostgreSQL, where the indexes are encapsulated within the database, we can't walk them directly. So, we have to emulate this ridiculous behavior using an embedded H2 database which tracks these "dirty" changes as they happen. To do this, we have to track index changes faithfully, validate them at exactly the right moments, and synchronize updates to the "virtual" state of these indexes in the dirty database very carefully. Changes to the timing of validation and flushing had ripple effects throughout this implementation. Hence, the complicated set of changes.

• one question: what do you mean by "check if write trigger fires for an extent field" (the TODO at RecordBuffer.java:10241)?
  - WRITE triggers are fired at record level
  - the ASSIGN triggers (for a field) are not allowed to be defined for EXTENT fields.

I was referring to whether a change to an extent field would necessitate the later firing of a write trigger, the way a change to a scalar field does. The question occurred to me because I saw you had determined assign triggers were not allowed for extent fields. Do you have a test case that with a write trigger which changes only an extent field? I forgot to follow up on this TODO and write one.

• one more optimization that can be added. Knowing that triggers do not make sense for temp-tables, a quick-out test for permanent tables (in isTriggerEnabled maybe?) could save a few CPU ticks (by avoiding unneeded map lookups and other processing).

Good idea. I didn't realize you could not set a trigger on a temp-table. Does this hold true for session triggers, or just schema triggers?

• I saw that the update contains other changes (TemplateWorker & TempTableBuilder)

Oops, TempTableBuilder should not have been included. I was testing an interim update from Vadim and zipped it in by mistake. XmlFilePlugin and TemplateWorker are in there because I told Constantin I would implement a separate suggestion of his and roll it into my next update, but they're unrelated.

I will be back with a detailed review. I intend to run the update with my old testcases (although probably you already did that).

Actually, I did not. I created a new test case framework as this task became entangled with the validation and then dirty database implementations. I would appreciate it if you would run your old test cases against it. I'm attaching a new version which fixes defects in TriggerTracker and DatabaseEventType, removes TempTableBuilder, and has temporary debug code added to RecordBuffer.

Thank you for reviewing this.

Eric,

I reviewed closely your last update. Changes make sense. I did not find any issues except for a few javadoc (DirtyShareContext.getDirtyInfo() and the two implementations lack a blank line for separating params and @return) and in @DirtyShareMultiplexerImpl.java:201 javadoc comment is open twice.

• indeed, creatingBuffer I was referring in 2nd item from note 20. However, this must not be used only for created records. I mean, the same object should handle triggers on a record, no matter how they get in memory including the loaded ones. Please see two-buffers-for-same-record.i the attached archive. P4GL will fire the WRITE trigger only once (when the record loaded is flushed, using the first buffer that loaded the record as far as I could infer). We are triggering it twice, probably the record is written twice to database, once for each buffer referring the record (book and x-book).
• ASSIGN triggers cannot be defined on extent fields this is a compile-time error (Fields with extents may not have ASSIGN triggers. (3433)). I did not implemented this because I was told that the input code for conversion is P4GL correct (or at least it passed the P4GL compiler).
• P4GL fires the WRITE triggers on record if any field has changed, including the EXTENT ones. P2J does not fire WRITE triggers in if only EXTENT fields are changed. (See the trigger-write-from-extent.i in the archive.)
• Trying to add a trigger to a temp-table is a compile error (You may not associate database triggers with workfiles or temp-tables. (3335)). Error message not added for same reason as for ASSIGN to EXTENT fields. Schema triggers cannot be added as the temp-tables are not available at that time. I don't think that there's a way to add a database trigger to a dynamic temp table.
• Here are other testcases that failed with this update:
1. two-buffers-for-same-record2.i. Again, this proves that flushes/WRITE are triggered once per record. Validating a buffer will validate all that refer the same record. One more UI issue here: in P4GL, you can see all 29 lines. In P2J only the last 3 (24-26) are visible, that's why I used the stream for recording the output.
2. write-test5.i I believe the validation happens too early. In P4GL the entire procedure is executed, P2J stops with ** book already exists with book-id 0. (132) error.
3. write-trigger-test5.i This is the 'original' testcase described in note 1. The first WRITE is executed at correct moment (step 9). The second is still broken (step 14 instead of 16).
4. complex-with-trigger-logs.i In cleanup part the @WRITE@s are executed correctly. However, the one from block 1.1 occurs too late, in block 1.2, the last one (should have been in line 29) is never executed.

The attached archive contains the data definitions and data for table Book. I believe it is slightly different than the one from repository. The files need to be processed one at a time. I could not design a better framework. I give up of schema triggers as they were not important from this task point of view. The test-cases have the expected output (rendered by P4GL) at the end of file, in a comment. The numbers represent the buffer ids and should normally be ignored. If you need I can render the outputs in some side by side. Please let me know if I can help otherwise.

Attached update has passed regression testing and is checked into bzr revs. 10609 (all changed/new files) and 10610 (removed file I forgot to commit w/ 10609).

Ovidiu: it does not address all of the concerns from your review, but it should have fixed the multiple write trigger firing problem. I will continue working on the other issues you reported, thanks.

The scope of this issue grew way beyond the originally reported problems that impacted M11. The original problems have long since been resolved, so I am moving this issue out of M11.