FWD » Core Development » Database

Code review 20130224a:

Changes look OK to me. Note that database_access.rules needs a header entry for your changes.

I fixed the header and have re-attached the update. This is going into conversion regression testing now.

It passed conversion testing and is committed to bzr as revision 10198.

I have tested both in P2J and 4GL and also with NO-ERROR and some other examples like

create person.
person.last-name = "Bogus1".
person.ssn = "11".
delete person validate(last-name eq "BOGUS", "1 We only delete when the last name is bogus!") no-error.
delete person validate(last-name eq "BOGUS", "2 We only delete when the last name is bogus!").

in 4GL:

2 We only delete when the last name is bogus!

in P2J

** 2 We only delete when the last name is bogus!  (-1)

What else should be implemented, checked or tested in this issue?

Interesting. I have not previously seen a Progress-style error which does not have the leading asterisks and trailing error number.

Please propose a change which adjusts for these minor differences with minimal or (if possible) no change to the converted code. Other than that, if the core logic is working, there is nothing more to do with this issue.

Oh, I believed you, I just found it interesting how inconsistent Progress is.

Eric Faulhaber wrote:

Please propose a change which adjusts for these minor differences with minimal or (if possible) no change to the converted code. Other than that, if the core logic is working, there is nothing more to do with this issue.

It seems I did not see in 4GL messages enumerated with (-1). I suggest

1) use

ErrorManager.recordOrThrowError(-1, validateMessage().toStringMessage(), false);

in BufferValidator.validate() in order to supress prefix in this case

2) correct ErrorManager.buildErrorText() :

   public static String buildErrorText(int num, String text, boolean prefix)
   {
      if (num < 0)
      {
         if (prefix)
         {
            text = "** " + text;
         }
         return text;
      }
      else
      {
         String spec = (prefix ? "** %s. (%d)" : "%s. (%d)");
         return String.format(spec, text, num);
      }
   }

2 things:

1. Are you sure that failing a validation in the 4GL is considered an "ERROR" in the 4GL sense? What do the ERROR-STATUS attributes/methods return in the NO-ERROR case? Does an ERROR condition get raised in the non-NO-ERROR case? I suspect the 4GL doesn't throw an ERROR and does not update the ERROR-STATUS handle, because it is just drawing message text. This is the same way we do it when a UI validation fails. There is no ERROR condition raised in that case.

2. Don't change buildErrorText() before we really know if this is ERROR behavior. In addition, we often use a negative number in an ErrorManager call when we can't duplicate the 4GL error but we know there should be one there. The proposed result would not be correct for other callers of builtErrorText(), so the proposed change would not accepted.

Non-Error case

create person.
person.last-name = "Bogus1".
person.ssn = "11".

message "ERROR-STATUS:ERROR" ERROR-STATUS:ERROR.
message "ERROR-STATUS:NUM-MESSAGES" ERROR-STATUS:NUM-MESSAGES.
message "ERROR-STATUS:GET-NUMBER[1]" ERROR-STATUS:GET-NUMBER[1].

