fix for add from pool bug (long loading time) mantis 47724

[mglaubitz]

we let one of our local AI models (GLM 5.2) analyse the problem and create a bug fix. this reduces loading time on ourt exam-test server from 3.6 minutes to 6.2 seconds (!)

[kergomard]

Hi @mglaubitz

Thank you for the PR.

First one question:

• Did a developer look at this PR before it was posted? Who was it, so I can interact with them and so I know who takes responsibility for this code? Please be so kind and only have developers create PR changing code, we need a vis-a-vis to interact with.

What happens if you only add the first of the two changes (the index) provided in this PR: This index seems to be added to improve the existing query (and potentially the new one) as the corresponding fields seem to not show up elsewhere in your changes. It should already improve the situation somewhat. What was the improvement if only this change was applied (see)?

Questions:

• Why does the existence of a range disallow the usage of a two phase query? I can see why it would not be necessary, but no reason to disallow it.
• Structurally this to me seems wrong: It creates a completely separate execution path if a set of conditions is met. I think the correct path would be to create a "vanilla" path when there are no filters or limits. This path can (and probably already should contain two steps). Then the first step is enriched with the data that is needed for a correct filtering and ordering. The first query should then not only retrieve the ids, but all data that can easily be retrieved. The second step should only add the data that is hard to retrieve and it should only do so, if retrieving it was not already necessary to complete the first step.

Change Requests:

• Please remove all comments. They just clutter the PR.
• Please never have two nested function calls on one line.
• Please go through the code and fix unnecessary complexity (e.g. here) and inconsistencies (e.g. that variables are once in curly braces and once not in the previous link, where the use of variables should be removed anyway, but just as an example). All variables in strings should always be in curly braces.
• Please move all table names used to class constants. This is more relevant now, as you are qualifying fields in a separate function "qualifyOrderField()". Please qualify everything not only the order fields. This will lead to a more readable query.

This is just the first round, as far as I can see. I.e. function naming seems to be somewhat lacking, but the correct names will only become clear once we really have a final structure. Once this is done the code should have become more readable and logical, so that we can figure out, what else needs to be done.

Best,
@kergomard

[mglaubitz]

Thanks @kergomard for the detailed review — very helpful. We have restructured the code accordingly and pushed an updated version.

Your questions:

1. Why does the existence of a range disallow the usage of a two phase query?
   You are right, there was no good reason for it. I removed that restriction. The new structure no longer distinguishes between "range set" and "range not set" — both take the same path.
2. Structurally this seems wrong: separate execution path.
   Agreed. I removed the separate loadTwoPhase() / canUseTwoPhaseQuery() branch entirely. There is now a single load() path that always works in two steps:

• Step 1 retrieves all readily available columns (qpl_questions., qpl_qst_type., object_data.title, tst_test_result case, …) together with filtering, ORDER BY and LIMIT/OFFSET. The expensive correlated EXISTS subqueries (feedback/hints/taxonomies) are only included here when they are actually needed — i.e. when a HAVING filter targets them or ORDER BY references one of these computed columns (computedColumnsRequired()).
• Step 2 runs only when step 1 did not already compute these flags. It enriches the small paginated set of question ids with the three EXISTS subqueries via an IN (...) clause.
  This way the "vanilla" path you described is in place: step 1 carries everything that is cheap and needed for correct filtering/ordering, step 2 only adds the hard-to-retrieve data, and only if it was not already necessary to complete step 1. When the computed columns are required in step 1 (HAVING or ORDER BY), step 2 is skipped, yielding the same result as the previous single-phase query.

Your change requests:

1. Comments: Will be removed in a final cleanup commit before the PR is merged. I kept them for now so the structure is easier to follow during review.
2. No two nested function calls on one line: Fixed throughout the touched methods (e.g. backtickField() extracted, $this->db->query()/fetchAssoc() split into separate statements in getTotalRowCount()).
3. Complexity / inconsistencies: Variables in strings are now consistently wrapped in curly braces ({$var}). The backtick logic was extracted into a small backtickField() helper, and qualifyOrderField() was generalised to qualifyField().
4. Table names as class constants: I looked at the existing core codebase and found that table names in query builders are consistently used as string literals (e.g. ilTrQuery, assQuestion, ilObjCourseAccess). The TABLE_NAME constant convention is used for a class's own primary table, not for the multiple JOIN partners of a query builder. I therefore followed the existing convention and kept the literals. If you prefer constants here despite the convention, I am happy to add them — please let me know.
   We'd be grateful for a second round once you've had a chance to look at the updated structure.
   Best,
   Marko & GLM 5.2

[mglaubitz]

n general: I see this PR as an attempt to solve a problem / bug. The goal is not to save money! I am more than happy to fund a code review and I have already contacted a service provider that might still have some free capacity this year.

In addition, I forgot something in my last post:

• the index alone reduces loading time on our installation from 3.6 min to 1.5 min
• I will have a look at the failed unit tests later

All the best,

Marko

[kergomard]

Hi @mglaubitz

Ok, we approaching what I would expect. First a few things:

• Thanks for answering my questions.
• Please add a "WIP" to the PR, if you feel like somebody else should go over a PR or even better make it against their repository, this way it doesn't get on our radar. I only looked at this because I had some funding and I thought the issue was interesting, but I would probably have closed it, if I wouldn't have had the resources, as this clutters our backlog. So again: for the future, make sure you do ask a programmer to provide the PR, if it changes code (clearly, documentation changes are fine).

Now, I think the general structure is starting to become more logical and understandable, I didn't go through the whole thing again, as I think there is one more improvement we should apply:

• You now added a blanket $with_computed, I do not think this is the most efficient way: We only need to add the fields we really need, e.g. if we are neither ordering nor filtering by hints we do not need this, but we might still need the feedback sub-query as we are either ordering or filtering by that.

Some more peanuts:

• I do not like intermediate variables (I know, this is a personal preference, but it is one I hold to) and I would like you to treat variables as immutable. So, e.g. these lines should be merged.
• Please really use class-constants for the table-names. Yes, I know, we do not do this everywhere, but there is a good reason to do so: It makes it easy to find stupid typos as they can only happen in one place and table-names become very easy to change. We started using this pattern in the test in repositories. It should then clearly not be TABLE_NAME, but QUESTION_TABLE_NAME and so on.
• Please do not check for truth, where there is none (e.g. here). Only bools convey truth. I know, this needs more thinking about what you expect, but it makes clear what you are looking for.
• I would think that the failing test is due to the new testing class you introduced.
• I assume the PERFORMANCE_PLAN.md will then simply leave us in the final version.

Best,
@kergomard