Project

General

Profile

Feature #9543

implement missing BUFFER attributes

Added by Eric Faulhaber over 1 year ago. Updated 11 months ago.

Status:
Test
Priority:
Normal
Assignee:
Target version:
-
Start date:
Due date:
% Done:

100%

billable:
No
vendor_id:
GCD
case_num:
version_reported:
version_resolved:
production:
No
env_name:
topics:

Related issues

Related to Base Language - Bug #10294: ProDataSet BufferImpl.synchronize(DataRelation) ArrayIndexOutOfBoundsException Test

History

#1 Updated by Eric Faulhaber over 1 year ago

Attributes:

  • AUTO-SYNCHRONIZE (none/none)
  • CAN-READ (none/none)

#2 Updated by Constantin Asofiei over 1 year ago

  • Assignee set to Eduard Soltan

Eduard, please add conversion support for these two.

#3 Updated by Eduard Soltan over 1 year ago

Added conversion support on 9543a, revision 15770.

For AUTO-SYNCHRONIZE can be setable and queryable, although some experiments with setting the parameter in OE does not yield any result.

def temp-table tt1 field f1 as int.
def buffer btt1 for tt1.

buffer btt1:AUTO-SYNCHRONIZE = yes.

message buffer btt1:AUTO-SYNCHRONIZE .

The program will just message on the screen ?.

CAN-READ attribute is only queryable.

#4 Updated by Eduard Soltan over 1 year ago

  • Status changed from New to WIP
  • % Done changed from 0 to 30
  • reviewer Constantin Asofiei added

#6 Updated by Constantin Asofiei over 1 year ago

Review of @9543a, revision 15771:
  • why the AutoSynchronize interface? If the attribute is specific only to BUFFER, no need for this interface. Also, this interface is missing in the branch.
  • please update gaps, also. If the attribute is read-only, register it in common-progress.rules:read_only_attribute.
  • did you test how the setter converts for CAN-READ?
  • the @LegacyAttribute(name = "CAN-FIND") has wrong name in Buffer.java.
  • BufferImpl.canRead() still needs to use UnimplementedFeature

#7 Updated by Eduard Soltan over 1 year ago

Addressed review issues in rev. 15773.

Constantin Asofiei wrote:

  • did you test how the setter converts for CAN-READ?

Well, is converts into btt1.readOnlyError("CAN-READ", new logical(true));. And the code does not compile.

#8 Updated by Eduard Soltan over 1 year ago

Constantin Asofiei wrote:
  • did you test how the setter converts for CAN-READ?

Well, is converts into btt1.readOnlyError("CAN-READ", new logical(true));. And the code does not compile.

I added readOnlyError method in Buffer.java.

#9 Updated by Ovidiu Maxiniuc over 1 year ago

I do not think you need to add a new method.
The missing method is most likely CommonHandle.readOnlyError(String attribute, Object expr);. It is implemented in HandleChain which BufferImpl inherits.

#10 Updated by Eduard Soltan over 1 year ago

Ovidiu Maxiniuc wrote:

I do not think you need to add a new method.
The missing method is most likely CommonHandle.readOnlyError(String attribute, Object expr);. It is implemented in HandleChain which BufferImpl inherits.

You are right, but method definition still should be kept in Buffer interface.

#11 Updated by Ovidiu Maxiniuc over 1 year ago

There is another conversion issue. Instead of

btt1.readOnlyError("CAN-READ", new logical(true));
The following code should have been generated:
buffer(btt1).unwrap().readOnlyError("CAN-READ", new logical(true));

PS:
Constantin, shouldn't we generate the optimised logical.TRUE instead when a literal is used as a (~n input) parameter?

#12 Updated by Constantin Asofiei over 1 year ago

Ovidiu Maxiniuc wrote:

Constantin, shouldn't we generate the optimised logical.TRUE instead when a literal is used as a (~n input) parameter?

This is another discussion, not part of this task.

Review of 9543a rev 15775:
  • for setters, @LegacyAttribute(name = "AUTO-SYNCHRONIZE") needs to be marked with setter = true
  • gaps are not updated
  • for read-only attributes we emit like this; you do not need Buffer.readOnlyError
                         <rule>isAssign
                            <action>methodText = "readOnlyError"</action>
                            <action on="false">methodText = "getCurrentEnvironment"</action>
                         </rule>
    
    

