FWD » Core Development » Database

Eduard,

3) qh:get-first() find the record with recid 1432 and updated index field. In Cursor.results is added another 1432 value (now look like [1432, 1432]), however repoCache is not updated. This will cause later issues, in CompoundQuery processing.

Did you meant another qh:get-next(), right? If you do get-first a second time, the Cursor will get you the original cursor data. If you do get-next, indeed you will reach [1432, 1432].

Review of 10386a:

• I don't think the changes are right (or maybe I am misinterpreting something). The repoCache is a complex structure that maps a list of PK to a specific Row. If you have two list of PK that are identical and map to the same Row, the first prevails in OE. Try:

qh:reposition-to-rowid(rowid(tt1)). // it will reposition to the FIRST row in the cursor in OE
qh:get-next().// it will go to the SECOND row in the cursor in OE

With 10385a (although not tested), I think the reposition will go to the SECOND row directly, because it was overridden. With trunk, the add is ignored, so the reposition will in fact go (theoretically) to the first row.

Assignee set to Alexandru Lungu

Constantin, shall I pick this up or was it a mistake changing the Assignee?

Alexandru Lungu wrote:

• I don't think the changes are right (or maybe I am misinterpreting something). The repoCache is a complex structure that maps a list of PK to a specific Row. If you have two list of PK that are identical and map to the same Row, the first prevails in OE. Try:

[...]

With 10385a (although not tested), I think the reposition will go to the SECOND row directly, because it was overridden. With trunk, the add is ignored, so the reposition will in fact go (theoretically) to the first row.

You are right, reposition-to-rowid(rowid(tt1)) statement will reposition the cursor to the first element of the list.

In fact I think this is the whole problem. If you look in CompoundQuery.loadByValues, which in turn will call the query.load on each compound component. Looking at the load method in different query class (AdaptiveQuery, etc) there is a call for repositionByID method which will move the cursor to the first element in the list. next will move the cursor to the second element, because of this juggling we get a infinite loop.

Committed on 10385a, rev. 16091.

repositionByID is performed only in certain cases, added additional reposition parameter to loadByValues and load methods.

Suppose I have in cursor [1432, 1432], cursor is set on the second element. After the execution of loadByValues and subsequently of the repositionById from AdapativeQuery.load, cursor gets moved to the first element of the cursor. I mean neither FIRST/LAST/NEXT/PREV is executed at this point, and we just introduce some side effects.

Ah, I see. CompoundQuery does a NEXT and forces its AQ to load that second row, but AQ actually loads the first row because of the 4GL behavior (or loading the first row with that id). Is this the core problem here?
Apparently, loadByValues has this invariant that rows are unique, but this is not true apparently. CompoundQuery thinks that it is uniquely loading the data in the AQ, but in fact it is susceptible to this invariant flaw (cursor not having unique id).

I indeed think now that the reposition should only occur when actually using REPOSITION statement. The FIRST/NEXT/PREV/LAST should be smarter to avoid the repositioning quirk with multiple equal ids in the cursor.

I need to put more thought on it, but my very first idea is about making the repoCache actually store a counter for that id - so that rows are unique? Instead of storing the id, to store a <id, crt> pair. By default, any reposition will use <crt> set on 0, but internal usages of the reposition (e.g. triggered by loadByValues) to actually use the a more described reposition. Maybe we can encapsulate the pair as a "CursorLocator" and pass it around. Using .toIds to get only the ids just like before and .toPairs to get the list of pairs as needed by loadByValues. Eventually, .toFirstPairs to get the list of pairs where the "crt" is aggressively set to 0 (to make it work nicely with the reposition).

Alexandru Lungu wrote:

Ah, I see. CompoundQuery does a NEXT and forces its AQ to load that second row, but AQ actually loads the first row because of the 4GL behavior (or loading the first row with that id). Is this the core problem here?

This is exactly the problem.

Experimented a bit with the query a scrolling query.

def temp-table tt1 field a1 as int
                   field a2 as char
                   field a3 as int
                   field a4 as logical
                   index main_idx a1 a2 a3.

def temp-table tt2 field b1 as int.                   

create tt1.
tt1.a1 = 3.
tt1.a2 = "first".
tt1.a3 = 4.
tt1.a4 = yes.
release tt1.

create tt1.
tt1.a1 = 5.
tt1.a2 = "second".
tt1.a3 = 1.
tt1.a4 = no.
release tt1.

create tt1.
tt1.a1 = 11.
tt1.a2 = "third".
tt1.a3 = 14.
tt1.a4 = yes.
release tt1.

define query qh for tt1 scrolling.

OPEN QUERY qh FOR EACH tt1.

get first qh. // cursor [2304]

def var fRecId as rowid.

fRecId = rowid(tt1).

message "1 " available(tt1) tt1.a1 recid(tt1).

get next qh. // cursor [2304, 2305]
message "2 " available(tt1) tt1.a1 tt1.a2.