delete person validate(last-name eq "BOGUS", "1 We only delete when the last name is
bogus!") no-error.

message "ERROR-STATUS:ERROR" ERROR-STATUS:ERROR.
message "ERROR-STATUS:NUM-MESSAGES" ERROR-STATUS:NUM-MESSAGES.
message "ERROR-STATUS:GET-MESSAGE[1]" ERROR-STATUS:GET-MESSAGE[1].
message "ERROR-STATUS:GET-NUMBER[1]" ERROR-STATUS:GET-NUMBER[1].

ERROR-STATUS:ERROR no
ERROR-STATUS:NUM-MESSAGES 0
ERROR-STATUS:GET-NUMBER[1] 0
ERROR-STATUS:ERROR yes
ERROR-STATUS:NUM-MESSAGES 1
ERROR-STATUS:GET-MESSAGE[1] 1 We only delete when the last name isbogus!
ERROR-STATUS:GET-NUMBER[1] 0

Error case

create person.
person.last-name = "Bogus1".
person.ssn = "11".

message "ERROR-STATUS:ERROR" ERROR-STATUS:ERROR.
message "ERROR-STATUS:NUM-MESSAGES" ERROR-STATUS:NUM-MESSAGES.
message "ERROR-STATUS:GET-NUMBER[1]" ERROR-STATUS:GET-NUMBER[1].

delete person validate(last-name eq "BOGUS", "1 We only delete when the last name is
bogus!").

message "ERROR-STATUS:ERROR" ERROR-STATUS:ERROR.

ERROR-STATUS:ERROR no
ERROR-STATUS:NUM-MESSAGES 0
ERROR-STATUS:GET-NUMBER[1] 0
1 We only delete when the last name isbogus!

What about adding one more parameter to buildErrorText and recordOrThrowError:

boolean withNumber

and do not print number in message if withNumber is false? But use number, including -1, for da.addRecord(new ErrorEntry(num, errmsg))).

Or you prefer to leave message for BufferValidator.validate() like now, with number?

Before I can answer, I have one more question. In both cases (NO-ERROR and without NO-ERROR), what is returned from error-status:get-number(1)?

Greg Shah wrote:

Before I can answer, I have one more question. In both cases (NO-ERROR and without NO-ERROR), what is returned from error-status:get-number(1)?

Everywhere ERROR-STATUS:GET-NUMBER[1] 0. I have corrected examples and outputs to note 12.

I don't see the value in making changes to builtErrorText(). If I understand correctly, we just need to bypass the call to builtErrorText() in this case.

It seems to me that the solution should be this:

1. Pass an error number of 0 instead of -1.
2. Add a boolean asMsg parameter to recordOrThrowError(). As usual, just add this to the main "worker" version and pass false from the current methods so that we can minimize the change throughout P2J.
3. If asMsg is true, bypass the call to builtErrorText() and just use the given text as the message text.

If I am correct, then you can duplicate the exact behavior with that approach.

Do you plan to support Alternate Buffer Pool (since 10.2B)?

I have tested DELETE with validation when record is locked. In this case 4GL simply ignores this record.

One user executes

delete from person.
create person.
person.last-name = "Bogus".
person.ssn = "1".
FIND FIRST person EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
message "find".
FIND FIRST person EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
message "find".
FIND FIRST person EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
message "find".

FIND FIRST person EXCLUSIVE-LOCK NO-WAIT NO-ERROR.
delete person validate(last-name eq "BOGUS", "We only delete when the last name is
bogus!").
message "deleted".

** No Person record is available. (91)
We only delete when the last name is bogus!

Be careful with your conclusions here. This result can be heavily affected by the 4GL caching/flushing behavior AND by the "dirty database" support. Eric will have to respond in some detail to give thoughts about your results.

Vadim Nebogatov wrote:

Do you plan to support Alternate Buffer Pool (since 10.2B)?

Not to my knowledge. There are no BUFFER-POOL entries in the .df file of the current project.

How would this affect your implementation of this feature?

Code review 20130729a:

Code looks fine. However, I would suggest, instead of changing handle and ControlFlowOps, leave them as is, and add back an implementation of ErrorManager.recordOrThrowError(int[] nums, String[] texts, boolean prefix), which simply calls recordOrThrowError(nums, texts, prefix, false). Also add an @throws EndConditionException javadoc entry to BufferValidator.validate() to describe the special case in which EndConditionException is thrown.

After these changes, please regression test the update and check it in if it passes.

Vadim Nebogatov wrote:

I have tested DELETE with validation when record is locked. In this case 4GL simply ignores this record.

I'm not sure what you mean by "ignores this record". The second user could not obtain the record, because it was requested with a lock with the NO-WAIT, NO-ERROR options. Since it was locked by the first user, the second user's person buffer is simply empty when the delete statement is called (hence the 91 error). Please explain what this test is meant to determine.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

I have tested DELETE with validation when record is locked. In this case 4GL simply ignores this record.

I'm not sure what you mean by "ignores this record". The second user could not obtain the record, because it was requested with a lock with the NO-WAIT, NO-ERROR options. Since it was locked by the first user, the second user's person buffer is simply empty when the delete statement is called (hence the 91 error). Please explain what this test is meant to determine.

You are correct. Sometimes I forget that delete (and with validation too) works with concrete record in contrast to delete from. Please ignore my phrase. It seems my latest test is senseless.

Regression 20130801_101909 completed. There are 6 FAILED tests in ts_tests.

1. tc_job_002           FAILED            failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. tc_dc_slot_024       FAILED            failure in step 8: 'timeout before the specific screen buffer became available (Mismatched data at line 4, column 9. Expected '' (0x0000 at relative Y 4, relative X 9) and found '1' (0x0031 at relative Y 4, relative X 9).)'
3. tc_job_clock_002     FAILED            failure in step 20: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
4. tc_job_clock_004     FAILED            failure in step 3: 'Timeout while waiting for event semaphore to be posted.'
5. tc_job_matlcron_001  FAILED            failure in step 40: 'timeout before the specific screen buffer became available (Mismatched data at line 21, column 0. Expected 'R' (0x0052 at relative Y 21, relative X 0) and found '' (0x0000 at relative Y 21, relative X 0).)'
6. tc_job_matlcron_003  FAILED            failure in step 40: 'timeout before the specific screen buffer became available (Mismatched data at line 21, column 0. Expected 'R' (0x0052 at relative Y 21, relative X 0) and found '' (0x0000 at relative Y 21, relative X 0).)'

I will execute one more time when devsrv01 will be available again

Regression 20130802_053132 completed. There are 4 FAILED and 3 FAILED_DEPENDENCY tests in ts_tests.

1. tc_job_002           FAILED            failure in step 40: 'Unexpected EOF in actual at page # 1.'
2. tc_job_clock_001     FAILED            failure in step 9: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
3. tc_job_clock_003     FAILED_DEPENDENCY dependency chain: tc_job_clock_001
4. tc_job_clock_004     FAILED            failure in step 10: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
5. tc_job_clock_005     FAILED            failure in step 23: 'timeout before the specific screen buffer became available (Mismatched data at line 5, column 18. Expected '' (0x0000 at relative Y 5, relative X 18) and found '┌' (0x250C at relative Y 5, relative X 18).)'
6. tc_job_clock_006     FAILED_DEPENDENCY dependency chain: tc_job_clock_001 --> tc_job_clock_003
7. tc_dc_slot_023       FAILED_DEPENDENCY dependency chain: tc_job_clock_001 --> tc_job_clock_003 --> tc_job_clock_006

I think it is ok to commit.

Eric Faulhaber wrote:

Vadim Nebogatov wrote:

Do you plan to support Alternate Buffer Pool (since 10.2B)?

Not to my knowledge. There are no BUFFER-POOL entries in the .df file of the current project.

How would this affect your implementation of this feature?

Sorry, I missed your reply, there were no email notifications.

I am not sure if it affects implementation. I asked just because I saw the following in DELETE statement reference:

If you define an alternate buffer for a table, you can delete a record from that buffer by using the name of the buffer with the DELETE statement

vmn_upd20130731a.zip update passed regression testing and was committed to bzr rev 30372.