#13 Updated by Eduard Soltan over 1 year ago

Constantin Asofiei wrote:

Ovidiu Maxiniuc wrote:

Constantin, shouldn't we generate the optimised logical.TRUE instead when a literal is used as a (~n input) parameter?

This is another discussion, not part of this task.

Review of 9543a rev 15775:
  • for setters, @LegacyAttribute(name = "AUTO-SYNCHRONIZE") needs to be marked with setter = true
  • gaps are not updated
  • for read-only attributes we emit like this; you do not need Buffer.readOnlyError
    [...]

Fixed review issue in 9543a, rev. 15776.

Now the statement buffer btt1:CAN-READ = yes will be converted as buffer(btt1).unwrap().readOnlyError("CAN-READ", new logical(true));.

#14 Updated by Constantin Asofiei over 1 year ago

You are missing history entry in a file. gaps are not updated.

#15 Updated by Eduard Soltan over 1 year ago

Constantin Asofiei wrote:

You are missing history entry in a file. gaps are not updated.

Updated gaps, and added history entry.

#16 Updated by Constantin Asofiei over 1 year ago

Eduard Soltan wrote:

Constantin Asofiei wrote:

You are missing history entry in a file. gaps are not updated.

Updated gaps, and added history entry.

Please use rt_lvl_stub, not none.

#17 Updated by Eduard Soltan over 1 year ago

Constantin Asofiei wrote:

Please use rt_lvl_stub, not none.

The necessary change added.

#18 Updated by Eduard Soltan over 1 year ago

Performed a conversion of Regression test project and it went without any problems.

#19 Updated by Constantin Asofiei over 1 year ago

  • Status changed from WIP to Merge Pending

Please merge after 9763a

#20 Updated by Eduard Soltan over 1 year ago

9543a was merged into trunk revision 15786 and archived.

#21 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Merge Pending to Test

#22 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Test to WIP

Actually, this is back to WIP.

Eduard: did you get a chance to test how these attrs should behave at runtime?

#23 Updated by Eduard Soltan about 1 year ago

According to the docs for AUTO-SYNCHRONIZE attribute and a in a book OpenEdge Development: ProDataSets by John Sadd.

When AUTO-SYNCHRONIZE is TRUE and a new row is placed in the buffer, the synchronize action occurs. The buffer:SYNCHRONIZE is called automatically when the buffer has a CREATE, DELETE, RELEASE, FIND, FOR-EACH, QUERY GET, or BUFFER-COPY run on it. 

#24 Updated by Constantin Asofiei about 1 year ago

OK, so we already support SYNCHRONIZE method via Buffer.querySynchronize - so looks like we just need to call it in those places, when AUTO-SYNCHRONIZE=true.

Please work on some tests to show how this works for each case.

#25 Updated by Eduard Soltan about 1 year ago

Created a few test cases for behaviour of AUTO-SYNCHRONIZE attributes.

output to "AUTO-SYNCHRONIZE.log".

def var h as handle.
def temp-table tt1 field f1 as int
                   field f2 as int.

def temp-table tt2 field f2 as int
                   field f3 as int.

def temp-table tt3 field f1 as int
                   field f2 as int.

/* POPULATE TT1 TEMP-TABLE */
create tt1.
tt1.f1 = 3.
tt1.f2 = 3.
release tt1.

create tt1.
tt1.f1 = 4.
tt1.f2 = 4.
release tt1.

/* POPULATE TT2 TEMP-TABLE */
create tt2.
tt2.f2 = 4.
tt2.f3 = 4.
release tt2.

create tt2.
tt2.f2 = 6.
tt2.f3 = 6.
release tt2.

create tt2.
tt2.f2 = 0.
tt2.f3 = 0.
release tt2.

define dataset dtt1 for tt1, tt2 
       data-relation rel1 for tt1, tt2 relation-fields (f2, f2).

/* SETS THE AUTO-SYNCHRONIZE ATTRIBUTE ON TT1. */

buffer tt1:AUTO-SYNCHRONIZE = yes.

find first tt2 where tt2.f2 = 6.

