FWD » Core Development » Database

• lock all records targeted by the query
• perform a bulk delete of the records

This will happen only if the table has no DELETE triggers associated. Otherwise, a FOR EACH...: DELETE .... END. will need to be executed by deleteForAll.

I want to stress that the run-time is there, but may be flawed considering it was dead-code by now. Some part is commented with:

   /* *
    * Perform a bulk delete of all records in the table associated with this
    * buffer.
    * <p>
    * DELETE FOR was also tested with Progress 4GL with WHERE clause. It is not needed for this
    * method, but behaviour is the same. As result it is noticed that even such statement also
    * waits for whole table even if only one record is locked independently whether this record
    * satisfies WHERE clause or not . So, it seems idea of table-level locking is correct.
    * <p>
      Below is Savepoint based approach for deleteAll.
      Since we don't currently have simple answers for the Hibernate second-level cache or
      for dealing with any records currently held in buffers, we'd like to first use a simple,
      "converted-style" FOR-EACH loop as the basis for the RecordBuffer.deleteAll
      implementation. This approach leverages the code we already have to deal with these
      issues. If we find this causes memory problems, due to the bulk delete of very large
      tables, we can revisit the savepoint implementation and figure out these remaining issues.
    */

Another note: we should always force an outer tx when we do the bulk delete. Because the code like for each book exclusive-lock: delete book. end. can be executed inside an outer transaction (thus the FOR EACh is a sub-tx) and also a FULL tx. And there are differences; the point is for the bulk delete, (if not already in) create a full tx and and the delete will be 'all or nothing'.

• if this would be a full-tx, create an explicit savepoint for it
• if this is a full tx, create the full tx

I think this should cover all cases, but we need to test.

We have the bulk delete for temporary tables, maybe that can be used as a source of inspiration.

Alexandru Lungu wrote:

We have the bulk delete for temporary tables, maybe that can be used as a source of inspiration.

To some degree, but we didn't have the need to track primary keys for record locking with temp-tables, so this implementation is more complicated.

Another thing to consider: are there transient or updated records not yet flushed, which must be considered before doing a bulk delete? We had a similar issue for temp-tables. I think in that case, we fell back to a looping delete, which I'm not sure is the way to go, but that's a separate matter.

Eric Faulhaber wrote:

Alexandru Lungu wrote:

We have the bulk delete for temporary tables, maybe that can be used as a source of inspiration.

To some degree, but we didn't have the need to track primary keys for record locking with temp-tables, so this implementation is more complicated.

Another thing to consider: are there transient or updated records not yet flushed, which must be considered before doing a bulk delete? We had a similar issue for temp-tables. I think in that case, we fell back to a looping delete, which I'm not sure is the way to go, but that's a separate matter.

That's a good point, we may need to notify any buffer if their loaded record has been deleted. Or fallback to loop delete if this isn't feasible.

Starting to look into the changes ...

Review of 9656a:

added Persistence.invalidateCache, as a central API to force cache invalidation. This allows specifying a SQL to get the PKs for records which need to be exclusively-locked.

• The approach looks OK to me. Please add this new API to UnsafePersistence and do the exception handling using isUnsafe(). This allows the delegation or error management to the API consumer.
• Having an exception should attempt to unlock the records?

the DMO versioning is incremented either for the entire DMO or for the records in the SQL

• Good. Though, I think versions can be quite a huge collection, so maybe we shall be smarter on the data structure to group the identifiers by table. Or maybe we can make the DmoVersioning a per-table class instead of per-database.

current session cache evicts all records - although I think if current session has buffers with records loaded, they will not work OK; DMO versioning should be enough

• I really think this is a big problem. Buffers storing cached DMOs is an invariant AFAIK. Allowing buffer to store records that are not in the session cache is tricky. I don't have an immediate example why that is not good, but this should be investigated.
• Increasing the versioning of a DMO that is stored inside a buffer is also risky. The are plenty of checkStale calls that will simply throw StaleRecordException when attempting to SAVE.

• For troubleshooting, we can eventually log that a direct-access table lock was successfully, or if it wasn't, then report what buffers were holding the state.

If intertwining with the FWD buffer-state is a requirement here, there are plenty of things to investigate in regard to buffers having uncached or stale records.

Alexandru Lungu wrote:

• Having an exception should attempt to unlock the records?

