Moodle

Improve some queries in accesslib

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.9.2, 2.0
  • Fix Version/s: 1.9.3, 2.0
  • Component/s: Libraries, Roles / Access
  • Labels:
    None

Description

Review some queries in accesslib, switching from outer joins to unions, checking cross-db.

Reference: http://moodle.org/mod/forum/discuss.php?d=104137

Activity

Hide
Eloy Lafuente (stronk7) added a comment -

Well, finally I've ended with the attached patch for 19_STABLE, where a 3-UNION query is performed.

// - courses where user has an explicit enrolment
// - courses that have an override (any status) on that capability
// - categories where user has the rights (granted status) on that capability

The 2 first are executed always, while the 3rd is only added if we have previously detected that capability in any category, so sites not using category assignments will save it.

I think it respect the original behaviour of the heavy OUTER JOIN query, retrieving exactly the same target courses in any situation.

Now it's time to measure if it really produces any improvement, both in sites having and not having role assignments at category level.

Any feedback will be welcome. Thanks for spotting this!

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Well, finally I've ended with the attached patch for 19_STABLE, where a 3-UNION query is performed. // - courses where user has an explicit enrolment // - courses that have an override (any status) on that capability // - categories where user has the rights (granted status) on that capability The 2 first are executed always, while the 3rd is only added if we have previously detected that capability in any category, so sites not using category assignments will save it. I think it respect the original behaviour of the heavy OUTER JOIN query, retrieving exactly the same target courses in any situation. Now it's time to measure if it really produces any improvement, both in sites having and not having role assignments at category level. Any feedback will be welcome. Thanks for spotting this! Ciao
Hide
Penny Leach added a comment -

for those of us who discovered colordiff and now can't live without it:

http://paste.dollyfish.net.nz/56a07f

Show
Penny Leach added a comment - for those of us who discovered colordiff and now can't live without it: http://paste.dollyfish.net.nz/56a07f
Hide
Sam Marshall added a comment -

This fails on my Postgres (8.1.9) - it's the problem I referred to, sorting union queries doesn't 'just work'. When I unioned queries I ended up doing it by column number ('order by 2 ASC' in this case).

SELECT c.id,c.sortorder,c.shortname,c.idnumber,
ctx.id AS ctxid, ctx.path AS ctxpath,
ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel,
cc.path AS categorypath
FROM mdl_course c
JOIN mdl_course_categories cc
ON c.category=cc.id
JOIN mdl_context ctx
ON (c.id=ctx.instanceid AND ctx.contextlevel=50)
JOIN mdl_role_assignments ra
ON (ra.contextid=ctx.id AND ra.userid=120612)
UNION
SELECT c.id,c.sortorder,c.shortname,c.idnumber,
ctx.id AS ctxid, ctx.path AS ctxpath,
ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel,
cc.path AS categorypath
FROM mdl_course c
JOIN mdl_course_categories cc
ON c.category=cc.id
JOIN mdl_context ctx
ON (c.id=ctx.instanceid AND ctx.contextlevel=50)
JOIN mdl_role_capabilities rc
ON (rc.contextid=ctx.id AND (rc.capability='moodle/course:view' )) ORDER BY c.sortorder ASC

ERROR: missing FROM-clause entry for table "c"

Show
Sam Marshall added a comment - This fails on my Postgres (8.1.9) - it's the problem I referred to, sorting union queries doesn't 'just work'. When I unioned queries I ended up doing it by column number ('order by 2 ASC' in this case). SELECT c.id,c.sortorder,c.shortname,c.idnumber, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, cc.path AS categorypath FROM mdl_course c JOIN mdl_course_categories cc ON c.category=cc.id JOIN mdl_context ctx ON (c.id=ctx.instanceid AND ctx.contextlevel=50) JOIN mdl_role_assignments ra ON (ra.contextid=ctx.id AND ra.userid=120612) UNION SELECT c.id,c.sortorder,c.shortname,c.idnumber, ctx.id AS ctxid, ctx.path AS ctxpath, ctx.depth AS ctxdepth, ctx.contextlevel AS ctxlevel, cc.path AS categorypath FROM mdl_course c JOIN mdl_course_categories cc ON c.category=cc.id JOIN mdl_context ctx ON (c.id=ctx.instanceid AND ctx.contextlevel=50) JOIN mdl_role_capabilities rc ON (rc.contextid=ctx.id AND (rc.capability='moodle/course:view' )) ORDER BY c.sortorder ASC ERROR: missing FROM-clause entry for table "c"
Hide
Sam Marshall added a comment -