/* CREATE A TT1 RECORD WITH F2 = 0, WHICH SHOULD UPDATE TT2 BUFFER TO tt2.f2 = 0 */
create tt1.

message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 0 0

/* CREATE A TT1 RECORD WITH F2 = 4, WHICH SHOULD UPDATE TT2 BUFFER TO tt2.f2 = 4 */

find first tt1 where tt1.f2 = 4.

message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE TT1 RECORD WITH F2 = 4 USING BUFFER-COPY, WHICH SHOULD UPDATE TT2 BUFFER TO tt2.f2 = 4 */
create tt3.
tt3.f1 = 4.
tt3.f2 = 4.

buffer tt1:buffer-copy(buffer tt3:handle).

message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* TEST THE EFFECT of RELEASE OF TT1 ON TT2 */

find first tt2 where tt2.f2 = 6.

message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 6 6

create tt1.

message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 0 0

release tt1.

message "AVAILABLE tt2: " available(tt2). // AVAILABLE tt2: no

/* TEST THE GET FIRST ON RELASE */
def query qh for tt1.

open query qh for each tt1 where tt1.f2 = 4.
get first qh.

message "TT2 " tt2.f2 " " tt2.f3.

output close.

#26 Updated by Constantin Asofiei about 1 year ago

Can this be described in short, the synchronize is automatically called when the buffer's record is either loaded, released or deleted?

Please check another case:

/* CREATE A TT1 RECORD WITH F2 = 0, WHICH SHOULD UPDATE TT2 BUFFER TO tt2.f2 = 0 */
create tt1.
message "TT2 " tt2.f2 " " tt2.f3. // Should output TT2 0 0
tt1.f2 = 4.
message "TT2 " tt2.f2 " " tt2.f3. // TODO: what does this show?

#27 Updated by Constantin Asofiei about 1 year ago

Also, are there cases when auto-synchronize is disabled temporarily, like for save-row-changes or fill method calls?

#28 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Can this be described in short, the synchronize is automatically called when the buffer's record is either loaded, released or deleted?

Yes sure I was preparing a post for this.

define dataset dtt1 for tt1, tt2 
       data-relation rel1 for tt1, tt2 relation-fields (f2, f2).

Note the relation in dataset:
- tt1 is the main buffer
- tt2 is the child buffer
- (tt1.f2, tt2.f2) is relation fields.

tt1.create()

When we a new record tt1 gets created with tt1.create(), it gets the default values for tt1.f1 = 0 and tt1.f2 = 0. For tt2 buffer it will try to load the the first record that corresponds to the following condition tt2.f2 = tt1.f2.

If it will not find any record that do not satisfy the condition, it will just release the tt2 buffer.

tt1.release()

Release of tt1 will trigger a release of tt2.

find tt1 where tt1.f1 = 4.

Will find a record with tt1.f1 = 4 in tt1 temp-table, and will load in tt2 buffer a record with tt2.f2 = tt1.f2.

There 2 edge cases:

1. if it will not find any record in tt1 temp-table with tt2.f1 = 4 it will release both buffers.

2. if it will not find any record in tt2 temp-table with tt2.f2 = tt1.f2, it will just release the tt2 buffer.

In any on edge cases no error will be raised for tt2 buffer.

buffer tt1:buffer-copy(buffer tt3:handle).

It will try to load in tt2 temp-table the the record with tt2.f2 = tt1.f2 , note it will use the updated value of tt1.f2.

For any get first qh or for each tt1, behavior is similar to find. It tries to load in tt2 buffer the record with tt2.f2 = tt1.f2 and current value of tt2.f2.

#29 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Can this be described in short, the synchronize is automatically called when the buffer's record is either loaded, released or deleted?

Please check another case:

The change of value of the relation field from tt1, does not have any effect on the relation buffer (tt2).

I this case the output will be TT2 0 0.

#30 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Also, are there cases when auto-synchronize is disabled temporarily, like for save-row-changes or fill method calls?

I wanted to create first some simple test cases. But according to the docs the synchronize action does not occur in this case.

If the BUFFER-COPY is part of a FILL operation, a before-image operation (such as SAVE-ROW-CHANGES), or a deep-copy during parameter passing or COPY-TEMP-TABLE, then the synchronize action does not occur.

