Better if tested against one course having a lot of courses (> 5000). Alternatively, you can hack the $blocksz initial value (5000) to something lower than the total number of courses in the testing site. Also, we can rely on feedback from moodlers for this.

1) Go to any course, settings block -> import

2) You wil get the "Find a course to import data from", showing one list of courses (if there is any you can import from) and one search box.

3) Perform some searches (1-char ones are ok), both by course short-name and full-name.

4) You get up to 10 matching courses. Results are correct.

5) Try some search by non-matching short-name or full-name

6) You get the message "There are no courses to display" and the search box is shown, so a new search can be launched.

Under the 4 DBs (mysql, postgres, mssql, oracle)
Better if tested against one course having a lot of courses (> 5000). Alternatively, you can hack the $blocksz initial value (5000) to something lower than the total number of courses in the testing site. Also, we can rely on feedback from moodlers for this.
1) Go to any course, settings block -> import
2) You wil get the "Find a course to import data from", showing one list of courses (if there is any you can import from) and one search box.
3) Perform some searches (1-char ones are ok), both by course short-name and full-name.
4) You get up to 10 matching courses. Results are correct.
5) Try some search by non-matching short-name or full-name
6) You get the message "There are no courses to display" and the search box is shown, so a new search can be launched.

Description

If a user has the required capabilities to import from some courses, but there exists 250 courses before them that they do not have the capability to import from, the search list on the import page is empty and claims theres no courses to import from, the search box also doesn't appear in this instance.

The problem is this line in 'backup/util/ui/restore_ui_components.php': Line 166:
$resultset = $DB->get_recordset_sql($sql, $params, 0, 250);

The search is clearly hardcoded to only check the first 250 courses. By removing the offset and limit, it properly returns a list of courses the user can import from.

I imagine this was done for performance reasons so we don't end up checking the capabilities across all X number of courses, however simply giving up after a certain number is no good.

I've attached a small patch that simply removes the limit from the query.

Tony Levi
added a comment - 06/Jan/12 1:35 PM Please apply patches, this is still coming up and has been reported several times according to the list of duplicates.
nice github links and rebased to latest 22, 21, 20:
https://github.com/tlevi/moodle/tree/mdl26037
https://github.com/tlevi/moodle/tree/mdl26037_21
https://github.com/tlevi/moodle/tree/mdl26037_20
This has been outstanding for too long, given it is a solved problem.

I know this has been open since ages ago, but each time I arrive here I finish in a dead-end.

1) Current implementation limits the pre-fetched courses to 250, and that's wrong, for sure. But can be workaround by fine-tunning the search.

2) Any attempt to perform the search globally, without any limit, will kill the performance in big sites (thousands of courses), no matter how it's implemented because, at the end... it implies site-wide capability checks and that does not scale ok at all.