Here's the test script I'm using in case maybe I am doing something wrong, but I don't think so...

http://moodle.pastebin.com/d606f216c

I am running against a read-only copy (few weeks old but whatever) of our live database, so will be able to give timing info when it works.

Show
Sam Marshall added a comment - Here's the test script I'm using in case maybe I am doing something wrong, but I don't think so... http://moodle.pastebin.com/d606f216c I am running against a read-only copy (few weeks old but whatever) of our live database, so will be able to give timing info when it works.
Hide
Eloy Lafuente (stronk7) added a comment -

Attaching new (v2) patch that addresses some problems to provide cross-db when using UNION queries:

  • ORDER BY problems under PG.
  • Duplicate column names under all DBs.
  • TEXT columns in UNIONs under MSSQL and Oracle

To make this work under all DBs I'm using one intermediate "inline_view" that only get target courses and then that's joined with the course table to get all the requested $coursefields.

I've tested the query against all DBs and seems to be working now. It should be slightly slower than v1 (because of the extra join with courses table), but should remain noticeable quicker than current implementation.

Any feedback will be welcome. Ciao

Show
Eloy Lafuente (stronk7) added a comment - Attaching new (v2) patch that addresses some problems to provide cross-db when using UNION queries:
  • ORDER BY problems under PG.
  • Duplicate column names under all DBs.
  • TEXT columns in UNIONs under MSSQL and Oracle
To make this work under all DBs I'm using one intermediate "inline_view" that only get target courses and then that's joined with the course table to get all the requested $coursefields. I've tested the query against all DBs and seems to be working now. It should be slightly slower than v1 (because of the extra join with courses table), but should remain noticeable quicker than current implementation. Any feedback will be welcome. Ciao
Hide
Robert Russo added a comment -

We've integrated your patch in our dev environment and it works wonders.

It is about 20% slower than previous solutions, but behaves as expected for category roles and hidden roles.

I want to thank everyone (Sam and Eloy) for helping me find the error in my logic. We were just going to write a block for category roles (we only have 2) to "show category courses", but now Moodle's /my page behaves as it did from the beginning, but 80 times faster.

After I saw Sam's two UNION sql, I started working on a three UNION query, but Eloy beat me to it.

We had the original patch working by removing the order by, but v2 is the total solution for us thus far.

Now the /my page and the course_list block are usable for large installations.

We are moving this to production in the next hour or so and will let you know how it works performance-wise with real users.

Show
Robert Russo added a comment - We've integrated your patch in our dev environment and it works wonders. It is about 20% slower than previous solutions, but behaves as expected for category roles and hidden roles. I want to thank everyone (Sam and Eloy) for helping me find the error in my logic. We were just going to write a block for category roles (we only have 2) to "show category courses", but now Moodle's /my page behaves as it did from the beginning, but 80 times faster. After I saw Sam's two UNION sql, I started working on a three UNION query, but Eloy beat me to it. We had the original patch working by removing the order by, but v2 is the total solution for us thus far. Now the /my page and the course_list block are usable for large installations. We are moving this to production in the next hour or so and will let you know how it works performance-wise with real users.
Hide
Eloy Lafuente (stronk7) added a comment -

The patch has been applied to CVS (19_STABLE and HEAD). Resolving this as fixed. Thanks everybody.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - The patch has been applied to CVS (19_STABLE and HEAD). Resolving this as fixed. Thanks everybody. Ciao

Dates

  • Created:
    Updated:
    Resolved: