Improve LLMS_Database_Query performance when no pagination information is needed

[eri-trabiccolo]

See: https://github.com/gocodebox/lifterlms/pull/928#discussion_r325180248

Basically we can avoid counting the found rows when executing db queries if we don't need pagination information.

Remaining classes:

• [ ] LLMS_Notifications_Query
• [ ] LLMS_Query_Quiz_Attempt
• [ ] LLMS_Query_User_Postmeta

[thomasplevy]

@eri-trabiccolo This made a lot of sense in the comment but now that I'm looking at it I'm not sure what criteria we can use to determine if pagination information is not needed. As the example we could assume that if per_page is 5 or 1 or something we probably don't need pagination but that's a dangerous assumption to make.

I think what you were suggesting is adding a no_paging argument that can be passed in and when no_paging is added then we can exclude the SQL_CALC_FOUND_ROWS from the SELECT.

Is this what you're thinking?

[eri-trabiccolo]

Wait I'm not sure I have clearly explained what I have in mind. In my idea we could instantiate a LLMS_Database_Query (extended) object with some arguments, and one of them could be no_found_rows. LLMS_Database_Query should have no criteria nor has to assume anything. The "controller" (meaning the one which would instantiate an LLMS_Database_Query ) must know whether or not it needs pagination info. An example of queries which doesn't need pagination info: https://github.com/gocodebox/lifterlms/blob/master/includes/abstracts/llms.abstract.post.data.php#L223

In this case we could have:

        $query_args = wp_parse_args(
            $args,
            array(
                'per_page' => 10,
                'types'    => 'all',
                'no_found_rows' => true,
            )
        );

That's what I meant. Is this a little bit more clear? :D

[thomasplevy]

@eri-trabiccolo

Yes, absolutely. In the above example we'd need to modify two methods in the LLMS_Query_User_Postmeta class:

https://github.com/gocodebox/lifterlms/blob/09c3353aaeb2a8785868f2cf1cf67c4146e94068/includes/class.llms.query.user.postmeta.php#L27

This should should set the "no_found_rows" to false by default.

And:

https://github.com/gocodebox/lifterlms/blob/09c3353aaeb2a8785868f2cf1cf67c4146e94068/includes/class.llms.query.user.postmeta.php#L197

This will retrieve the value of no_found_rows and modify the SELECT accordingly.

Am I still following?

[eri-trabiccolo]

Yes you're still following, but I'd say to set the default no_found_rows to false in the abstract class, here: https://github.com/gocodebox/lifterlms/blob/3.36.0/includes/abstracts/abstract.llms.database.query.php#L156

and then modify the method you said: https://github.com/gocodebox/lifterlms/blob/09c3353aaeb2a8785868f2cf1cf67c4146e94068/includes/class.llms.query.user.postmeta.php#L197

and this too: https://github.com/gocodebox/lifterlms/blob/3.36.0/includes/abstracts/abstract.llms.database.query.php#L367

bail if $this->no_found_rows is true.

[thomasplevy]

@eri-trabiccolo All sounds good, do it up.

We should make these changes to any class that extends the abstract

There's a few other ones in core, assignments has one, and I think there's two in the REST plugin

[eri-trabiccolo]

I've pushed a PR for this in #939 BUT I don't consider it complete. I've just added the logic to make the use of no_found_rows query param work. I've opened it so that, if you have time, you can give me your feedback while I'm still working on it.

Anyone interested

This is the WIP branch: https://github.com/eri-trabiccolo/lifterlms/tree/db-query-perf

[eri-trabiccolo]

An example where where this can be useful: When checking if a lesson is complete we don't need to know the total results of the query, so we don't need to perform another query to retrieve the FOUND_ROWS().

In this particular edge case the customer has 1 course with 16 sections and 206 lessons. When displaying the course syllabus widget we perform:

• 16 queries to retrieve all the lessons per each lesson + 16 queries to retrieve the FOUND_ROWS (useless, there's no pagination there) - (we also update the lessons meta cache every 16 times here)
• 206 queries to check if a lesson is complete for the current user + 206 queries to retrieve FOUND_ROWS (useless, there's no pagination there)

HS-122060

(this customer's issue is related to the fact that his hosting provider limits the number of db queries to 72K per hour - of course the structure of the customer's course is the main problem, but we could stil do better).

[thomasplevy]

@eri-trabiccolo how many more occurrences of this do you think there are?

If you have some time can you add a checklist of the remaining items to this issue so we can determine how many more we have to work out and then (eventually) feel confident that we can close this issue?

[eri-trabiccolo]

@thomasplevy I dreaded this moment...

Remaining items to update:

• [ ] LLMS_Notifications_Query
• [ ] LLMS_Query_Quiz_Attempt
• [ ] LLMS_Query_User_Postmeta

[thomasplevy]

@eri-trabiccolo that wasn't so bad, was it?

I added the checklist to the initial issue so we don't have to scroll looking for it in the future

[eri-trabiccolo]

@thomasplevy You never know until you face it.