FWD » Core Development » Database

Summary of the Issue¶

Two key issues have been identified with the COPY-TEMP-TABLE function:

1. When Should Error 11885 Be Thrown
Legacy error 11885 is the error thrown by Progress when a COPY-TEMP-TABLE is performed on 2 temporary tables that don't match.

The error message is: A unique primary index is required in the target table, each field of which is mapped to some source field, in order to do a replace-mode or parent-mode COPY/GET/MERGE/FILL type of operation on a dataset table. (11885)

This task is meant to properly investigate what is the behavior in the legacy environment and the exact meaning of each field of which is mapped to some source field.
What we discover so far is that for each component of the destination table's primary key we must find a field in the source table with the same legacy name. This is how we are throwing the error now, but it seems that Progress is actually also checking the types of these fields and these need to also match.

1. Replicating behavior when replace-mode = TRUE

According to Progress documentation, the AVM ignores the append-mode parameter when replace-mode = TRUE.
Right now in FWD we have inconsistencies between running
hSecondTemp:COPY-TEMP-TABLE(hFirstTemp, TRUE, TRUE, TRUE). and hSecondTemp:COPY-TEMP-TABLE(hFirstTemp, FALSE, TRUE, TRUE)., but if the append-mode parameter is ignored, this should not happen.
A testcase is needed here to better understand this.

Testcase for 18855 error¶

DEFINE TEMP-TABLE ttFirst
    FIELD a AS LOGICAL
    FIELD b AS INTEGER.

DEFINE TEMP-TABLE ttSecond
    FIELD a AS INTEGER
    FIELD c AS INTEGER
    INDEX idxPrimary IS PRIMARY UNIQUE a.

DEFINE VARIABLE hFirstTemp  AS HANDLE NO-UNDO.
DEFINE VARIABLE hSecondTemp AS HANDLE NO-UNDO.

hFirstTemp  = TEMP-TABLE ttFirst:HANDLE.
hSecondTemp = TEMP-TABLE ttSecond:HANDLE.

CREATE ttFirst.
ASSIGN ttFirst.a = yes
       ttFirst.b = 11.

CREATE ttSecond.
ASSIGN ttSecond.a = 33
       ttSecond.c = 33.

hSecondTemp:COPY-TEMP-TABLE(hFirstTemp, TRUE, TRUE, TRUE).

DEFINE VARIABLE outputString AS LONGCHAR NO-UNDO.

outputString = outputString + "Contents of ttFirst:" + CHR(10).
FOR EACH ttFirst:
    outputString = outputString + 
                   "a: " + STRING(ttFirst.a) + ", " +
                   "b: " + STRING(ttFirst.b) + CHR(10).
END.

outputString = outputString + CHR(10) + "Contents of ttSecond:" + CHR(10).
FOR EACH ttSecond:
    outputString = outputString + 
                   "a: " + STRING(ttSecond.a) + ", " +
                   "c: " + STRING(ttSecond.c) + CHR(10).
END.

MESSAGE STRING(outputString) VIEW-AS ALERT-BOX.

Next steps¶

• A thorough investigation is needed to determine which data types are considered compatible in Progress. It's clear that matching data types do not necessarily need to be identical. For instance, if ttFirst.a is of type INT64 and ttSecond.a is of type INTEGER, Progress does not throw an error. Understanding the exact compatibility rules will allow us to accurately replicate this behavior in FWD.
• Create a testcase for the replace-mode issue to better understand the behavior.

I tested on the above testcase more combinations of data types.
These are the results:

It seems that beside the case where the data types are equal, the only other match would be on INT and INT64.
In the case that we were to copy here an INT64 into an INT and it would not fit, the legacy behavior is throwing these errors in order:

While testing this another weird thing came up.
This code in progress does not raise an error:

DEFINE TEMP-TABLE ttFirst
    FIELD a AS CHAR
    FIELD b AS INTEGER.

DEFINE TEMP-TABLE ttSecond
    FIELD a AS INTEGER
    FIELD c AS INTEGER
.

DEFINE VARIABLE hFirstTemp  AS HANDLE NO-UNDO.
DEFINE VARIABLE hSecondTemp AS HANDLE NO-UNDO.

hFirstTemp  = TEMP-TABLE ttFirst:HANDLE.
hSecondTemp = TEMP-TABLE ttSecond:HANDLE.

CREATE ttSecond.
ASSIGN ttSecond.a = 33
       ttSecond.c = 33.

hSecondTemp:COPY-TEMP-TABLE(hFirstTemp, FALSE, TRUE, TRUE).

DEFINE VARIABLE outputString AS LONGCHAR NO-UNDO.

outputString = outputString + "Contents of ttFirst:" + CHR(10).
FOR EACH ttFirst:
    outputString = outputString + 
                   "a: " + STRING(ttFirst.a) + ", " +
                   "b: " + STRING(ttFirst.b) + CHR(10).
END.

outputString = outputString + CHR(10) + "Contents of ttSecond:" + CHR(10).
FOR EACH ttSecond:
    outputString = outputString + 
                   "a: " + STRING(ttSecond.a) + ", " +
                   "c: " + STRING(ttSecond.c) + CHR(10).
END.

MESSAGE STRING(outputString) VIEW-AS ALERT-BOX.

The type check for emitting the 11885 error has been implemented and we now we also ensure the srcBuf is not empty before attempting to emit the error, aligning with Progress behavior.
While investigating the overflow issue where INT64 to INT raised an error in Progress but not in FWD, I found an additional functional inconsistency for cases without overflow. For example:

DEFINE TEMP-TABLE ttFirst
    FIELD a AS INT64
    FIELD b AS INTEGER.

DEFINE TEMP-TABLE ttSecond
    FIELD a AS INT
    FIELD c AS INTEGER
    INDEX idxPrimary IS PRIMARY UNIQUE a.

CREATE ttFirst.
ASSIGN ttFirst.a = 30
       ttFirst.b = 11.

CREATE ttSecond.
ASSIGN ttSecond.a = 1
       ttSecond.c = 33.

In progress the result is:

While in FWD:

In TemporaryBuffer.copyAllRows, data should be copied via:

copy((DataModelObject) srcRec, (DataModelObject) dstRec, propsMap, null); 

propsMap = createPropsMap(looseCopy, srcBufImpl, dstBufImpl, errorMode);

         boolean typesCompatible = (srcType == dstType) ||
                                   (srcType == ParmType.INT && dstType == ParmType.INT64) ||
                                   (srcType == ParmType.INT64 && dstType == ParmType.INT);

         if (srcType == null || dstType == null || !typesCompatible)

Questions to Address:¶

