Improve performance of statistics queries #494
Conversation
|
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. |
|
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:
***@***.***>
|
|
@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. |
|
Thanks. That is better. It is a trade-off between changing all thost function calls, and just repeating |
|
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:
***@***.***>
|
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.
|
Thanks. That looks good now. Merged. |
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.