FWD » Core Development » Database

We have the perception that we have a problem with nested FINDs that reference a buffer or buffers controlled by a containing FOR loop. We should track how often this happens and if it is really as prevalent as we think, then we should measure the alternative joined approach to see what we could expect from such an optimization. This may be a first step in deciding the importance of #3194.

From what I understood, we want to test if either nested find or joined approach is better. After a few tests using two temporary tables, tt1 and tt2 with the same structure, I found out the following cases:

1. If the record in tt2 is not found, the code inside the for loop will not be executed. An error is thrown for failing to find a record.
   

      FOR EACH tt1.
         FIND FIRST tt2 WHERE tt2.field-tt21 = tt1.field-tt11.
         tt1.field-tt11 = RANDOM(1, 10) MODULO 10.
      END.
   

2. If the record in tt2 is not found, the code inside the for loop is still executed because NO-ERROR is used.
   

      FOR EACH tt1.
         FIND FIRST tt2 WHERE tt2.field-tt21 = tt1.field-tt11 NO-ERROR.
         tt1.field-tt11 = RANDOM(1, 10) MODULO 10.
      END.
   

3. The code is executed only if a record is found.
   

      FOR EACH tt1, FIRST tt2 WHERE tt2.field-tt21 = tt1.field-tt11.
         tt1.field-tt11 = RANDOM(1, 10) MODULO 10.
      END.
   

There is a big difference between using nested finds and join. Case 3 is similar to case 1, but there are instances where the user wants to run code when a record doesn't exist (case 2). I want to know if I addressed the problem correctly and if I should continue to research the issue. I don't think that it will take much long if what I found until now proves helpful.

This is a good analysis. It certainly highlights the differences in control flow and danger of refactoring such code.

However, the core questions for this task are:

• We are trying to evaluate how often in real 4GL code does this condition occur? Getting a measurement of this for different applications will help us evaluate the importance of optimizations in this area. As your findings above prove, such optimizations would have to be done carefully. Without some measurement of how often this occurs, we are just speculating that this "nested related FIND in a FOR" is a common problem.
• If this is as common as we think, then we would like to know if refactoring of the nested FIND as a multi-table FOR could be used to consistently improve performance.

Nice! Please post the diff so I can review the approach. It can be tricky to do this kind of static analysis because:

• The nesting can happen at an arbitrarily deep block level but the relationship may still be there.
• The nesting can be inside of called procedures, functions, methods.
• The linking might be done through a passed buffer parameter instead of a more direct linkage.

One other note: I think we also need to match the "FIND unique match" case. This is where the FIND has no FIRST/LAST/NEXT/PREV@/CURRENT qualifier keyword. Instead, it is a match to a unique record based on a literal or the WHERE clause.

Due to the complexities noted above, we would need to do this analysis in some prior step and cannot easily calculate this in a single expression or function. I was thinking that for this purpose it might be easier to implement some runtime tracking that could detect this no matter how the linkage occurs. We discuss this more in #3194. Please read that task and post questions as needed. This #6709 task is meant to implement the tracking/measurement of the #3194 problem so that we can determine if we should work on a solution.

Code Review 6709-rpt.diff

Neither annotations/annotations.xml and annotations/annotations_prep.xml can't be used for the nested_find.rules because it is not executed when we run analytics/reporting. You would need to use annotations/early_annotations.xml for anything that needs to be used in reporting.

There is a flaw in the nested_find.rules logic. The only time where there is a nested find is when the FIND is referencing the same exact buffer as the containing FOR. Otherwise, these are unrelated queries and should not be linked.

The current approach won't work at conversion time, but it should be possible at runtime.

Agreed. Since the analytics approach won't be able to detect the wider range of cases, I think the report is not needed. I think it is best to focus on the runtime approach.

Greg Shah wrote:

Neither annotations/annotations.xml and annotations/annotations_prep.xml can't be used for the nested_find.rules because it is not executed when we run analytics/reporting. You would need to use annotations/early_annotations.xml for anything that needs to be used in reporting.

Got it.

