FWD » Core Development » Database

This is right! However, the optimization shouldn't be only on OUTER-JOIN; it can be used for non-outer join queries. Also, it can have a bigger join keys (not only fk, but fk1 = pk1 and f2 = pk2. Lastly, as you noted, the index over fk or (fk1, fk2) should be unique and all components MANDATORY.

1. QueryComponent.getWhere():
   

   FQLExpression baseWhere = getFQLPreprocessor().getFQL();
   if ((iteration != FIRST && iteration != LAST) || top)
   {
      return baseWhere;
   }
   //construct subselect and return it
   

2. In QueryComponent.prepareFQLPreprocessor():
   

   boolean subselect = !top && (iteration == FIRST || iteration == LAST);
   RecordBuffer buffer = getBuffer();
   String dropAlias = subselect ? buffer.getDMOAlias() : null;
   String replacementAlias = dropAlias != null ? DBUtils.getSubselectAlias(dropAlias) : null;
   return FQLPreprocessor.get(dfWhere,
                              buffer.getDatabase(),
                              buffer.getDialect(),
                              getReferencedBuffers(),
                              dfArgs,
                              referenceSubs,
                              true,
                              dropAlias,
                              replacementAlias,
                              false,
                              null);
   

I want to introduce a dedicated method isSubselectNeeded(FQLExpression baseWhere) to consolidate the logic for determining subselect necessity. This method would also check for index-based optimization criteria:

   private boolean isSubselectNeeded(FQLExpression baseWhere) 
   throws PersistenceException
   {
      if ((iteration != FIRST && iteration != LAST) || top)
      {
         return false; // Subselect not needed
      }

      String entity = buffer.getEntityName();
      IndexHelper indexHelper = IndexHelper.get(buffer.getDatabase());
      List<String> indexes = indexHelper.getIndexesForEntity(entity, true); // Get unique indexes

      if (isIndexOptimizable(baseWhere, indexes))
      {
         return false; 
      }

      return true; // Subselect is needed if no optimization criteria are met
   }

Right now I have a very basic check in the isIndexOptimizable(), there is still work to be done here.
Calling FQLExpression baseWhere = getFQLPreprocessor().getFQL(); within prepareFQLPreprocessor() or isSubselectNeeded() causes a circular dependency since getFQLPreprocessor() invokes prepareFQLPreprocessor(). Resolving this dependency is the next step before proceeding to refine the isIndexOptimizable() method.

• buffer.getDMOAlias() can be computed inside FqlPreprocessor as we have access to the buffer.
• DBUtils.getSubselectAlias(dropAlias) is stateless, so it doesn't pose any problem.

PS: Mind that FqlPreprocessor.preprocess does the whole work. It iterates the AST multiple times, so there are several phases of information extraction. If the information needed to check if the sub-select is required or not can be done in an earlier phase that the one that requires the dropAlias and replacementAlias.

For the record, emit, where dropAlias and replacementAlias are actually used, is the very last step. checkUniqueFind is used before emit, so you have the knowledge before actually using emit. Thus, you can always pass down the right dropAlias and replacementAlias. If you detect in checkUniqueFind that the sub-select is not required, simply make both dropAlias and replacementAlias null. dropAlias and replacementAlias are used in the cache key, but that will only mean that the FQL preprocess will be used less. Functionally, it should work, but it may affect performance of the cache - we will analyze this last. I don't know about bufferMap ... extra investigation should be conducted.

Successfully integrated propertyMatches into the subselect decision logic and the above testcase generates the simplified query.

However, my current implementation is causing regressions in a large customer application. The issue arises in a scenario involving record insertions and deletions. While the test passes successfully on the first run, subsequent runs fail due to a "record already exists" error during creation. Restarting the server did not resolve the issue, suggesting that the problem lies in the persistent database rather than the cache.

Despite debugging efforts, I have not yet pinpointed why the record is not being deleted and how my changes are contributing to this behavior. My next step will be to replicate the issue in a smaller test case for further investigation.

The problem was caused by another function detecting the need for the subselect which I had initially overlooked and was not considering the new conditions and also treating wrong the case where the baseWhere was null. With these fixes the regression is solved.

