I'm leaving this ticket open though, because the current code doesn't clean up what was left before.

I didn't make it automatic now: this is potentially destructive. I tested this extensively, but I would like other people to review this patch and post comments, before calling this function from the cron.

To test:
1. have a look at the code : http://cvs.moodle.org/moodle/tag/lib.php?r1=1.43.2.33&r2=1.43.2.34 or in your local copy of a recent checkout of MOODLE_19_STABLE
2. make a backup of your database, add a line "tag_cleanup();" to function tag_cron() (around line 804 in Moodle 1.9), call the cron until you see "Executed tag cron".
3. have a look at your tags, see if everything is cleaner than before

the tag_cleanup() looks nearly perfect IMO, some comments:

1) The first $tags = get_records('tag'): I will delete (can become really BIG) it and perform record_exists() while iterating.
2) The TWO foreach loops. Both are based in some, potentially big too, get_records() call. I will change them to their get_recordset() alternatives, iterating with rs_fetch_next_record() and closing the record set at the end. In general, I think always something can grown over, say, 1000 (perhaps less) records... should be handled by recordsets instead of big arrays.
3) Agree seems to be the type of task not needed to be executed foreach cron invocation. I think you can put that call within the 20% section in cron.php (if ($random100 < 20) )
4) The second foreach loop. I think it can be easily transformed into one simple OUTER join returning all the tags not having instances, something like:

SELECT tg.id
FROM {$CFG->prefix}tag tg
LEFT JOIN{$CFG->prefix}tag_instance ti ON ti.tagid = tg.id
WHERE tg.tagtype = 'default'
AND ti.tagid IS NULL

or, alternatively:

SELECT tg.id
FROM {$CFG->prefix}tag tg
WHERE tg.tagtype = 'default'
AND NOT EXISTS (
SELECT 'x'
FROM {$CFG->prefix}tag_instance ti
WHERE ti.tagid = tg.id)

Not tested! But any of them will avoid the iteration and executing one query for each tag.

Hope this helps, ciao

I think that with this type of changes the tag cleanup could be done in each cron invocation.

I did not test the patch at all because my tag* tables are empty and I am no tag expert

IMO, to keep it working in the 20% section is ok, because with current (solid) API is difficult to have those "orphan" instances anymore (in fact the original bug was about to make the clean as part of the upgrade process, only once, and not in cron).

And then this:

• Also we should always try to use LEFT JOIN instead of "AND NOT EXISTS (SELECT..)" - the time difference can be in orders of magnitudes when dealing with large tables.

Don't agree at all!

I think they are 99.9% the same under all DBs, in fact NOT EXISTS will work better under some circumstances (not indexes in join column, for example). And it's far more readable.

Real differences happen with "AND NOT IN (SELECT)" because the IN clause is executed per-each record!

BUT NOT EXISTS is, as commented, perfect, IMO.

Ciao

Do we have this "NOT IN" and "NOT EXISTS" in coding guide somewhere? Maybe we should add a new wiki page with tips for writing of efficient sql queries

It would be amazingly great to have a guide to writing efficent sql in Moodle, especially with all the JOINs that are in the code. I guess you two should be the ones writing it..