So really, I don't know if you're completely sure about 2) (take out limits) being better than 1) (limit and fine-tune the search). Or if using one alternative 3) increase 250 to something under acceptable limits (say 5000, based in Tony's used numbers) will be enough for the majority of the cases (sites), and fine-tunning the search, or going straight to the target course and restoring from it would be good enough.

Perhaps we could do it in chunks, like Tony's patch provides... but providing sort of "next/prev" navigation in the list (to avoid checking all the courses) and they are shown in 10 by 10 increments o so. That will produce, implicitly, that the teacher need to fine-tune the search to avoid having to "navigate" dozens of times before arriving to the target course.

Or perhaps we could order them in some way there are more chances to get the target ones first (like ordering by lastaccess to them, note that this will break alphabetical ordering).

Not sure really about which is the best to get this fixed (to some acceptable behavior) once and for all. Thoughts?

Eloy Lafuente (stronk7)
added a comment - 23/Jan/12 11:16 AM I know this has been open since ages ago, but each time I arrive here I finish in a dead-end.
1) Current implementation limits the pre-fetched courses to 250, and that's wrong, for sure. But can be workaround by fine-tunning the search.
2) Any attempt to perform the search globally, without any limit, will kill the performance in big sites (thousands of courses), no matter how it's implemented because, at the end... it implies site-wide capability checks and that does not scale ok at all.
So really, I don't know if you're completely sure about 2) (take out limits) being better than 1) (limit and fine-tune the search). Or if using one alternative 3) increase 250 to something under acceptable limits (say 5000, based in Tony's used numbers) will be enough for the majority of the cases (sites), and fine-tunning the search, or going straight to the target course and restoring from it would be good enough.
Perhaps we could do it in chunks, like Tony's patch provides... but providing sort of "next/prev" navigation in the list (to avoid checking all the courses) and they are shown in 10 by 10 increments o so. That will produce, implicitly, that the teacher need to fine-tune the search to avoid having to "navigate" dozens of times before arriving to the target course.
Or perhaps we could order them in some way there are more chances to get the target ones first (like ordering by lastaccess to them, note that this will break alphabetical ordering).
Not sure really about which is the best to get this fixed (to some acceptable behavior) once and for all. Thoughts?
Ciao

Because the query only picks a few columns from mdl_course, I think it isn't a big deal because the page isn't (relatively) hit that much. If this search appeared on the front page it would be something different entirely.

For real world cases, we've had this deployed on sites with 50k+ courses for a year; the performance of that particular page might be a little slower (it still comes up in a second or two...) but it is nothing in the scheme of things. I can't tell visibly between with/without the patch.

Tony Levi
added a comment - 23/Jan/12 12:04 PM Because the query only picks a few columns from mdl_course, I think it isn't a big deal because the page isn't (relatively) hit that much. If this search appeared on the front page it would be something different entirely.
For real world cases, we've had this deployed on sites with 50k+ courses for a year; the performance of that particular page might be a little slower (it still comes up in a second or two...) but it is nothing in the scheme of things. I can't tell visibly between with/without the patch.

Tony Levi
added a comment - 23/Jan/12 1:21 PM To word that more directly, I think the above patch should be applied unless someone with a truly colossal site can show that it has a detrimental effect...

I'd be happy to test it but we don't have any 2.x sites over, or even approaching, 50,000 courses to match Tony's testing.

I'm not sure that option 1 is a possibility in all cases. Part of the issue is that when the courses a user does have permission to import from are all located in the database after 250 other courses from which they don't have permission to import, the user is told they can't import from any courses and they aren't given a search box. No search box at all means no fine tuning. If you keep a low limit, then it seems the search box needs to be tweaked so that it still shows up and the error would need to be changed so that it's not telling the user they can't import any courses when really they can, but just not from the first 250.

Since Tony has had the patch running for a year with 50k+ courses and minimal impact, I think that speaks to eliminating the limit or at least increasing it dramatically. Alternatively, make the limit an admin setting. Most sites could disable or raise the limit but any site that is "truly colossal," as Tony said, could leave it enabled with a number that works for them. It's also a lot easier to change an admin setting than it is to re-push code on the chance that someone has an "oops moment" of setting it too high and dragging down their server.

Chris Follin
added a comment - 24/Jan/12 4:51 AM I'd be happy to test it but we don't have any 2.x sites over, or even approaching, 50,000 courses to match Tony's testing.
I'm not sure that option 1 is a possibility in all cases. Part of the issue is that when the courses a user does have permission to import from are all located in the database after 250 other courses from which they don't have permission to import, the user is told they can't import from any courses and they aren't given a search box. No search box at all means no fine tuning. If you keep a low limit, then it seems the search box needs to be tweaked so that it still shows up and the error would need to be changed so that it's not telling the user they can't import any courses when really they can, but just not from the first 250.
Since Tony has had the patch running for a year with 50k+ courses and minimal impact, I think that speaks to eliminating the limit or at least increasing it dramatically. Alternatively, make the limit an admin setting. Most sites could disable or raise the limit but any site that is "truly colossal," as Tony said, could leave it enabled with a number that works for them. It's also a lot easier to change an admin setting than it is to re-push code on the chance that someone has an "oops moment" of setting it too high and dragging down their server.

I've just been looking at this now, Eloy brought it to my attention this morning.
Having looked over the patch and having had a play with it I think that it is a good improvement, even over the performance impacts it may have.
Presently this gets my tentative +0.5 from me.

I do think that a new issue should be opened to properly explore the performance improvements that can be further made to this code:

get_searchsql is including empty searches into the SQL's where. During testing I noticed that postgres is processing these in some shape or form still as performance improved when I removed them e.g. (c.name ILIKE '%%' ESCAPE '').

Explore the memory implications in using get_records rather than get_recordset for the search. I can certainly see why it's been done, and given this is not a commonly hit page I don't think this is urgent.

At the end of the day Eloy you are much more versed in Database performance than I am so final call is yours.
As mentioned a tentative +0.5 from me, perhaps if you are still unsure we should integrate this on master only, create the new issue and investigate it immediately, and then after we are happy with the result on master backport to the stable branches?
Up to you

I should add I only played with this patch on Postgres and as I'm working in HQ this week it was only tested on a server with hundreds of courses, not thousands.
As Tony has been running with this on large sites for a fair period I think it is likely to be safe, but a bit of cross engine stress testing would be nice.
Certainly testing this on a site with several thousand courses, of which the testing user only has the required capability on a single course would be an interesting test for this patch.
I'd be happy to test this however I am off shortly on holiday for 2 weeks and it would have to wait until I am back.
If you like I can try and push someone at HQ into setting up the required engines and sites to give this a proper test.

Sam Hemelryk
added a comment - 24/Jan/12 10:29 AM Hi guys,
I've just been looking at this now, Eloy brought it to my attention this morning.
Having looked over the patch and having had a play with it I think that it is a good improvement, even over the performance impacts it may have.
Presently this gets my tentative +0.5 from me.
I do think that a new issue should be opened to properly explore the performance improvements that can be further made to this code:
get_searchsql is including empty searches into the SQL's where. During testing I noticed that postgres is processing these in some shape or form still as performance improved when I removed them e.g. (c.name ILIKE '%%' ESCAPE ' ').
Explore the memory implications in using get_records rather than get_recordset for the search. I can certainly see why it's been done, and given this is not a commonly hit page I don't think this is urgent.
At the end of the day Eloy you are much more versed in Database performance than I am so final call is yours.
As mentioned a tentative +0.5 from me, perhaps if you are still unsure we should integrate this on master only, create the new issue and investigate it immediately, and then after we are happy with the result on master backport to the stable branches?
Up to you
I should add I only played with this patch on Postgres and as I'm working in HQ this week it was only tested on a server with hundreds of courses, not thousands.
As Tony has been running with this on large sites for a fair period I think it is likely to be safe, but a bit of cross engine stress testing would be nice.
Certainly testing this on a site with several thousand courses, of which the testing user only has the required capability on a single course would be an interesting test for this patch.
I'd be happy to test this however I am off shortly on holiday for 2 weeks and it would have to wait until I am back.
If you like I can try and push someone at HQ into setting up the required engines and sites to give this a proper test.
Cheers
Sam

+1 to giving this a stress test on other engines; we run PG everywhere so if mysql or others run these queries badly we wouldn't know.

The chunking means the memory use is fixed, and only picking a few small columns means it shouldn't be big so the growth really shouldn't be horrible. Not sure what the impact is to maximum use for the page though.

Tony Levi
added a comment - 25/Jan/12 7:58 AM +1 to giving this a stress test on other engines; we run PG everywhere so if mysql or others run these queries badly we wouldn't know.
The chunking means the memory use is fixed, and only picking a few small columns means it shouldn't be big so the growth really shouldn't be horrible. Not sure what the impact is to maximum use for the page though.

We use MySQL. I tested Tony's patch in a development environment (not as powerful as our production fleet) with a copy of a site that has 16,000 courses. I loaded the page numerous times. Observationally with just my eyes, it's the same load time with the patch as it is without the patch. When I actually time it, it ranges from the same to 2 seconds longer.

Chris Follin
added a comment - 01/Feb/12 7:14 AM We use MySQL. I tested Tony's patch in a development environment (not as powerful as our production fleet) with a copy of a site that has 16,000 courses. I loaded the page numerous times. Observationally with just my eyes, it's the same load time with the patch as it is without the patch. When I actually time it, it ranges from the same to 2 seconds longer.

Minor improvements, but calculating the caps array and user once and delegating to has_all_capabilities()

I've tested it against the four BDs (small sites) and seems to be working ok. It would be great if, in the mean time, we can get some feedback about how the final patch is working against your real big sites. TIA!

Eloy Lafuente (stronk7)
added a comment - 27/Feb/12 9:47 AM - edited I'm sending this to integration (for 21_STABLE, 22_STABLE and master). I've added one extra commits on top of Tony's ones that:
Fixes the iteration to work perfectly under mssql (it behaves differently for out-of-result sets).
Makes the code ready for PHP 5.4 (no multi-level continue and break allowed).
Minor improvements, but calculating the caps array and user once and delegating to has_all_capabilities()
I've tested it against the four BDs (small sites) and seems to be working ok. It would be great if, in the mean time, we can get some feedback about how the final patch is working against your real big sites. TIA!
Thanks Tony and everybody, ciao

- Better if tested against one course having a lot of courses (> 5000). Alternatively, you can hack the $blocksz initial value (5000) to something lower than the total number of courses in the testing site. Also, we can rely on feedback from moodlers for this.

1) Go to any course, settings block -> import

