Hi, Anthony.

Could you be more specific about what you mean by "updating a module"? Do you mean configuring an activity module?

How are you sure this is related to course completion?

I mean configuring an activity module.

My assumption was it is related to completion because the error occurs when it's pulling out a list of tracked users (see the error). I assume the query used allows the possibility of multiple results with the same user id (which the unique factor). I did look at the data in the db to see if there was anything that appeared irregular, but couldn't see anything. I can't replicate the error or replicate the query exactly because I don't know some of the params.

Thanks for the additional information, Anthony.

Actually, the errors indicate this is related to activity completion.

We would really need some way to replicate the problem before we can attack it.

Was the activity duplicated? Could you experiment with turning activity completion off and on in the course and see if that has an effect?

Done a bit more testing -

It happens to every activity within one course on the Moodle instance (not on any others that I can locate).

It only happens when an activity is updated (not when it is created).

It only happens if completion tracking is set to 'do not indicate activity completion'.

It doesn't matter whether an activity was duplicated.

Switching off activity completion for the course stops the error appearing, switching it back on makes the error reappear.

We had a problem once with the completion system that has now been resolved, where it was possible there would be duplicate rows which could cause this kind of issue, but that was fixed a while back (I forget which version but it's in OU live for instance) - there is a unique index on userid,coursemoduleid on course_modules_completion.

Is that unique index definitely in place on whatever server this happens on?

Yes - it's there.

The only server where I can see this behaviour is learn2acct.

I'll reassign our Redmine bug to you.

I'll leave this with you, Sam.

By using dbdebug script I was able to identify the query that is causing the problem:

SELECT u.id, u.firstname, u.lastname, u.idnumber 
FROM mdl_user u 
INNER JOIN mdl_role_assignments ra ON ra.userid = u.id 
INNER JOIN mdl_role r ON r.id = ra.roleid 
INNER JOIN mdl_user_enrolments ue ON ue.userid = u.id 
INNER JOIN mdl_enrol e ON e.id = ue.enrolid 
INNER JOIN mdl_course c ON c.id = e.courseid
 WHERE (ra.contextid = 171455 OR ra.contextid IN (1,160501,160502,602509,171455 )) 
 AND c.id = 200182 AND ue.status = 0 AND e.status = 0 AND ue.timestart < 1332506418 
 AND (ue.timeend > 1332506418 OR ue.timeend = 0) AND ra.roleid IN (15,5) 

This query will return multiple entries for the same user, giving an error, if:

• they have more than one role
• or they have more than one enrolment

Not sure if the last is permitted but the first definitely is.

Also I am not sure why we are checking role - seems like enrolment would be sufficient. Seems to me that when moving to 2.x we should probably have ditched the role part and replaced this with standard sql from get_enrolled_sql or whatever that is, rather than custom sql. Looking at it now.

I have now done changes to make the completion system use the standard 'enrolled' SQL instead of some weird custom combination between that and what it used to do before 'enrolled' existed. As the standard enrolled SQL already does a DISTINCT, this also fixes the reported problem.

In addition to the test script above, I also verified that unit tests do not give errors.

I suggest this fix is only applied to 2.3 because:

• Listing users twice (and then dropping the extras in get_records_sql) will not cause any practical problems beyond the warning message.
• The patch doesn't apply (at all) to 2.2 due to whitespace/comment changes.

Hi Sam,

Looks fine to me. Only comment is that you are not using what seems to be the 'standard pattern' for get_enrolled_sql of joining explicitly on the enrolled sql (see for example forum_get_potential_subscribers).

The only reason I mention it is that perhaps it has advantages for some DB engines to write that way (I don't know), or perhaps everyone just copied the same way and you didn't

Thanks Dan. I had a look at the example you pointed to with the join, I see what they're doing but it looks a teeny bit weird to me I'm not aware of a reason why that would be any faster but that may of course just be me not knowing so much about whichever database...

Maybe see what the integration reviewer thinks? If they agree it should be changed, I'll go ahead and change it, not a problem. Hope that's ok, submitting for review now.

Yep fine here to submit for integration.

(note I only mentioned it because its the way used almost universally across codebase)

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

Just reading through comments I see that this is drawing on the get_enrolled_sql use.
Personally I don't have anything against the approach you've taken here Sam, however I'm definitely no DB expert of over to Eloy!

(Forgot to mention everything else looks fine)

Well, the key here is that "IN" is, generally, evil. It's NEVER quicker than the EXISTS/JOIN alternatives, NEVER!

Some RDBMS have them optimized to be converted to one of the laters so differences are not noticeable. But not all.

And, more yet, MySQL sucks with all subqueries (more or less), so always we can, we try the JOIN alternative, that is the one working better under all DBs (comparable to EXISTS in all them but MySQL, and better than IN in al them, but in PostgreSQL and MSSQL).

So I'd say that JOIN is the best candidate without doubt. And IN is evil and should be avoided unless 100% necessary.

That's the thing, basically, afaik, ciao

So, in your changes, for the one simply counting users, you don't need to join anything. It's just the count of the records returned by get_enrolled_sql().

And for the other query, I'd change it to JOIN as commented above.

So I'm reopening this... thanks!

Wow, I completely forgot about this one. Sorry for the delay. Re-submitting.

Based on Eloy's comments I have now changed the code. Unfortunately there is a reason I did this with subqueries before - it's basically because that's the only thing that was possible with the get_enrolled_sql output. So, in order to make this change work, I needed to extend the get_enrolled_sql function so that it can be used in joins. I have also updated the unit tests (and rerun them for both completionlib and accesslib). I redid the manual test script as well.

Dan pointed out to me that I misunderstood Eloy's request - it was not to remove ALL use of subqueries, which I originally coded, but to change it from 'IN (subquery)' to 'FROM (subquery) eu'. Which was a much simpler change. I have updated my branch ready for integration review. There is now no change to accesslib or anything scary.

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

Rebased (no conflicts).

It looks perfect, so I'm about to integrate it.

The only point, commented above, is that I think it's NOT necessary to join with the

table as far as the get_enrolled_sql() query already has that table joined, if I'm not wrong. For the count() operation I mean, obviously for the get_tracked_users() it's necessary in order to fetch user details.

Anyway, that way both methods seem more "paired", so no probs.

Ciao

Integrated (23 only), thanks!

Works Great

Thanks for fixing this Sam.

We could celebrate it today... but better if we perform a bigger party after releasing Moodle 2.3.

Print this message and come to Perth that day, it's valid for one beer, wine, coke or... water, as you wish.

Many thanks for your collaboration!

Ciao

thanks all

Just for the record - I know it looks like it's not, but in fact, I think the join with user table is necessary for the count operation. The reason is that we allow people to pass in a WHERE condition, which refers to fields in the user table. And the SELECT done by get_enrolled_sql does not include all fields of the user table.