FWD » Core Development » Database

containsRecordBufferDmo was a quick fix to have the 6667i branch fast tracked into trunk. It was a very aggressive change to guarantee functionality at the expense of performance. In reality, the best approach was to actually make the nursery visible inside removeRecords so that the records would have been flushed to the primary database before attempting to remove the records using a DELETE. We didn't take such decision then because it was riskier and required more testing. Also, there is a functional issue: removeRecords is reached from other code than EMPTY TEMP-TABLE (e.g. for each tt: delete tt. end.), in which case the index should be honored (i.e. only the records visible on a specific index should be deleted / made visible).

This was discussed before in #9280 and the resolution is in #8959-19. However, please wait the merge of #9275 first as it has a broad refactoring of the RecordNursery anyway.

Buffer.delete() is used for temporary tables because there is no order by clause generated at conversion, but a recid asc order by clause is added for persistent tables and this transforms the query into a PreselectQuery.

How it looks in the converted code:

   @LegacySignature(type = Type.METHOD, name = "test5")
   @Test(order = 6)
   public void test5()
   {
      internalProcedure(Test9318EmptyTempTableIndex.class, this, "test5", new Block((Body) () -> 
      {
         Buffer.delete(tt9318, "upper(f2) = 'YT'", "index11");
      }));
   }

• RecordMeta - method to retrieve all index uids.
• TemporaryBuffer - honor delete index/primary index/all indices in removeRecords(), a delete() overload to receive the delete index from a Buffer.delete() call. A deleteAll() overload to make it possible to distinguish between EMPTY TEMP-TABLE and FOR EACH .. DELETE .. END (sets implicitBulkDelete).
• RecordBuffer - added overloads mentioned in TemporaryBuffer
• Buffer - added overload to pass the delete index
• AbstractTempTable - part of distinguishing between EMPTY TEMP-TABLE and FOR EACH .. DELETE .. END
• database_access - adds the index to a Buffer.delete() call, this only happens for temp-tables
• index_selection - adds the implicit_index annotation to a record_phrase (this is then used in database_access)

Alexandru, can you do a partial review of this implementation? I want to have your opinion on this and #9318-10 - #9318-11.

• However, I am not enjoying the implicitBulkDelete and deleteIndex class members. The setter and getter of these flags are not that far away, so I would prefer to propagate them by arguments, instead of saving them at class-level. The more class members, the more a risk to unsync them.

Addressed the review from #9318-15 in 9318a/15589. I removed implicitBulkDelete and deleteIndex members and created new overloads to pass them as arguments.

• Please avoid overloading removeRecords and enforce all callers to decide what kind of delete they want. There is no clear default. You chose implicit = false and deleteIndex = null, but this means honoring the nursery on surrogate PK (recid). This is not necessary intuitive for a bulk delete. Maybe honor on all indexes will suite better. Or maybe honor on the original 4GL PK, that may be built upon a specific field. My point is that we can't assess any reasonable default here.
  • Do the same for deleteAllImpl and enforce callers to decide whether they are implicit or not. Do not overload!
• Please remove forceLoopingDelete conditional from forceLoopingDelete. The whole goal of this task is to avoid forcing loop delete if the RN has records. It is better to simply honor the nursery on the right index.
  • Without this change, your modifications aren't taking effect. Please consider proper debugging to check how your changes take effect.
• Delete on all indexes is not right. Unique indexes have positive idx and non-unique indexes have negative idx - see how RecordMeta.indexIdByName is constructed. You need to iterate from -rm.getNonuniqueIndexNames().length() to +rn.getUniqueIndexNames.length() or something like that. Please make tests and confirm by debugging.
  • Thus, I don't think getAllIndexIds is required.

Alexandru Lungu wrote:

Review of 9318a:

• Please avoid overloading removeRecords and enforce all callers to decide what kind of delete they want. There is no clear default. You chose implicit = false and deleteIndex = null, but this means honoring the nursery on surrogate PK (recid). This is not necessary intuitive for a bulk delete. Maybe honor on all indexes will suite better. Or maybe honor on the original 4GL PK, that may be built upon a specific field. My point is that we can't assess any reasonable default here.

The implementation defaults to honoring the primary index, indeed there is no clear default so I used implicit = false and deleteIndex = null. The idea is that if we have a delete index, then we will expect the implicit value to be false so that it is honored. There are three scenarios as mentioned in #9318-10, I'll have to determine this through testing as I checked if the implicit and delete index are properly set for the mentioned scenarios, while leaving the other usages of removeRecords as they were and used overloads.

• Delete on all indexes is not right. Unique indexes have positive idx and non-unique indexes have negative idx - see how RecordMeta.indexIdByName is constructed. You need to iterate from -rm.getNonuniqueIndexNames().length() to +rn.getUniqueIndexNames.length() or something like that. Please make tests and confirm by debugging.
  • Thus, I don't think getAllIndexIds is required.

indexIdByName contains both unique and nonunique indexes, the resulted array from getALlIndexIds can be sorted. The RecordNursery suggestion seems to be referring to what is used in RecordNursery.containsRecordBufferDmo(), which gets the index counts using

buffer.getDmoInfo().getIndexCount(false);

Thus using the following instead is better than relying on getAllIndexIds():

            int uniqueIndexesCount = rm.getUniqueIndexNames().length;
            int nonUniqueIndexesCount = rm.getNonuniqueIndexNames().length;
            for (int idxUid = -nonUniqueIndexesCount; idxUid <= uniqueIndexesCount; idxUid++)
            {
               rn.makeVisible(dmoId, multiplexID, idxUid);
            }

Another note for removeRecords, the Object[] args will have to be used as the final argument.

I committed 9318a/15680, still need to look into the default values. My unit tests pass, but I consider the best next step is to convert ETF and test the changes while I also extend the tests I already have.

• ETF (ddf) - already passed
• ChUI regression tests
• Customer 1 - smoke tests
• Customer 2 - smoke tests and unit tests
• Customer 3 - smoke tests, unit tests and fwdtests
• Customer 4 - harness/report/navigation

I will be emailing everyone involved after the review.