FWD » Core Development » Database

Tomasz,

Do you remember why in SortCriterion.parse, when adding recid to the sort criteria an asc/desc is gotten from the last component of the index? It does not make sense to me, shouldn't it be by default be asc?

Such that when all other index components are equal, it will select the first record in the table order. And for LAST all asc/desc parameters will be reversed, and the last record in the table will be selected.

Eduard Soltan wrote:

Tomasz,

Do you remember why in SortCriterion.parse, when adding recid to the sort criteria an asc/desc is gotten from the last component of the index? It does not make sense to me, shouldn't it be by default be asc?

Such that when all other index components are equal, it will select the first record in the table order. And for LAST all asc/desc parameters will be reversed, and the last record in the table will be selected.

Sorry Tomasz, I saw that you made the last change in this area, but the part of the code of interest is pretty old and it is from the beginning of the project. So please ignore my previous post.

Sorry Tomasz, I saw that you made the last change in this area, but the part of the code of interest is pretty old and it is from the beginning of the project. So please ignore my previous post.

I saw this code before and I recall we talked about this several times already. Please do some Redmine searching for this thing and quote here any tests/issues that may be relevant (already discussed).

The only places where I could find (pKeyAscend ? "asc" : "desc") mentioned is #7414-14 and #1666-15.

Ovidiu wrote in #7414-14:

Adding the PK as last component of a non unique index was done to make all indices unique, therefore we will have a predictable results when iterating the table. This is clear. The fact that the direction of this component was the same as the last legacy component is not as much for performance, but also to keep compatibility with 4GL. At least this is how I remember. The fact that we had performance issue was because initially we ignored the sorting directions completely. This was fixed and it was working correctly, for both permanent and temp-tables...

Ovidiu wrote in #1666-15:

I investigated the reverse indexed FIND s and, indeed there is a problem. At a given moment, in SortCriterion.parse(), considering only non-unique cases, I cannot tell if for the sort string these is indeed an index associated with it (eventually in reverse order) in order to add an ascending or descending id column. I tried different solutions to get this information at that moment, passing different parameters but the only solution I see now is checking the DMOIndex for the combination (and the reversed). This is because at conversion time, the index name is expanded to its components (eventually reversed for FIND LAST / FIND PREV case). On the other side, the reversed id as the last component for making an index unique will always fit the index (or it reverse) if it was declared so in the ddl.
Of course, the extra id column can stay always ascending (including for reversed searches), but in this case the queries won't benefit from indexing the tables.

I also noticed that indices are created in ddls with ascending or descending id column based on the same rule.

Unfortunately I didn't find any testcases related to this.

do you remember why in SortCriterion.parse, when adding recid to the sort criteria an asc/desc is gotten from the last component of the index? It does not make sense to me, shouldn't it be by default be asc?

Ovidiu, do you have a feedback for this? I am concerned that we might miss something.

There is a regression in Chui regression tests project. In the current implementation, recid asc is added to the sort criteria. If the query is of type last or prev for all it's the sort componenets asc/desc qualifiers is changed.

However there is a case for example find prev tt1 use-index desc_idx no-lock no-error, gets converted into query.next() query. But all of it's index sort-components are turned upside down at conversion time. In IndexHelper.lookupIndexForSort we detect whether the index was reversed at conversion time, and we could send it as a parameter that will decide whether asc or desc will be concatenated.

Alexandru Lungu wrote:

Ovidiu, do you have a feedback for this? I am concerned that we might miss something.

I created the following testcase for temp-tables:

output to \tmp\10480.txt.

define temp-table tta
  field f1 as integer
  field f2 as integer
  index idx1 as primary f1 asc f2 asc.

define temp-table ttd
  field f3 as integer
  field f4 as integer
  index idx1 as primary f3 asc f4 desc.

message "I. Populate".
create tta. f1 = 1. f2 = 1. message rowid(tta) f1 f2.
create tta. f1 = 1. f2 = 1. message rowid(tta) f1 f2.
create tta. f1 = 1. f2 = 2. message rowid(tta) f1 f2.
create tta. f1 = 1. f2 = 2. message rowid(tta) f1 f2.
create ttd. f3 = 1. f4 = 1. message rowid(ttd) f3 f4.
create ttd. f3 = 1. f4 = 1. message rowid(ttd) f3 f4.
create ttd. f3 = 1. f4 = 2. message rowid(ttd) f3 f4.
create ttd. f3 = 1. f4 = 2. message rowid(ttd) f3 f4.

message "II. Query".
message temp-table tta:name "fwd".
for each tta by f1 by f2:
    message rowid(tta) f1 f2.
end.

message temp-table ttd:name "fwd".
for each ttd by f3 by f4 descending:
    message rowid(ttd) f3 f4.
end.

message temp-table tta:name "rev".
for each tta by f1 descending by f2 descending:
    message rowid(tta) f1 f2.
end.

message temp-table ttd:name "rev".
for each ttd by f3 descending by f4:
    message rowid(ttd) f3 f4.