There is a flaw in the nested_find.rules logic. The only time where there is a nested find is when the FIND is referencing the same exact buffer as the containing FOR. Otherwise, these are unrelated queries and should not be linked.

This is my intent, but I wanted to share my work until now. Also, I guess you mean referencing the same buffer in the where clause. FOR EACH tt: FIND FIRST tt. END. doesn't work in 4GL.

Agreed. Since the analytics approach won't be able to detect the wider range of cases, I think the report is not needed. I think it is best to focus on the runtime approach.

You are right. However, the static analysis can provide all find queries inside for each (excluding the ones called inside procedures or dynamic queries). Runtime analysis can cover more cases, but only for the executed scenario. The report can deliver a lower bound which may be higher than the runtime result.

I want to finish the current report ASAP and start working on implementing the runtime tracker.

This is my intent, but I wanted to share my work until now.

Understood. No worries.

Also, I guess you mean referencing the same buffer in the where clause.

Yes, exactly. This is a kind of implicit join which could be refactored into something more performant.

I want to finish the current report ASAP and start working on implementing the runtime tracker.

OK

OK, this is close to correct.

1. Please switch out the use of a Stack for the ScopedSymbolDictionary. The idea is similar but the ScopedSymbolDictionary will search up the scopes to find a match. This is important because the match might not be with the nearest enclosing FOR EACH.

2. The following code is matching at the wrong node:

      <rule>type == prog.KW_FOR and 
            this.descendant(1, prog.KW_EACH) and
            this.nextSibling.type == prog.BLOCK
         <action>recordAst = this.getImmediateChild(prog.RECORD_PHRASE, null)</action>
...

The problem here is that it will only match one RECORD_PHRASE while there may actually be many. Also, the check on BLOCK is not needed. The better way to do this is to match on a condition like this:

      <rule>relativePath("STATEMENT/KW_FOR/KW_EACH/RECORD_PHRASE")
...

Inside that rule (i.e. when that match is true), this and copy will have token type RECORD_PHRASE.

Code Review 6709-rpt-20221205.diff

Good, it is getting closer.

1. I don't think we should limit the for each processing to only match temp-tables and buffers. All record phrase targets (tables, temp-tables, buffers, work-tables) are valid.

2. We can't use schemaname because different buffers can share the same schemaname but cannot share the same bufname. If the reference is not to the same buffer then it isn't an implicit join so it isn't a nested case.

3. The the FIND statement WHERE clause processing, you can easily limit the processing to field references using evalLib("fields") or (more explicitly) evalLib("fieldtype", this.type). This is a cleaner approach than searching for a node that has a schemaname that includes a ..

         AdaptiveQuery query4 = new AdaptiveQuery();

         forEach("loopLabel13", new Block((Init) () -> 
         {
            query4.initialize(wtcustomer, ((String) null), null, "wtcustomer.recid asc");
         }, 
         (Body) () -> 
         {
            query4.next();
            new FindQuery(tt1, "tt1.fieldTt11 < ?", null, "tt1.fieldTt11 asc", new Object[]
            {
               (P2JQuery.Parameter) () -> wtcustomer.getCustnum() // can't retrieve buffer from lambda
            }).next();
         }));

This is how a FindQuery is generated as a nested find. The problem is that I can't retrieve the buffer from the where clause. A solution for this would be to change the lambda to FieldReference.

I think FieldReference is less clean looking and it will be slower. In #4928, we do have some ideas to improve it using lambdas but it won't ever be as clean as the current approach. We could implement this as an optional conversion which could then be used by the runtime for detection of nesting. If that was only done for this kind of testing, then it might be OK.

Eric: What other ramifications are there which we need to consider?

1. I don't think we should limit the for each processing to only match temp-tables and buffers. All record phrase targets (tables, temp-tables, buffers, work-tables) are valid.

I made the changes to include tables and work-tables.