Unfortunately, while the test now runs, it appears that no queries are actually being optimized. I need to identify a query that should be optimized and investigate where the detection process is failing. I suspect that my changes in PropertyMatches might be the cause, as they were only tested with the small test case. It's possible that for more complex cases, the matches get computed to null and so preventing the optimization from being applied.

The method checks for unique constraints containing only one field and matches it with the property.

I think this should be assessed now. Most of the PK/FK are groups of more than 1 field in #8616 application.

Alexandru Lungu wrote:

I think this should be assessed now. Most of the PK/FK are groups of more than 1 field in #8616 application.

This has been addressed in the latest commit on 9317a, revision 15628.
The implementation now checks whether the properties of the join buffer appearing in the WHERE clause form a unique index and whether the corresponding properties of the other buffer involved are mandatory.
With this change, the number of optimized queries in the test has increased to 260 and local performance tests show promising results. Yesterday, the local tests duration varied between 9.5 and 10 seconds and now the average has improved to 8.8 seconds. A full performance test has been started on the bare metal.

Alex/Eric, please review 9317a, revisions 15624–15628.

Overall, I would want to refactor a bit the javadoc/parameter and method names to reflect the join with a FIRST/LAST scenario. We used the "subselect" term to reflect that such construct will convert in an SQL subselect. However, this is confusing as it can also refer to CAN-FIND that may be embedded as a sub-select. Lets find another keyword for it - lets say "joinWithSubselect".

• QueryComponent
  • rename hasSubselect to joinWithSubselect and getter to isJoinWithSubselect. Edit the javadoc.
  • I must admit that I am concerned that hasSubselect is a mutable flag that is changed both in prepareFQLPreprocessor and getWhere and is queried by getWhere and getHasSubselect. Thus, the computation of this flag is highly dependent on the order in which these set/get methods are called. The state automata is too complex. If one calls getHasSubselect somewhere in before/after the getWhere/prepareFQLPreprocessor code, then the caller will get stale data. Lets make the code a bit more robust and proof to future changes.
    • make getHasSubselect return getFQLPreprocessor().isSubselectNeeded
    • change getWhere to simply check if (!proc.isSubselectNeeded()) return baseWhere;
    • restore the subselect flag in prepareFQLPreprocessor
    • overall, isSubselectNeeded is already cached in FQLPreprocessor, so it doesn't need another caching in QueryComponent
• PreselectQuery
  • I don't understand the change here. At first sight, I would have opted for a (iteration != FIRST && iteration != LAST) || !comp.getHasSubselect() (note the use of || instead of &&). Please make some tests for this decision. I think you need something like for each a, first b on a, for each a, last b on a, for each a, each b on a (note that a and b should share some common fields with the same name and unique indexed). Do you have a test-case that motivates this change?
• FQLPreprocessor
  • collectPropertyMatches is changing the state of the preprocessor. As you call it once again, you may affect the output of the first collectPropertyMatches.
    • Does it make sense to run collectPropertyMatches only once (where was called at the first time) and simply decide whether it should be with true or false to simple? This way we avoid two calls to collectPropertyMatches.
  • The code of isOptimizable and areCorrespondingPropertiesMandatory are good!
• Other
  • To avoid the bloat of the false flag anywhere, can you overload the method to make the last flag false?