end.

I. Populate
0x0000000000002900 1 1
0x0000000000002901 1 1
0x0000000000002902 1 2
0x0000000000002903 1 2
0x0000000000002100 1 1
0x0000000000002101 1 1
0x0000000000002102 1 2
0x0000000000002103 1 2

II. Query
tta fwd
0x0000000000002900 1 1
0x0000000000002901 1 1
0x0000000000002902 1 2
0x0000000000002903 1 2
ttd fwd
0x0000000000002102 1 2
0x0000000000002103 1 2
0x0000000000002100 1 1
0x0000000000002101 1 1
tta rev
0x0000000000002903 1 2
0x0000000000002902 1 2
0x0000000000002901 1 1
0x0000000000002900 1 1
ttd rev
0x0000000000002101 1 1
0x0000000000002100 1 1
0x0000000000002103 1 2
0x0000000000002102 1 2

I think the real problem here is the detection when the index is used in reverse order and setting it right so that the SQL planners can actually use the desired index. When the processing order is reversed, the PKs must also be reversed. Therefore, if the index is defined as (F3 asc, F4 desc, PK asc) in SQL, the query must order by F3 desc, F4 asc, PK desc to have an index match and the result identical to 4GL.

I have encountered this case in a regression tests, only there find prev tt1 use-index desc_idx no-lock no-error query was converted into a query.next and all of its index componeents where used in reverse order.

Latest revision of 10480a, rev. 16134 fixes the test in chui tests and #10480-14.

Review of 10480a:

• I am OK with the technical approach.
• However, there are several places where SortCriterion.parse is used, so I suggest double-checking these to ensure that the direction is relevant or not (is it safe to use the ascending default?):
  • FQLHelper c'tor: this.sortCriteria = SortCriterion.parse(sort, buffer); and this.altCriteria = SortCriterion.parse(dirtyDialect, sort, dmoAlias, dmoMeta, true);
  • AdaptiveComponent.initialize: sortCriteria = Collections.unmodifiableList(SortCriterion.parse(originalSort, buffer));
  • AdaptiveComponent.addDynamicSortCriterion: sortCriteria = Collections.unmodifiableList(SortCriterion.parse(originalSort, getBuffer()));. (This can be investigated from enhanced browses when pressing on the column header to sort)
  • IndexHelper.lookupIndexForSort:
    • SortCriterion.parse(sort, buffer, false); is not assessed
    • sorters.put(key, new DMOSorter(buffer.getDatabase(), dmoIface, SortCriterion.parse(sort, buffer, true, direction))); is using customized direction.
  • PresortQuery.createResults: List<SortCriterion> criteria = SortCriterion.parse(sort, components().get(0).getBuffer());
  • QueryComponent.setIteration: List<SortCriterion> criteria = SortCriterion.parse(sort, buffer);
  • SortIndex.create: List<SortCriterion> criteria = SortCriterion.parse(sortPhrase, buffer, false);

Please make unit tests for this short-list to ensure they are properly handling these direction changes.

Added tests/table/index/TestSuite.cls unit tests.

From my observation, I noticed that recid desc is used only in case a full non-unique index is used in the query definition and all of its index components are turned upside down compared to the index definition.

Found a case in PresortQuery, where only the asc order is used for recid and a small regression in a customer application unit tests.

Fixed in 10480a, rev. 16135.

Testing of all customer applications has passed with 10480a.

Alexandru Lungu wrote:

Did this happen only with run-time testing? The fix only include DDL changes ... were these tested as well?

This includes conversion, database import and runtime

Got it. Then I think 10380a integration in trunk will be highly dependent upon customer's schedule to update the database schema, right?
Will this change be compatible with current database, right? But it will simply be slower.

Alexandru Lungu wrote:

Will this change be compatible with current database, right? But it will simply be slower.

Yes, because the right index will not be used.

Huh, this will be tough. Having it in trunk will affect at least one production customer. We will have a meeting tomorrow noon for this.

Artur, can you pick up 10480a fox performance testing (only runtime)?

Alexandru Lungu wrote:

Artur, can you pick up 10480a fox performance testing (only runtime)?

Got news that performance testing went successfully.

Eduard, see 10480a rev 16136 - The 'recid' component in the index is always 'asc' even for temp-tables.

Alexandru, this caused a weird scenario in an app, for temp-tables: you have a query which, when it doesn't match an index, acts as a 'preselect', because the 'order by' didn't match any index (as 'recid' was 'desc' instead of 'asc' in the index, and 'order by' had it like 'recid as'); if within that FOR EACH block a record in this 'preselected result set' gets deleted, FWD still iterates it - and tries to work with it. It may be that in such cases FWD emits a different type of query and gets invalidated, but at least in this scenario there was no invalidation being performed. Is this something of concern which we should further investigate, i.e. FOR EACH blocks which are not on an index and H2 'preselects' the rows, while FWD does not invalidate on DELETE?

if within that FOR EACH block a record in this 'preselected result set' gets deleted, FWD still iterates it

