Improve performance of statistics queries

[aolley]

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]

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]

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 <#494 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEGFEV3VLZQUZ4ACM5NWETZMN63PAVCNFSM6AAAAABK37MOW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRXHEYTEMBXGU> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[aolley]

@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]

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]

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 <#494 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAEGFESNLYHF3VFXKQQMXCDZMOYMXAVCNFSM6AAAAABK37MOW6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDEMRYGI4TQOJWGY> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[timhunt]

Thanks. That looks good now. Merged.