The point is for the outer block (which should be a (sub-)tx block) will catch the exception and unlock the records. Now, the problem is if we are in a larger outer TX block, then all the records will remain locked for the duration of the outer full TX. I think this needs to be fixed.

• Though, I think versions can be quite a huge collection, so maybe we shall be smarter on the data structure to group the identifiers by table. Or maybe we can make the DmoVersioning a per-table class instead of per-database.

I'm not going to refactor this at this time.

current session cache evicts all records - although I think if current session has buffers with records loaded, they will not work OK; DMO versioning should be enough

• I really think this is a big problem. Buffers storing cached DMOs is an invariant AFAIK. Allowing buffer to store records that are not in the session cache is tricky. I don't have an immediate example why that is not good, but this should be investigated.
• Increasing the versioning of a DMO that is stored inside a buffer is also risky. The are plenty of checkStale calls that will simply throw StaleRecordException when attempting to SAVE.

The concern is only for buffers in the same session. I plan to add an API to post an event, intercepted via 'stateChanged', which will allow the buffer to refresh. We are out of 4GL behavior at this time, but the same is for direct JDBC access; but the point is for the buffers to refresh the record (or release if record was deleted).

Design-wise, maybe the goal should be to lock a table for SQL use completely. If it is in use by FWD, don't allow access. If it is not, then do not allow access to FWD on that table until unlocked by SQL. this whole paragraph is about "same-session". This will basically force a safe interaction by SQL with the database. Technically, it is a direct-access bracket.

Yes, I was thinking about this, too.

If intertwining with the FWD buffer-state is a requirement here, there are plenty of things to investigate in regard to buffers having uncached or stale records.

My thinking is notify all buffers for this DMO that they need to refresh the record. This will not 'recache' the old record.

The concern is only for buffers in the same session. I plan to add an API to post an event, intercepted via 'stateChanged', which will allow the buffer to refresh. We are out of 4GL behavior at this time, but the same is for direct JDBC access; but the point is for the buffers to refresh the record (or release if record was deleted).

Oh right, this makes sense. stateChanged should do the trick.

The state-changed 'buffer refresh' event was added to 9656a/15759 (after rebase).

I've tested how 4GL behaves if there is a case like this:

do transaction:

   for each tword exclusive-lock: message tword.w2. end.
   pause.
end.

All records in the inner FOR EACH loop remain locked for the duration of the outer DO TRANSACTION loop. This means that we are OK, if a lockSql is provided, all locked records will remain locked for the duration of the FULL TX.

The invalidateCache may be used for the future bulk-delete for permanent tables, too (but it may not be necessary, for example the 'lock' support I think should be extracted in a separated API). Also, the BULK_DELETE event needs to be implemented in RecordBuffer.stateChanged.

What I've tested at this point is:

   private void updateTword()
   {
      UnsafePersistence persistence = PersistenceFactory.getInstance("fwd", true);
      try
      {
         String sql1 = "update tword set w2 = concat(w2, 'abcdefg') where w2 = '1';";
         String sql2 = "update tword set w2 = concat(w2, 'zxcvbnm') where w2 = '2';";
         String sqld = "delete from tword where w2 in ('3', '4', '5', '6', '7');";

         int i1 = persistence.executeSQL(sql1);
         int i2 = persistence.executeSQL(sql2);
         int d = persistence.executeSQL(sqld);

         System.out.println(i1 + " " + i2 + " " + d);
      }
      catch (PersistenceException ex)
      {
         undoLeave();
      }
      finally
      {
         try
         {
            persistence.invalidateCache(Tword.class);
            persistence.refreshBuffers(Tword.class);
         }
         catch (PersistenceException e)
         {
            undoLeave();
         }
      }
   }

and it was called from a FULL TX block:

         doBlock(TransactionType.FULL, "blockLabel1", new Block((Body) () -> 
         {
            updateTword();
         }));

Constantin Asofiei wrote:

The invalidateCache may be used for the future bulk-delete for permanent tables, too (but it may not be necessary, for example the 'lock' support I think should be extracted in a separated API).

Actually, invalidateCache is needed to invalidate the cache from other sessions. And the 'lock', too.

Also, the BULK_DELETE event needs to be implemented in RecordBuffer.stateChanged.

This still stands.

Ovidiu, can you review 9656a, please? I'd like to get this into trunk.

Review of 9656a/15759.