tt1.a1 = 11.
tt1.a2 = 'changed1'.
tt1.a3 = 15.         // cursor [2304, 2305, 2305]

get next qh. // cursor [2304, 2305, 2305]

message "3 " available(tt1) tt1.a1 tt1.a2.

get next qh. // cursor [2304, 2305, 2305, 2306]

message "4 " available(tt1) tt1.a1 tt1.a2.

query qh:REPOSITION-TO-ROWID(fRecId). // cursor [2304, 2305, 2305, 2306]

get next qh. // cursor [2304, 2305, 2305, 2306]

message "5 " available(tt1) tt1.a1 tt1.a2 tt1.a3.

get next qh. // cursor [2304, 2305, 2305, 2306]

message "6 " available(tt1) tt1.a1 tt1.a2 tt1.a3.

get next qh. // cursor [2304, 2305, 2305, 2306]

message "7 " available(tt1) tt1.a1 tt1.a2 tt1.a3.

tt1.a1 = 16.
tt1.a2 = 'changed1'.
tt1.a3 = 17.         // cursor [2304, 2305, 2305, 2306, 2305]

get next qh. // cursor [2304, 2305, 2305, 2306, 2305]

message "8 " available(tt1) recid(tt1) tt1.a1 tt1.a2 tt1.a3.

tt1.a1 = 25.
tt1.a2 = 'changed2'.
tt1.a3 = 12.         // cursor [2304, 2305, 2305, 2306, 2305, 2306]

get next qh.

message "9 " available(tt1) recid(tt1) tt1.a1 tt1.a2 tt1.a3.

get next qh.

message "10 " available(tt1)  recid(tt1) tt1.a1 tt1.a2 tt1.a3.

I found with changing of a index on a iteration record will put the newly created row at the end of the cursor. I think is better to store the index of the node in the cursor array..

Question, when a call to Cursor.getResult is made with the cursor positioned between the rows (lets say at position: 0.5), should the method return the value at postion: 0?

In Java docs it states the following:

Retrieve the result at the cursor's current position, which should always be at an index boundary (not between result indices).

This suggest that the cursor will be in a valid position (0, 1, 2, etc) when calling this method, however from empirical observation I saw that this is not always the case.

For example, when iterating over the query for the first time cursor.getNext, which returns null, will always position the cursor at currentPosition + 0.5. And a subsequent call to cursor.getRow will return the row at current position.

Review of 10385a:

• Please make the Locator immutable as it will be easier to keep track of its data without being concerned that it may change unexpectedly. Instead of setting ids, consider creating new instances with the right ids.
• boolean reposition = false; and reposition = true; doesn't seem relevant anymore
• Add javadoc to CursorLocator
• In PQ.repositionByID, it is weird to have the following. This is triggered when you have a query with N components and you run a reposition with M rowids (M < N).
  

  +      Long[] ids = (Long[]) locator.ids;
         ids = DBUtils.trimToTableCount(ids, this);

Alexandru Lungu wrote:

Review of 10385a:

Committed changes to handle review issues on rev. 16094.

• In PQ.repositionByID, it is weird to have the following. This is triggered when you have a query with N components and you run a reposition with M rowids (M < N).

Doesn't DBUtils.trimToTableCount method trim the ids array only when the array size is bigger then query.getTableCount();?

Doesn't DBUtils.trimToTableCount method trim the ids array only when the array size is bigger then query.getTableCount();?

Hmm, you are right. But this should still hold for Locator, right? So trimToTableCount should return another "trimmed" locator?

Alexandru Lungu wrote:

Hmm, you are right. But this should still hold for Locator, right? So trimToTableCount should return another "trimmed" locator?

Right, I committed the change of DBUtils.trimToTableCount in rev. 16095.

Review of 10385a:

• Mind that cursor. is viable is cursor is not null (i.e. the query is scrolling). If the query is not scrolling, this will rise a NPE. For instance, see AQ.repositionByID
• Please rename CursorLocator to Locator. Qualified references don't make much sense (Cursor.CursorLocator).
• Please make position null instead of -1 if you don't know the exact reposition position. Even if it seems like the position can't be negative, this will change in #9724 (where after a reposition, all prev. rows have negative indexes and next rows have positive indexes).
  • Don't forget about if (locator.position -1) kind of changes, to replace with null

Alexandru Lungu wrote:

Review of 10385a:

• Mind that cursor. is viable is cursor is not null (i.e. the query is scrolling). If the query is not scrolling, this will rise a NPE. For instance, see AQ.repositionByID
• Please rename CursorLocator to Locator. Qualified references don't make much sense (Cursor.CursorLocator).
• Please make position null instead of -1 if you don't know the exact reposition position. Even if it seems like the position can't be negative, this will change in #9724 (where after a reposition, all prev. rows have negative indexes and next rows have positive indexes).
  • Don't forget about if (locator.position -1) kind of changes, to replace with null