• Some places like if( that should be rewritten as if (
• Update copyright year

Committed on 9317a rev.15629 the changes needed based on the code review.
For the PreselectQuery change, Alex is right. I included all the logic in hasJoinWithSubselect and used that in PreselectQuery and also in QueryComponent.getWhere.
I also tested the changes with the suggested testcases and on a bigger customer application and it works as expected.
Alex, please review. Thank you!

I went ahead and started testing the changes, on a big customer application I tested regression tests and the results are ok. I will continue with unit tests.

Overall, I would want to refactor a bit the javadoc/parameter and method names to reflect the join with a FIRST/LAST scenario. We used the "subselect" term to reflect that such construct will convert in an SQL subselect. However, this is confusing as it can also refer to CAN-FIND that may be embedded as a sub-select. Lets find another keyword for it - lets say "joinWithSubselect".

This should still be assessed

hasJoin = !comp.isJoinWithSubselect();

This is the only bit of code I am uncertain ATM. I think it should be tested more in depth with some debugging - maybe an example with OF keyword to trigger a join?

Otherwise, the changes are good, but should be thoroughly tested.

Trying to create a test using the OF keyword, I stumbled upon another bug that might need fixing.
With the schema detailed in #9317-3, for this testcase:

DEFINE QUERY q1 FOR table1, table2.

OPEN QUERY q1 FOR EACH table2 OF table1 WHERE table1.pk = table2.fk NO-LOCK.

GET FIRST q1.
IF AVAILABLE table1 THEN
    DISPLAY table1.pk table2.fk WITH FRAME f1.

CLOSE QUERY q1.

I think 4GL shows a compile error, right? In that case, FWD shouldn't be fixed as it presumes correct 4GL input.
For OF, you need for each table1, each table2 ....

I build the following testcase:

DEFINE QUERY q1 FOR table1, table2.

OPEN QUERY q1 FOR EACH table1,
              FIRST table2 OF table1 WHERE table1.pk = table2.fk NO-LOCK OUTER-JOIN.

GET FIRST q1.
IF AVAILABLE table1 THEN
    DISPLAY table1.pk table2.pk WITH FRAME f1.

CLOSE QUERY q1.

Now the testcase generates the following SQL:

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
left  outer join table2 table2__im1_ on (table2__im1_.pk = table1__im0_.pk and table1__im0_.pk = table2__im1_.fk) 
order by table1__im0_.pk asc, table1__im0_.recid asc 
limit ?

The changes in PreselectQuery are triggered and the SQL query is generated correctly. I simulated additional cases and all seem to work as expected. I think the PreselectQuery changes are good.

In addition, I ran unit tests for a large customer application, and no regressions were found. However, this testing was done before the last commit, so the most recent changes have not yet been validated with unit tests.

• 15630: esthetic changes
• 15631: changes for the outer join optimization to take effect
• 15632: history entries

Review of 9317a:

• if (joinRef != null && !joinRef.equals(ref)) this makes the query matching a bit more relaxed, but mind that FIRST table2 OF table1 WHERE table1.pk = table2.fk NO-LOCK OUTER-JOIN. is a very rare construct. Usually, FIRST table2 OF table1 is enough without "duplicating" the implicit condition. The OF will automatically do tt1.f1 = tt2.f1. I understand that using a WHERE will create two conditions over pk. But I would avoid making this more relaxed as the code that follows doesn't allow multiple logical "joinRef".
  • for instance, the upcoming for (int i = 0; i < parms.length; i++) will search the parms[i] that will match the joinRef. With your changes, there will be 2 theoretical such joinRefs, so two possible refSubstIndex. The code will simply pick the first one as the for-loop is ascending. Making the for-loop descending will yield a different output. This makes the solution buggy as a = b && a = c will behave different than a = c && a = b, although they are theoretically the same.
  • if you need to optimize this now, then more extensive changes are needed. ATM, I prefer to opt out optimizing this now as there is no urge to do so. Please rewrite your test to dismiss the where clause when testing OF.
  • I think this makes the FieldReference change redundant (?).
• Can you provide an example in which hasJoin = !comp.isJoinWithSubselect(); is mandatory?
  • What happens if we don't do this change?
  • Debugging an example with OF, will hasJoin be true or false?
  • How does this affect the FQL?

--¶

OPEN QUERY q1 FOR EACH table1,
              EACH table2 OF table1 NO-LOCK.

• hasJoin = (iteration != FIRST && iteration != LAST); is true - iteration is NEXT
• !comp.isJoinWithSubselect(); is also true (the same reason, the iteration).

This generates:

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ 
where table2__im1_.pk = table1__im0_.pk 
order by table1__im0_.pk asc, table1__im0_.recid asc, table2__im1_.pk asc, table2__im1_.recid asc 
limit 1

--¶

OPEN QUERY q1 FOR EACH table1,
              FIRST table2 OF table1 NO-LOCK.

• hasJoin = (iteration != FIRST && iteration != LAST); is false - iteration is FIRST
• !comp.isJoinWithSubselect(); is also false because getFQLPreprocessor().isSubselectNeeded(); will return true (baseWhere is null so the subselect is needed) ? this requires investigation.
  In FQLPreprocessor.isSubselectNeeded():
  

        if (fql == null)
        {
           return true;
        }
  

This generates:

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ where table2__im1_.recid = ( select table2__im2_.recid as col4_1_ 
                                                            from table2 table2__im2_ 
                                                            where table2__im2_.pk = table1__im0_.pk 
                                                            order by table2__im2_.pk asc, table2__im2_.recid asc 
                                                            limit 1) 
order by table1__im0_.pk asc, table1__im0_.recid asc 
limit 1

--¶

As is the code now, we need the where explicit to see a difference in behaviour with and without the change in PreselectQuery. This is why the previous testcase was built like that.

OPEN QUERY q1 FOR EACH table1,
              FIRST table2 OF table1 WHERE table1.pk = table2.fk NO-LOCK.

• hasJoin = (iteration != FIRST && iteration != LAST); becomes false - iteration is FIRST
  This generates:
  

  select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
  from table1 table1__im0_ 
  cross join table2 table2__im1_ 
  where table1__im0_.pk = table2__im1_.fk order by table1__im0_.pk asc, table1__im0_.recid asc 
  limit 1
  

• hasJoin = !comp.isJoinWithSubselect(); is true because getFQLPreprocessor().isSubselectNeeded(); will return false.
  This generates:
  

  select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
  from table1 table1__im0_ 
  cross join table2 table2__im1_ where table2__im1_.pk = table1__im0_.pk and table1__im0_.pk = table2__im1_.fk 
  order by table1__im0_.pk asc, table1__im0_.recid asc limit 1
  

So here the emitted sql differs and without the changes the where clause coming from the OF clause is not included.
I will test now with this in FQLPreprocessor.isSubselectNeeded():

      if (fql == null)
      {
         return false;
      }

For the case baseWhere == null the subselect is not to be generated. This was overlooked. And now it becomes clear that we need the changes in PreselectQuery.
For this:

OPEN QUERY q1 FOR EACH table1,
              FIRST table2 OF table1 NO-LOCK.

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ 
order by table1__im0_.pk asc, table1__im0_.recid asc 
limit 1

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ 
where table2__im1_.pk = table1__im0_.pk order by table1__im0_.pk asc, table1__im0_.recid asc 
limit 1

..¶

Alexandru Lungu wrote:

So here the emitted sql differs and without the changes the where clause coming from the OF clause is not included.

• So this means that there was a bug in trunk regarding FIRST, OF and WHERE?
• Can you confirm that 4GL is indeed "appending" the OF and that the apparent bug in FWD trunk is not in fact a quirk replication?

To clarify my previous response, all the examples referenced were tested with the 9317a changes (where subselect optimization is active) and only differ by whether the modification in PreselectQuery is applied. I was addressing the question from the last review: Can you provide an example where hasJoin = !comp.isJoinWithSubselect(); is mandatory?

With a clean trunk, the subselect is generated and the query is correct, there is no bug in trunk. The issue occurs in 9317a when the change in PreselectQuery is missing.

For this:

OPEN QUERY q1 FOR EACH table1,
              FIRST table2 OF table1 NO-LOCK.

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ where table2__im1_.recid = ( select table2__im2_.recid as col4_1_ 
                                                            from table2 table2__im2_ 
                                                            where table2__im2_.pk = table1__im0_.pk 
                                                            order by table2__im2_.pk asc, table2__im2_.recid asc 
                                                            limit 1) 
order by table1__im0_.pk asc, table1__im0_.recid asc 
limit 1

select table1__im0_.recid as col0_0_, table2__im1_.recid as id1_, table2__im1_.pk as pk2_0_, table2__im1_.fk as fk3_0_ 
from table1 table1__im0_ 
cross join table2 table2__im1_ 
where table2__im1_.pk = table1__im0_.pk order by table1__im0_.pk asc, table1__im0_.recid asc 
limit 1

..¶

Alexandru Lungu wrote:

• Can you do a test with FOR EACH tt, EACH tt2 OF tt WHERE ... (aka use EACH instead of FIRST, but still keep the OF and WHERE).

This case does not get optimized, neither with the outer join nor with the subselect optimization, in both trunk and 9317a.

..¶

Alexandru Lungu wrote:

!comp.isJoinWithSubselect(); is also false because getFQLPreprocessor().isSubselectNeeded(); will return true (baseWhere is null so the subselect is needed) ? this requires investigation.

Is this something fixed in 9317a or is a remaining TODO? From my understanding is fixed (especially expanded in #9317-23) and seems to be a quite a common pattern, but please re-confirm.

This issue has been fully addressed and committed in the latest 9317a revision.

..¶

Alexandru Lungu wrote:

I am technically OK with 9317a in this state, but I recommend serious testing. As there are not that many different customer applications that follow this specific pattern, I suggest building a unit test suite to cover the examples in #9317 (especially #9317-23 and #9317-22). The unit tests shouldn't test whether the optimization kicks in or not, but in a context in which it does, the resulting query is functionally good.

I will put toghether a test plan and start working on unit tests.

• Ioana[icp]: large customer application unit tests and regression tests - DONE
• Andrei[ai]: large customer Gui application regression tests and smoke test - DONE
• Lorian[ls]/Ioana[icp]: large customer application harness tests - unavailable ATM
• Lorian[ls]/Andrei[ai]/Ioana[icp]: Chui regression tests - DONE
• Lorian[ls]: ETF tests - DONE
• Ioana[icp]: create, convert and test new 4GL unit tests - DONE
• Ioana[icp]: performance tests - DONE

I am rebasing 9317a as we speak, will update when that is done.

Rebasing 9317 cause some issues because of trunk rev. 15656:

revno: 15656 [merge]
author: Chichirau Razvan <rnc@goldencode.com>
committer: Radu-Andrei Apetrii <raa@goldencode.com>
branch nick: trunk
timestamp: Wed 2025-01-22 06:32:01 -0500
message:
  Added logic to honor comparisons between fields and unknown value while using indexes in FIND queries. (Refs: #9337)

      /** Flag indicating if the alias is the first or second operand. */
      final boolean isAliasFirst;

      /** The index of the SUBST node relative to the 'where' clause parameters. */
      final long substIndex;

this.isAliasFirst = false;
this.substIndex = -1;

In the trunk version, collectPropertyMatches defaults them to false and -1 when creating the AstWalkListener, so there shouldn't be any problem.

9317a is rebased, the revisions that need testing: 15679 - 15689.

Ioana-Cristina Prioteasa wrote:

Testing plan for 9317a:

• Andrei[ai]: large customer Gui application regression tests and smoke test

All good.

I enabled SQL logging during a performance test with a threshold of 0, ensuring all queries were logged. The test was conducted across three different scenarios. For each, I performed three warm-up runs and then recorded the queries executed during the fourth test, along with their total execution time.

From these results, I can conclude that 9317a introduces a performance improvement, but it does not fully resolve all the existing outer join optimization issues. Further investigation is needed on #8616 to identify and address the remaining inefficiencies.

What is the timing reflect? The total time for a specific query? What was the query analyzed? Or do you mean all SQL queries in PG?

The time is the total execution time of all queries executed during the test.

Ioana-Cristina Prioteasa wrote:

Testing plan for 9317a:

• Lorian[ls]/Andrei[ai]/Ioana[icp]: Chui regression tests

Chui regression tests passed ✅

Tested 150 performance tests and the average time for a run stands for what I observed in #9317-31.

In a big customer application I have 2 extra passing unit tests. As these changes should not introduce functional difference, I am actively investigating those.
I also wrote 10 new 4gl unit tests that verify the number of results returned by a query and the order of those results. These pass without and with 9317a changes.
I updated the testing plan in #9317-26 to reflect the current status.

ETF passed .

The two additional tests passing are not a direct result of 9317a. The failures occur when the outer join optimization is disabled, while the tests pass in both scenarios where outer join optimization is enabled, with and without 9317a. This suggests that the root cause of these test results lies in #8616 rather than #9317 and further investigation should be conducted there.

The testing plan is now complete. While harness tests are currently unavailable, all other testing steps showed no regressions.
In my opinion, 9317 is ready to be merged.

Ioana-Cristina Prioteasa wrote:

[...]
The testing plan is now complete. While harness tests are currently unavailable, all other testing steps showed no regressions.

Ovidiu is close to making the harness work. Ovidiu, please regression test 9317a with the harness in multi-tenant mode, when you have it working, and report success/failure here.

Committed on the testcases project, rev. 1704 the 10 new 4GL unit tests. These verify the number of results and the order the results are returned in various join queries, including the exampled in #9317-22 and #9317-23.

9317a/15689 passed the MT harness test ✅.