#31 Updated by Constantin Asofiei about 1 year ago

For BUFFER-COPY, if the destination buffer has no loaded record, then it CREATE's a record in that buffer. So, please test both cases.

Also, test BUFFER-COPY ... EXCEPT ... ASSIGN (i.e. the assignment for the relation field is done by ASSIGN and not BUFFER-COPY).

#32 Updated by Eduard Soltan about 1 year ago

I created some tests for buffer-copy as a method and buffer-copy statement.

buffer tt1:AUTO-SYNCHRONIZE = yes.
/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY ON NON-EMPTY BUFFER. */
release tt1.
release tt2.

create tt3.
tt3.f1 = 4.
tt3.f2 = 4.

find first tt1 where tt1.f1 = 3.
message available(tt1) available(tt2). 

buffer tt1:buffer-copy(buffer tt3:handle).

message "1. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY ON EMPTY BUFFER. */

release tt1.
release tt2.

buffer tt1:buffer-copy(buffer tt3:handle).

message "2. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING ASSIGN ON NON-EMPTY BUFFER. */

release tt1.
release tt2.

message available(tt1) available(tt2).

buffer-copy tt3  to tt1.

message "3. TT2 " tt2.f2 " " tt2.f3.

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING ASSIGN ON EMPTY BUFFER. */

release tt1.
release tt2.

message available(tt1) available(tt2).

buffer-copy tt3 to tt1 assign tt3.f1 = tt1.f1.

message "4. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING EXCEPT EXCLUDING tt1.f2 (RELATION-FIELD) ON EMPTY BUFFER. */

release tt1.
release tt2.

buffer-copy tt3 except f2 to tt1. 

message "5. TT2 " available(tt2).

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING INCLUDING ONLY tt1.f2 (RELATION-FIELD) ON EMPTY BUFFER. */

release tt1.
release tt2.

buffer-copy tt3 using f2 to tt1. 

message "6. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING EXCEPT f2 (RELATION-FIELD) AND ASSIGN OF tt1.f2 (RELATION-FIELD) ON NON-EMPTY BUFFER. */

release tt1.
release tt2.

find first tt1 where tt1.f1 = 5.
buffer-copy tt3 EXCEPT f2 to tt1 assign tt1.f2 = tt3.f2. 

message "7. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING ONLY tt1.f2 (RELATION-FIELD) ON NON-EMPTY BUFFER. */

release tt1.
release tt2.

buffer-copy tt3 using f1 to tt1. 

message "8. TT2 " available(tt2). // Should output TT2 no

/* POPULATE THE tt1 BUFFER WITH BUFFER-COPY USING tt1.f1 (non RELATION-FIELD) AND ASSIGN OF tt2.f2 (RELATION-FIELD) ON EMPTY BUFFER. */

release tt1.
release tt2.

buffer-copy tt3 using f1 to tt1 assign tt1.f2 = tt3.f2. 

message "9. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

Long story short. No matter of the state of tt1 buffer before the buffer-copy operation (empty or non-empty) as long as we update the value of relation field (tt1.f2) it will trigger automatically the update of child buffer in the relationship, tt2 buffer.

It does not matter if we are using a simple buffer-copy statement (without any except, using or assign), specify the relation field in using (using f2) or specify it in the assign (tt1.f2 = tt3.f1). BUFFER-COPY will trigger the update of tt2 buffer.

Will work now on testcases for save-row-changes and fill.

#33 Updated by Constantin Asofiei about 1 year ago

Please also double-check the ASSIGN statement if it triggers the synchronize or not. If the ASSIGN part of BUFFER-COPY triggers synchronize, and not a standalone ASSIGN, then we need to figure out a way to know that the assign is part of buffer-copy.

Also, on xfer there is tests/buffer/attributes/TestAutoSynchronize.cls - please port the tests to unit tests.

#34 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Please also double-check the ASSIGN statement if it triggers the synchronize or not. If the ASSIGN part of BUFFER-COPY triggers synchronize, and not a standalone ASSIGN, then we need to figure out a way to know that the assign is part of buffer-copy.

release tt1.
release tt2.

message available(tt1) available(tt2).
find first tt1 where tt1.f1 = 5.
buffer-copy tt3 to tt1 assign tt1.f2 = 4.