If it is an AdaptiveQuery, it should have been invalidated by AdaptiveQuery.stateChanged when the DELETE <buf> was called.
If it is an PreselectQuery, then PreselectQuery.stateChanged should have been called to force only id. Forcing only id will determine the query to refetch the DMO from the database and realize it is missing, so it should decide what to do based on SKIP-DELETED-RECORD.

def temp-table tt1 field f1 as int field f2 as char.
def temp-table tt2 like tt1 index ix1 f1.
def var i as int.

do i = 1 to 10:
   create tt1.
   tt1.f1 = i.
   tt1.f2 = string(i).
   release tt1.

   create tt2.
   tt2.f1 = i.
   tt2.f2 = string(i).
   release tt2.
end.

def buffer btt1 for tt1.
def var l as log.
l = true.

for each tt1 by tt1.f1:
   if l then do:
      find btt1 where btt1.f2 = "5".
      delete btt1.
   end.
   tt1.f1 = - tt1.f1.
   l = false.
   message "tt1" tt1.f1.
end.

def buffer btt2 for tt2.
l = true.
for each tt2 by tt2.f1:
   if l then do:
      find btt2 where btt2.f2 = "5".
      delete btt2.
   end.
   tt2.f1 = - tt2.f1.
   l = false.
   message "tt2" tt2.f1.
end.

• first FOR EACH gets converted to a PreselectQuery
  • the H2 query will be a LocalResultImpl which fetches the row targeted by the SELECT statement
  • in FWD, the PrelesectQuery receives the 'deleted' notification and will move to 'ids only' retrieval mode - this will let FWD to fetch every subsequent record and not rely on the returned row from the H2 ResultSet
• second FOR EACH gets converted to an AdaptiveQuery
  • in H2 query will 'walk the index', so the deleted record will not be visible.
  • in FWD, the AdaptiveQuery will not be notified for 'deleted' events, as 'lazy mode' is on for temp-tables.

So I think we are OK, but it still seems fragile - if you get an AdaptiveQuery which for some reason can't have H2 'walk the index', it will end up retrieving deleted records; maybe we should discuss in another task for some way to log something in this case.

BTW, you can duplicate this with above testcase (the second FOR EACH) if in TempTableHelper around like 515 you disable this code:

         if (!isDirty)
         {
            // add [id] to make it unique: 
            Property fieldId = ReservedProperty.ID;
            sb.append(", ").append(fieldId.column);

            if (lastDesc)
            {
               sb.append(" desc");
            }

            if (index.isUnique() && dialect.allowDBUniqueCheck())
            {
               sb.append(" soft_unique");
            }
         }

So I think we are OK, but it still seems fragile - if you get an AdaptiveQuery which for some reason can't have H2 'walk the index', it will end up retrieving deleted records;

I agree with this:

boolean lazy = (lazyMode || session.isLazyQueryExecution()) &&
                target == null && !isForUpdate && !isQuickAggregateQuery &&
                fetch != 0 && !fetchPercent && !withTies && offset == 0 && isReadOnly() &&
                indexDelegate.isUsingIndex();

This is the condition in H2 to use LAZY mode. lazyMode is the actual SQL flag, but all other conditions can negate that. For an AdaptiveQuery all should be true (aka do not use offsets, updating statements, groups, etc.). indexDelegate.isUsingIndex() is the only fragile thing here IMHO, but this is highly dependent upon sortUsingIndex. I worked on this in an early commit on FWD-H2 to always set this on true if the ORDER BY matches an index. In other words, if AdaptiveQuery is sorting on an index => sortUsingIndex is true => isUsingIndex is true => lazy is true.

maybe we should discuss in another task for some way to log something in this case.

Completely agree. But FYI, we always focused on having LAZY for queries that are naturally walking indexes in OE (i.e. AdaptiveQuery). If this is not happening, it is either that we did not convert properly or we were not aggressive enough with H2 to force him into using an index based on the ORDER BY:

• #7066 has some work to actually support USE-INDEX for forcing the iteration of an index to work together with LAZY. It has only the FWD-H2 part, but not the FWD part AFAIK.
• #7797 describes a past regression about reverse LAZY. Maybe it is related with this task.

So I think we are OK, but it still seems fragile
BTW, you can duplicate this with above testcase (the second FOR EACH) if in TempTableHelper around like 515 you disable this code:

Do you mean that #10480-29 without TempTableHelper code provided will provide different results in FWD vs OE? I am a bit mislead by "we are OK" part :)

Alexandru Lungu wrote:

BTW, you can duplicate this with above testcase (the second FOR EACH) if in TempTableHelper around like 515 you disable this code:

Do you mean that #10480-29 without TempTableHelper code provided will provide different results in FWD vs OE? I am a bit mislead by "we are OK" part :)

I mean the only/easiest way to duplicate this in FWD is to remove that code from TempTableHelper - it will show you the case where AdaptiveQuery creates a SELECT which is not walking the index in H2.

1480a was merged to trunk rev. 16235 and archived.