Instead of this:

         <action>recordAst = null</action>
         <rule>this.getImmediateChild(prog.TEMP_TABLE, null) != null
            <action>recordAst = this.getImmediateChild(prog.TEMP_TABLE, null)</action>
         </rule>
         <rule>this.getImmediateChild(prog.BUFFER, null) != null
            <action>recordAst = this.getImmediateChild(prog.BUFFER, null)</action>
         </rule>
         <rule>this.getImmediateChild(prog.TABLE, null) != null
            <action>recordAst = this.getImmediateChild(prog.TABLE, null)</action>
         </rule>
         <rule>this.getImmediateChild(prog.WORK_TABLE, null) != null
            <action>recordAst = this.getImmediateChild(prog.WORK_TABLE, null)</action>
         </rule>

You can just do this:

         <action>recordAst = this.getChildAt(0)</action>

There are only those 4 possible cases and they are always the first child of the RECORD_PHRASE.

Otherwise the proposed changes look good. If they pass testing, then you can commit them in 3821c.

Greg Shah wrote:

You can just do this:

    <action>recordAst = this.getChildAt(0)</action>

Did so.

If they pass testing, then you can commit them in 3821c.

I created a report before the modification above was introduced. The report was generated successfully, but no nested finds were found in the project. I plan to introduce a few non-related nested finds in the project and generate another report soon.

Committed 3821c/rev.14436. Added FOR EACH nested FIND report.

I've uncommitted 3821c rev 14436. It causes the following regression in conversion for a large customer application. See #6851-153.

Danut, please address the bug and make sure to test conversion with that application.

Looks like the problem was caused by a FOR FIRST table statement. Fixed by checking if the previous sibling of the RECORD_PHRASE is KW_EACH when getting the recordAst and saving a symbol. I took the individual statement from the file mentioned in #6851-153 and converted a similar example separately with no problems. I am currently testing conversion of the specified application and will make an update when it's done.

Greg Shah wrote:

Danut, please address the bug and make sure to test conversion with that application.

The bug is fixed and the conversion was successful. Should I commit the changes?

I am currently trying to convert

(P2JQuery.Parameter) () -> wtcustomer.getCustnum()

new FieldReference(wtcustomer, "custnum")

The bug is fixed and the conversion was successful. Should I commit the changes?

Please post an updated patch for review.

My question is where in the rules is the CustNum getter added? I want to keep wtcustomer and custnum separatelly so i can create a FieldReference.

We emit the getter or the FieldReference in convert/database_references.rules. I think the most likely scenario here is to use the accessor flag to force FieldReference to be used. Make sure to do this based on a flag that by default is off. In other words, we only want to turn this on when an explicit flag is set.

I'm OK with the 6709-20221216.patch.

You've tested conversion for both the large customer application and also Hotel GUI?

Greg Shah wrote:

You've tested conversion for both the large customer application and also Hotel GUI?

Yes, both applications converted successfully.

Can I commit the changes to 3821c?

Yes.

Committed 3821c/rev.14460. Added nested find annotation and report. Any annotated FIND that is nested in a FOR EACH will be displayed under FOR EACH Nested FIND when generating a report and classified based on the keyword used.

I will try to patch a customer application I have on my development environment to test the changes. If everything is right, I will do some preliminary tracking on that application.

Code Review 6709-20221222.diff

The conversion changes seem reasonable.

Do we really need to implement this as part of the query-off-end processing? All of this behavior is deeply specific to the persistence layer. It seems like the BufferManager or something else in persistence should easily be able to provide a list of the active FOR EACH queries. Extending the TransactionManager with even more query-specific logic is not good. At a minimum, we would only want this runtime processing when a flag is set in the directory. If it is not active, then there should be no processing and no impact of the tracking.

Although we do have some off-end processing inside the generic transaction/block processing, I really want to remove it. If this "get the list of all for each queries" processing really needs to be tracked during the block processing, I would hope we already expose the block/transaction notifications that are needed.

The matching logic in the FindQuery seems expensive. It seems like the inverse of what we need. We already have access to the buffer being referenced. Can't we just ask the buffer if it is associated with a FOR EACH query? Looping through all FOR EACH queries to see if this buffer is in the list is a lot of unnecessary work that some simple state in the buffer could answer. If we need more state in the buffer to store this, that seems a better way.

Eric: Please review and advise.