message "9. TT2 " tt2.f2 " " tt2.f3. // Should output TT2 4 4

I assign to tt1.f2 = 4, after buffer-copy it will load in tt2 buffer a record with tt2.f2 = 4. So I think that it indicates that ASSIGN statement triggers synchronize.

#35 Updated by Eduard Soltan about 1 year ago

For CAN-READ the only information I could find in the docs is:

Indicates whether the ABL user has permission to read the record associated with a buffer or buffer-field.

Data type: LOGICAL

Access: Read-only

Applies to: Buffer object handle, Buffer-field object handle

#36 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Please also double-check the ASSIGN statement if it triggers the synchronize or not. If the ASSIGN part of BUFFER-COPY triggers synchronize, and not a standalone ASSIGN, then we need to figure out a way to know that the assign is part of buffer-copy.

I see your point, looking at how it converts buffer-copy tt3 to tt1 assign tt3.f1 = tt1.f1

batch(() -> 
{
   new BufferCopy(tt3, tt1).run(false);
   tt1.setF2(tt3.getF2());
});

#37 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

Please also double-check the ASSIGN statement if it triggers the synchronize or not. If the ASSIGN part of BUFFER-COPY triggers synchronize, and not a standalone ASSIGN, then we need to figure out a way to know that the assign is part of buffer-copy.

Perhaps we could update the emit a new batch method with a boolean parameter, and in this way we could know we are in BufferCopy.

#38 Updated by Constantin Asofiei about 1 year ago

What happens if you do:

find first tt1 where tt1.f1 = 5.
tt1 assign tt1.f2 = 4.

Also, while inside BUFFER-COPY, do like:

find first tt1 where tt1.f1 = 5.
buffer-copy tt3 to tt1 assign i = func0() tt1.f2 = 4 i = func0().

where func0() checks the state of the tt2 buffer - this will tell us if the synchronize is done by BUFFER-COPY, when the ASSIGN finishes, or immediately on the tt1.f2 = 4 assignment.

#39 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

find first tt1 where tt1.f1 = 5.
tt1 assign tt1.f2 = 4.

No, the AUTO-SYNCHRONIZE does not happen at simple assign.

find first tt1 where tt1.f1 = 5.
buffer-copy tt3 to tt1 assign i = func0() tt1.f2 = 4 i = func0().

I put a message tt1.f2 available(tt2). statement in func0.

FUNCTION func0  RETURNS int ():
  message tt1.f2 available(tt2).
  return 4.
end.

find first tt1 where tt1.f1 = 3.
buffer-copy tt3 to tt1 assign i = func0() tt1.f2 = 4 i = func0().

message available(tt2).

The result is:
5 no
4 no
yes

So this proves that synchronize happens in buffer-copy.

#40 Updated by Constantin Asofiei about 1 year ago

Eduard Soltan wrote:

Constantin Asofiei wrote:

Please also double-check the ASSIGN statement if it triggers the synchronize or not. If the ASSIGN part of BUFFER-COPY triggers synchronize, and not a standalone ASSIGN, then we need to figure out a way to know that the assign is part of buffer-copy.

Perhaps we could update the emit a new batch method with a boolean parameter, and in this way we could know we are in BufferCopy.

I don't think that's needed - if the BufferCopy is emitted inside a batch assign (runtime can check this), then delay the auto-synchronize check until the batch assign finishes (with all the index validation and such). BTW, if buffer-copy fails validation, does synchronize still happen?

#41 Updated by Eduard Soltan about 1 year ago

Constantin Asofiei wrote:

BTW, if buffer-copy fails validation, does synchronize still happen?

No.

#42 Updated by Eduard Soltan about 1 year ago

  • % Done changed from 30 to 60

Committed on 9543b, rev. 15892. Added support for AUTO-SYNCHRONIZE attribute.

#43 Updated by Eduard Soltan about 1 year ago

  • Status changed from WIP to Review
  • % Done changed from 60 to 100

It seems that CAN-READ attribute is no longer used in client application. So I will move this task in review.

#44 Updated by Eduard Soltan about 1 year ago

  • reviewer Ovidiu Maxiniuc added
  • reviewer deleted (Constantin Asofiei)