• Persistence.java
  • I think the new methods are very useful additions;
  • line 1370: double ; :)
  • line 1373: you should use getDatabase(!dmoInfo.multiTenant). The getDatabase() will return the database of the default tenant, which is usually WRONG in MT. See the note at line 1449. This is the single serious issue I see;
  • lines 1385 and 1388: you can use the diamond (RecordIdentifier<>) and let Java infer the generic type.

Ovidiu Maxiniuc wrote:

Review of 9656a/15759.

Done, please see rev 15760.

Branch 9656a was merged to trunk rev 15762 and archived.

There is a single commit (15847) in 9633a.

Good job! The conversion part looks OK to me. The bulk delete runtime seem similar to the one from TemporaryBuffer.removeRecords(), but there are some aspects the new code handles differently (definitely-empty attribute, specific data-type for temp-tables, no multiplexing, locking).

There is one part which is not correct: the MT support. While the original code was specific to temp-tables therefore the tenant/peristence was Persistence.TEMP_CTX, in the new bulkDelete(), the Persistence.SHARED_CTX is assumed (lines 8842/8849). This is not correct. Instead the parameter should be computed as:

boolean sharedDb = !this.dmoInfo.multiTenant;

Please align the 2nd operand of && at line 8711 in RecordBuffer.java and remove @param suppDMOs tag at line8613, same file. At line 8689, there is a broken link to the old deleteAll() method.

Another thing in RecordBuffer.java is the forceLoopingDelete() method. It uses a newly created method TriggerTracker.getTracker() which in turn creates a short-lived TriggerTracker. Intentionally, instances of this class are managed inside it. Why not creating/moving the static method directly into TriggerTracker so no instances would leak?

And a final question: you created a new method bulkDelete() with a signature similar to existing removeRecords(). I was wondering if there is a advantage of overriding it?

Ovidiu Maxiniuc wrote:

There is a single commit (15847) in 9633a.

Good job! The conversion part looks OK to me. The bulk delete runtime seem similar to the one from TemporaryBuffer.removeRecords(), but there are some aspects the new code handles differently (definitely-empty attribute, specific data-type for temp-tables, no multiplexing, locking).

There is one part which is not correct: the MT support. While the original code was specific to temp-tables therefore the tenant/peristence was Persistence.TEMP_CTX, in the new bulkDelete(), the Persistence.SHARED_CTX is assumed (lines 8842/8849). This is not correct. Instead the parameter should be computed as:[...]

Please align the 2nd operand of && at line 8711 in RecordBuffer.java and remove @param suppDMOs tag at line8613, same file. At line 8689, there is a broken link to the old deleteAll() method.

Fixed. Committed them, new rev. 15848.

Another thing in RecordBuffer.java is the forceLoopingDelete() method. It uses a newly created method TriggerTracker.getTracker() which in turn creates a short-lived TriggerTracker. Intentionally, instances of this class are managed inside it. Why not creating/moving the static method directly into TriggerTracker so no instances would leak?

Make sense. I've removed RecordBufferforceLoopingDelete,TriggerTracker.getTracker and added static method hasTrigger.

And a final question: you created a new method bulkDelete() with a signature similar to existing removeRecords(). I was wondering if there is a advantage of overriding it?

There is no real advantage, personally previous naming of methods was confusing. But considering that TemporaryBuffer also has removeRecords, I've reverted this naming change.

Andrei, shouldn't loopDelete obtain an exclusive-lock on the records?

Also, the buffer scope is ignored - if a perm buffer is scoped only to that FOR EACH which is converted to deleteForAll, and it needs to have a loopDelete, when will the buffer's scope be opened?

Constantin Asofiei wrote:

Andrei, shouldn't loopDelete obtain an exclusive-lock on the records?

Right. I'll add lockType when initializing the query.

Also, the buffer scope is ignored - if a perm buffer is scoped only to that FOR EACH which is converted to deleteForAll, and it needs to have a loopDelete, when will the buffer's scope be opened?

In the init block of the for each, like it was before introducing deleteForAll.
But for bulkDelete, should I enclose everything in a doBlock to manage the scope or should I just use 'RecordBuffer.finished()'?

Rebased 9633a with trunk revision 15878, new revision is 15880.

I rebased 9633a to trunk revision 16302.

I have committed to 9633a revision 16305:

• Reverted ignore_scope annotation that avoided buffer scope duplication, because this was already solved in trunk #10567 (#10818).
• Added exclusive-lock to the loopDelete forEach block.
• Some javadoc changes.

Constantin/Alex/Ovidiu, is there anything else to do here?