Committed review issues on 10385a, rev 16096.

Manage to recreate the problem that appeared in ETF.

def temp-table tt1 field f1 as char
                   field f2 as char
                   field f3 as char
                   index idx f1 f2 f3.

for each book2:
    delete book2.
end.

create book2.
book2.f1 = "something1".
release book2.

create book2.
book2.f1 = "something2".
release book2.

create tt1.
tt1.f1 = "something1".
tt1.f2 = "something2".
tt1.f3 = "something3".
release tt1.

for each tt1, each book2 where book2.f1 = tt1.f1 by tt1.f1 by book2.f1:
    message tt1.f1.
end.

Components of the for each gets converted to Preselect queries without any cursor properties, which cause the NPE.

I solved this problem in 10385a, rev. 16098, alongside some issues in the another customer application.

Alexandru Lungu wrote:

This is a CompoundQuery next operation that finds data in the cursor. It is loaded in the components using loadByValues, but one of the components AdaptiveQuery doesn't have the respective id in the rows or the floor is done incorrectly.

Let me know if you want me to dig in further and find the exact replication test-case.

I have set up the customer application, and debugged the issue. The problem is that a Cursor.locator is send from CompoundQuery to one of its components, but along the way cursor.position gets messed up with the cursor position of its component.

I committed the fix on 10385a, rev. 16357. I do however have to test it on over application.

Alex, I have rebased 10385a, to trunk rev. 16359. I had a lot of conflicts with rev. 16348, could you check if the changes from 11019a are correctly preserve.

Review:

• please fix javadoc of parameter locator in Cursor.reposition, Cursor.getLocator (both overloads), Cursor.getRepositionLocator, Locator c'tor (both overloads), AdaptiveQuery.load, AQ,getCachedRow (javadoc for load), RAQ.load, trimToTableCount
  • Please do a second pass to check if javadoc is correct.
• move getLocator public methods before the private ones
• AQ.load can have Long[] data = locator.ids; instead of Object[] data = locator.ids;
• Please let getLocator input Object[] data and use Cursor.onlyId to do the id extraction. CQ.retreieveImpl now has PK extraction that is implemented already in Cursor.
• Make Locator constructors protected so that only Cursor can create new locators with getLocator and getRepositionLocator.
  • Remove one c'tor, replace Locator(Long[] ids, double position) with Locator(Long[] ids, Double position). Let caller send null for it.

I have fixed review issues. Tested some customer applications. There is something which does not make sense to me in CQ.retrieveImpl:

Object[] subData = P2JQueryExecutor.getInstance().execute(database, 
                                                          (String) null,
                                                          CompoundQuery::processComponent, 
                                                          this,
                                                          navigation,
                                                          comp,
                                                          counters[i],
                                                          lockType,
                                                          peek);

try
{
   Cursor.Locator locator = cursor.getLocator(subData);
   comp.getQuery().load(locator, lockType, false);
}
catch (MissingRecordException exc)
{
   continue;
}

Lets say a record is retrieved in a child component with a recid 1453, at this moment we should update the cursor of the child component. After that we return to CQ, and do a load on the same component that we just retrieved. It seems a bit unnecessary to me, and potentiality prone to the same issue which this task address.

Lets suppose I have the cursor for the first query component:

Position: 4

Cursor: [1415, 1416, 1417, 1418, 1416]

I have the cursor position on 4, the next record retrieved will have the recid: 1417 and the cursor should be updated in the following manner:

Position: 5

Cursor: [1415, 1416, 1417, 1418, 1416, 1417]

And this will happen in after the execution of CompoundQuery::processComponent method. However after load, the position will be changes 2 (in case of trunk), but 10385a will thrown an NPE.

Eduard, please mind the peek option. In peek mode, you do not load buffers, but rather gather the actual data that should be loaded. In non-peek mode, you just load the buffers and move on. This is on top of my head, maybe there are other details.

Also mind that in #11021, I removed the reposition in child components when doing loadByValues and it worked mostly fine, except some errors in the browse in ChUI. If this is bothering you, please pick up the investigation effort in #11021 and lets get that fixed first. It will skip the extra friction to maintain that code in this solution.

Alexandru Lungu wrote:

Also mind that in #11021, I removed the reposition in child components when doing loadByValues and it worked mostly fine, except some errors in the browse in ChUI. If this is bothering you, please pick up the investigation effort in #11021 and lets get that fixed first. It will skip the extra friction to maintain that code in this solution.

Yes, that is exactly what is bothering me.

Alexandru Lungu wrote:

Also mind that in #11021, I removed the reposition in child components when doing loadByValues and it worked mostly fine, except some errors in the browse in ChUI. If this is bothering you, please pick up the investigation effort in #11021 and lets get that fixed first. It will skip the extra friction to maintain that code in this solution.

With the elimination of reposition in CQ components, I think the changes in this task are no longer required.