#45 Updated by Ovidiu Maxiniuc about 1 year ago

Review of 9543b, r15892.

I think you are on the right track, but there are a couple of issues which should be addressed beside those with low importance.

  • BufferCopy.java:
    • line 5: copyright year should be updated;
    • line 69: the import does not seem to be in use any more. Please delete it;
  • BufferImpl.java:
    • line 5174: in case autoSyncronize.isUnknown() error condition 4083 should be raised. See the code at lines 830-834.
  • RandomAccessQuery.java:
    • line 5: copyright year should be updated;
    • line 4222: a single line is enough to separate logical blocks inside a method body;
  • RecordBuffer.java
    • line 1407: this seems like a rebasing issue. Please move your H entry after Constantin's and increment its number to 399;
    • lines 1708, 1711, 2200: I do not think having these fields/method as static is correct. In this case they must be synchronized and no two clients will be able to perform the 'batch mode assign' operation in parallel. Instead, they should probably be contextual or even member of the source buffer?
      The error might come from batch method being static, but it doesn't work with class fields, only with the parameter and local variables;
    • lines 3468: I think (but do not trust me, please test) the synchronization should happen only when the main operation was successful. In case batchError == false (similar to lines 3672, 4499, and others) the synchronization should not be performed. Or at least, it is not necessary;
    • line 9898: a single line is enough to separate logical blocks inside a method body.

#46 Updated by Eduard Soltan about 1 year ago

Ovidiu Maxiniuc wrote:

Review of 9543b, r15892.

  • RecordBuffer.java
    • lines 1708, 1711, 2200: I do not think having these fields/method as static is correct. In this case they must be synchronized and no two clients will be able to perform the 'batch mode assign' operation in parallel. Instead, they should probably be contextual or even member of the source buffer?
      The error might come from batch method being static, but it doesn't work with class fields, only with the parameter and local variables;
    • lines 3468: I think (but do not trust me, please test) the synchronization should happen only when the main operation was successful. In case batchError == false (similar to lines 3672, 4499, and others) the synchronization should not be performed. Or at least, it is not necessary;

I think you are right and it can be implemented using contextual information of BufferManager, I committed on 9543b, rev. 15893 implementation that address the issue.

#47 Updated by Ovidiu Maxiniuc about 1 year ago

Review of 9543b, r15893.

  • RecordBuffer.java:
    • line 2019: the field is missing the javadoc;
    • lines 3650, 4479, 4731: after a copy operation, I think the bufferCopyDestination flag should always be cleared, not with additional conditions. Otherwise, the flag could 'leak' to another copy operation and cause unexpected results;
    • in method copy(). At line 4438, dstBuf is marked as copy destination, but at line 4476 the flag is checked. Can it be altered meanwhile?

I think the best strategy is to put bufferCopyDestination flag in a more visible bracket (in visible pairs). In other words, the responsibility of clearing it should belong to the same method. Now this is not very clear, and this might cause imbalance in the flag's management.

#48 Updated by Eduard Soltan about 1 year ago

Ovidiu Maxiniuc wrote:

Review of 9543b, r15893.

  • RecordBuffer.java:
    • line 2019: the field is missing the javadoc;
    • lines 3650, 4479, 4731: after a copy operation, I think the bufferCopyDestination flag should always be cleared, not with additional conditions. Otherwise, the flag could 'leak' to another copy operation and cause unexpected results;
    • in method copy(). At line 4438, dstBuf is marked as copy destination, but at line 4476 the flag is checked. Can it be altered meanwhile?

I think the best strategy is to put bufferCopyDestination flag in a more visible bracket (in visible pairs). In other words, the responsibility of clearing it should belong to the same method. Now this is not very clear, and this might cause imbalance in the flag's management.

I realised something.

buffer-copy source to dest will be emitted as new BufferCopy(source, dest).run(true)

buffer-copy source to dest assing .. will be emitted as new BufferCopy(source, dest).run(false)

So I do not need to check buffer.bufferManager.isBatchAssignMode() for assign mode, I could just use validate parameter of RB.copy.

bufferCopyDestination flag explanation

In RecordBuffer there are 2 main copy method:

