search

studentquiz / moodle-mod_studentquiz

Moodle-Plugin
GNU General Public License v3.0
38 stars 38 forks source link

Improve performance of statistics queries #494

Closed aolley closed 3 months ago

aolley commented 3 months ago

Without this, the many sub-queries in stats calculations that JOIN the question_references table dont hit any of the indexes on that table. For sites with large question_references tables - this can be horrible for performance as it ends up doing a seqscan on that table.

By adding the usingcontextid field, we instead get indexscan's.

timhunt commented 3 months ago

Thank you for doing this. I should have noticed this was necessary before. Very important to hit this index. However, since this is all about query performance ...

These queries all process data from a single student quiz. Therefore, there will only every be a single contextid involved, and therefore there is no need to join the context table! Just add AND qr.usingcontextid = :contextid4 in the join on question_references, and add the extra query parameters in get_attempt_stat_joins_params. (Not my code that the params and the SQL are initialised so far apart, but now is probably not the time to fix that.)

Are you able to make this change? (If not, let me know.) Thanks.

aolley commented 3 months ago

Good catch, you're absolutely right. I'll make that change.

On Mon, 15 July 2024, 5:33 pm Tim Hunt, @.***> wrote:

Thank you for doing this. I should have noticed this was necessary before. Very important to hit this index. However, since this is all about query performance ...

These queries all process data from a single student quiz. Therefore, there will only every be a single contextid involved, and therefore there is no need to join the context table! Just add AND qr.usingcontextid = :contextid4 in the join on question_references, and add the extra query parameters in get_attempt_stat_joins_params. (Not my code that the params and the SQL are initialised so far apart, but now is probably not the time to fix that.)

Are you able to make this change? (If not, let me know.) Thanks.

— Reply to this email directly, view it on GitHub https://github.com/studentquiz/moodle-mod_studentquiz/pull/494#issuecomment-2227912075, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEGFEV3VLZQUZ4ACM5NWETZMN63PAVCNFSM6AAAAABK37MOW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRXHEYTEMBXGU . You are receiving this because you authored the thread.Message ID: @.***>

aolley commented 3 months ago

@timhunt I've updated the change to instead pass the contextid to where its needed and include it in the queries. I've done some quick testing so far and it looks like it does what it needs to do.

timhunt commented 3 months ago

Thanks. That is better.

It is a trade-off between changing all thost function calls, and just repeating $contextid = context_module::instance($cmid)->id in the various functions. Contexts are cached, so repeating that is not a performance issue. Is it very annoying if I ask you to change it again?

aolley commented 3 months ago

Well it's a function call, cached or not. Which is less performant. But hardly worth the complexity in the call changes, so sure why not. I'll update it soon.

On Mon, 15 July 2024, 9:11 pm Tim Hunt, @.***> wrote:

Thanks. That is better.

It is a trade-off between changing all thost function calls, and just repeating $contextid = context_module::instance($cmid)->id in the various functions. Contexts are cached, so repeating that is not a performance issue. Is it very annoying if I ask you to change it again?

— Reply to this email directly, view it on GitHub https://github.com/studentquiz/moodle-mod_studentquiz/pull/494#issuecomment-2228298966, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAEGFESNLYHF3VFXKQQMXCDZMOYMXAVCNFSM6AAAAABK37MOW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRYGI4TQOJWGY . You are receiving this because you authored the thread.Message ID: @.***>

timhunt commented 3 months ago

Thanks. That looks good now. Merged.