1. Are there other places where assigning int and in64 does not match progress behavior? Here I only updated this specific function, would overriding equals for ParmType and passing yes for int and int64 fix other issues like this?
2. When an overflow occurs, FWD raises a 15747 error and halts (with the fix explained here). In Progress, two additional errors are shown (see #9397-3). Should FWD replicate this behavior to match Progress more closely, or is the current handling sufficient?

The second error Couldn't update field 'a' of target in a BUFFER-COPY statement. (5368) does not seem to be in either ErrorManager or thrown elsewhere in FWD..

Committed on 9397a rev. 15585.
Added 5368 and 12304 errors, added type check for throwing 11885 error, fixed how AbstractTempTable.copyTempTable calls TemporaryBuffer.copyAllRows regarding skipUniqueConflicts parameter and in TemporaryBuffer.createPropsMap adjusted type check to also match INT and INT64.

From my point of view, the riskiest change here is the new error throws. I will continue with trying to create more testcases in Progress to try and obtain them in other ways and check if this still behaves as it should.

Alex/Ovidiu, please review. Thanks!

Review of 9397a rev. 15584/5.

In spite of the long list below, the update is actually good. These are hidden and edge-case scenarios we did not take into consideration until now. I am a bit concerned about the computational effort for index checking in TemporaryBuffer.copyAllRows() (which has to be executed on each copy operation) but I hope we can optimize it so it will not be observable at runtime.

• AbstractTempTable.java:
  • line 578: a space between if and the ( is required as in if (!res);
  • lines 5782 and 5783: to avoid wrapping and unwrapping, use BufferImpl.doGetName(). Ex: ((BufferImpl) srcDMO).doGetName()
  • line 575: it's a bit strange how the parameters of this method evolved. skipUniqueConflicts is documented as being ignored if replaceMode is on. However, the append and replaceMode are also passed as parameters. I assume this was the result of the observations in your testcases. This change will require testing;
  • in case of using the new ErrorManager APIs I made a habit of putting the error text in comment on the next line. These APIs have the advantage of being more centralized and offer additional support (like translation/i18n), but as you see are a bit opaque is not annotated this way. This is not mandatory, but helps the human reader understand faster what is this error about (as opposed to looking up in ErrorManager source).

• RecordBuffer.java, method copy:
  • line 3770: I do not think declaring ErrorConditionException being thrown is necessary. It is a unchecked Exception, and it is not even thrown directly;
  • lines 3806-3817: bad indentation. One extra tab, possible because these lines were put in a block while working on the issue;
  • lines 3887 and 3904: the e Exception is not checked at all. Although this is usually not an issue here, there more be more causes for executing these catch blocks. The id of the error condition should be checked and rethrow it if it is not 'interested' at this moment;
  • lines 3888 and 3905: I think propertyName is NOT the right argument. The property names are used by FWD internally. The legacy name is most likely expected, like: da.propertyMeta.getLegacyName();
  • lines 3889 and 3906: please use logical.FALSE (or logical.TRUE) constants, instead of creating new logical objects;
  • for ErrorManager.recordOrShowError(), see the above note about its opacity;

Regarding this cascading approach of catching an ErrorConditionException and use the ErrorManager to chain a subsequent one. I think this is new and I guess OE does something like this internally. Unfortunately, I think it will NOT work as you expect. At least in NO-ERROR (silent) mode, the errors are collected, but the ErrorConditionException is NOT propagated (instead the methods return false and statements ?). The new catch-es will not catch anything. I suggested in #9397-5 to use older ErrorManager.recordOrShowError(int[], String[] ...) but looking at the code this might not be possible because the individual error conditions are identified at different levels, not all at once.

• TemporaryBuffer.java:
  • line 757: both entry should 'consume' a single H entry no (213), one per merge into trunk operation;
  • line 4330: isSrcEmpty is always false due to line 4260;
  • line 4340 and next: stream API is not desired in FWD because of the performance overhead. There are several lambdas which were debatable. Especially in persistence, we struggle to optimize things as much as possible;
    • line 4345 and 4354: what does this help? If the legacy name is null (or Property, for latter), probably there is a problem which we might want to handle.
    • lines 4369 and 4371 should be indented by one space to match the left-hand operand of && operators;
    • lines 4367 to 4371: == for comparing classes is perfectly safe (instead of equals method). JVM guarantees there are no different Class instances for same class;
  • line 4481: after commit *= -1; a break or something similar is necessary. Otherwise, as the while loops, commit will alternate between 1 and -1 and finish with an unexpected result. If the loop was supposed to continue this would have been an issue. This is a smart trick, but probably commit = (commit == 0) ? 0 : -1; would have been safer (and arguably faster).

I updated the % done to 50, but it is probably higher than that.

• removed logic to rollback when a validation error occurs.
• Moved the 12304 error in copyAllRows to avoid throwing it when 11885 error is thrown.
• refactored the maps construction to not use streams.

Note: I kept the overall commit logic because it is still used when these errors are thrown:

      catch (PersistenceException exc)
      {
         commit *= -1;
         DBUtils.handleException(DatabaseManager.TEMP_TABLE_DB, exc);
         ErrorManager.recordOrThrowError(exc);
      }
      catch (IllegalAccessException | InstantiationException exc)
      {
         commit *= -1;

         throw new RuntimeException("Error creating record", exc);
      }
      catch (InvocationTargetException exc)
      {
         commit *= -1;
         DBUtils.handleException(DatabaseManager.TEMP_TABLE_DB, exc);

         throw new RuntimeException("Error creating record", exc);
      }
      catch (StopConditionException exc)
      {
         // thrown by Persistence when a JDBC-level error was encountered; Hibernate session will
         // have been closed, don't try to commit or rollback below
         commit = 0;

         if (LOG.isLoggable(Level.SEVERE))
         {
            LOG.log(Level.SEVERE, "Error copying all temp table records", exc);
         }

         throw exc;
      }

Tested the solution for these cases:

For all of the above the FWD behaviour matches progress.
I also tested with LOOSE-COPY=false and not matching schema, this also aligns.
Calling the copy operation with NO-ERROR also works as expected.

Alex/Ovidiu, please review 9397a rev. 15586-15587.

My plan is to start testing in the meantime a larger customer application to see if something was overlooked.

I have reviewed 9397a rev. 15588.

All issues I identified are done. Please also remove the unnecessary imports from TemporaryBuffer. No review needed for this.

Go ahead and start the regression tests. Good job!

Please merge 9397a to trunk now. Adjust the %Done accordingly.

9397a is merged to trunk as rev. 15627.

This can be closed.