1) copy at line 4421 that will be executed in case of BUFFER-COPY statement runtime, and the only usage is in BufferCopy.run

For this case it will set bufferCopyDestination flag in RecordBuffer.copy method from line 4421.

Trick part here is the buffer-copy source to dest assign ... statment, because for assignBatchMode synchronize should happen only after all assign statements are executed. We could check whether we are in assignBatchMode using validate parameter of RecordBuffer.copy:

- validate = true, we are not in assign batch mode.

- validate = false, we are in assignBatchMode

In RB.endBatch I could check if BufferCopy operation was performed if the bufferCopyDestination was set, and reset it at this moment and synchronize the destination buffer.

2) copy at line 4686 that will be executed in case of BUFFER-COPY() handle-based method runtime, and it has 2 usages BufferImpl.bufferCopy and BufferImpl.saveRowChangesImpl2. This is a problem, since the AUTO-SYNCHRONIZE should happen at tt1:buffer-copy and shouldn't happen at save-row-changes.

For this case it will set bufferCopyDestination flag in BufferImpl.bufferCopy.

And will reset it either in RecordBuffer.copy in case the fastCopy will succeed or in RecordBuffer.endBatch called from line 6151.

#49 Updated by Eduard Soltan about 1 year ago

Some improvements of buffer-copy in batch mode are in 9543b, rev. 15894.

#50 Updated by Ovidiu Maxiniuc about 1 year ago

  • Status changed from Review to Internal Test
I looked at the changes of this last revision. I think they should work. A couple of notes:
  • the isBufferCopyDestination() is not necessary. bufferCopyDestination is directly accessible, no need to expose it publicly. The setter has only one external usage and its visibility might be lowered to package;
  • the declaration of bufferCopyDestination at line 2019 still lacks the javadoc. Please mention there that it is used in relation to auto-synchronisation. The name is not implying this.

Then, you can go ahead with internal testing.

#51 Updated by Eduard Soltan about 1 year ago

Unit tests and fwd tests of large application passed. An issue was reported there but it seems that it was a problem with how the patch was applied on customer branch. Retested those myself, and there are no regressions.

Unit tests of a large GUI application passed.

I am planning to run smoke tests of a GUI application, and Chui regression tests.

#52 Updated by Eduard Soltan about 1 year ago

Summary of the testing performed:

- unit tests and smoke tests of large GUI appplication ✅

- smoke tests of a GUI application ✅

- chui regression testing ✅

- harness a customer application ✅

- plus another application smoke tests. ✅

Added testcases on tests/buffer/methods/TestAutoSynchronize.cls

#53 Updated by Eduard Soltan about 1 year ago

Some additional testing should be performed or it could merged to trunk?

#54 Updated by Ovidiu Maxiniuc about 1 year ago

I think the update is safe.

But, before the merge into trunk, please add the following changes (no code affected, but please do a build test before the commit, to be on the safe side):
  • BufferCopy.java: you rolled back your changes, but the removal of the import I asked you. Either add a H entry about it or revert the file completely;
  • PreselectQuery.java: line 699 is a copy of 696. It was copied with the H new entry, I guess, does not make any sense. Please remove it.

#55 Updated by Eduard Soltan about 1 year ago

Ovidiu Maxiniuc wrote:

I think the update is safe.

But, before the merge into trunk, please add the following changes (no code affected, but please do a build test before the commit, to be on the safe side):

Done, waiting for it to be put in merge pending.

#56 Updated by Constantin Asofiei about 1 year ago

  • Status changed from Internal Test to Merge Pending

Please merge 9543b after 10027a

#57 Updated by Eduard Soltan about 1 year ago

  • Status changed from Merge Pending to Test

9543b was merged to trunk 15936 and archived.

#58 Updated by Greg Shah about 1 year ago

  • Related to Bug #10294: ProDataSet BufferImpl.synchronize(DataRelation) ArrayIndexOutOfBoundsException added

#59 Updated by Eric Faulhaber 11 months ago

Is there anything left to do on this task, or can we close it?

#60 Updated by Eduard Soltan 11 months ago

Eric Faulhaber wrote:

Is there anything left to do on this task, or can we close it?

CAN-READ runtime support is not yet implemented, it isn't used in customer application.

Also available in: Atom PDF