2) You wil get the "Find a course to import data from", showing one list of courses (if there is any you can import from) and one search box.

3) Perform some searches (1-char ones are ok), both by course short-name and full-name.

4) You get up to 10 matching courses. Results are correct.

5) Try some search by non-matching short-name or full-name

6) You get the message "There are no courses to display" and the search box is shown, so a new search can be launched.

The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

Eloy Lafuente (stronk7)
added a comment - 01/Mar/12 7:45 PM The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.
TIA and ciao

Michael de Raadt
added a comment - 07/Mar/12 3:18 PM Does this really need to be tested across all four DBs?
Testing candidates are Eloy, Sam and Apu. Eloy and Sam are disqualified and Apu is ill.
I'll see if Dan can help otherwise this might need to wait.

Well, when doing it I tested it under the 4DBs, but with very tiny sites (<10 courses) and it worked, so no SQL-error is expected. In fact I detected some minor problems with mssql and I added 1-extra commit to fix that.

And Tony's solution has been tested both under mysql and postgresql against big sites successfully.

So, under my responsibility... I think I'm going to pass this... if anybody has any objection, tell it in the next 30 mins or shutup forever.

Eloy Lafuente (stronk7)
added a comment - 08/Mar/12 4:39 PM Well, when doing it I tested it under the 4DBs, but with very tiny sites (<10 courses) and it worked, so no SQL-error is expected. In fact I detected some minor problems with mssql and I added 1-extra commit to fix that.
And Tony's solution has been tested both under mysql and postgresql against big sites successfully.
So, under my responsibility... I think I'm going to pass this... if anybody has any objection, tell it in the next 30 mins or shutup forever.
Ciao

Eloy Lafuente (stronk7)
added a comment - 10/Mar/12 5:28 AM Your changes are now upstream and will be included in the next minor released scheduled for March 13th (next Monday!).
icao_reverse('arreis olik rebemevon afla letoh ognat');
Closing, ciao