Moodle

sending the result from pg 7.4 for review -they are similar, but not the same. I have cleaned it a bit, it was produced by pgAdmin III.

For sometime in the next week

Hi,

This is now done by the moodle/site:accessallgroups capability, normally assigned at course level. Users with this capability should be able to access all groups in a course, otherwise they are stuck to the groups they are assigned to.

Yu

sorry, i was trying to resolve another bug

Hi,

Some difference I have found. The diff file is attached (updated vs fresh), I have added some comments closed in ##comments##. Mostly are int/bigint difference.

type:
int/bigint/smallint/mediumint/tinyint
char/varchar
float/double

comements:
missing

key:
naming all different
some extra keys

this one is a combination
> KEY `mdl_forusubs_use_ix` (`userid`),
> KEY `mdl_forusubs_for_ix` (`forum`)
< KEY `user_forum_idx` (`userid`,`forumid`) ##extra different key##

signed/unsigned:

default:
some defaults are missing

size difference:
> `shared` tinyint(2) unsigned NOT NULL default '0',
236d241
< `shared` int(10) unsigned NOT NULL default '0',

I assue the bigints are the correct? and mdl_index_* are the proper way of indexing now? There's a couple silly keys on primary index in the updated version, I think those can be ignored? I can make the diff file much smaller if you tell me which is the correct one...

Some quick comments:

1) int(10) <--> bigint(10) diferences can be ignored. It was a fault to declare int(10) because the maximum number of digits that can be safely stored to ints is 9. No problem because we won't raise the limit (or we can try to migrate old fields to 10 in the furure).

2) Index naming. All the newly created DB follow rules as specified in http://docs.moodle.org/en/XMLDB_key_and_index_naming
But index names aren't important at all, because we don't use them at all. XMLDB internals are the responsible to find such indexes and drop/create them (and decide their names). So any index with the same characteristics (unique and fields) can be safely ignored in the comparison if the only diference is its name. Absolutely.

3) I've stripped the -NOT NULL default '' - from some TEXT fields. Such list is maintained here: http://docs.moodle.org/en/XMLDB_Problems#NOT_NULL_fields_using_a_DEFAULT_.27.27_clause More yet, after 1.7 final release we'll start to plan the move to "everything nullable will be nullable" strategy. Anyway, if the diferences are in the fields detailed above, yo can ignore them, else we'll have to take a look carefuly.

4) New indexes added. Yep. I detected some Foreign Keys missing in the DB tables and I added them to the XMLDB schema. All those FKs are created, for now, as Indexes, so if the new indexes are "conceptual" FKs. It's ok.

5) If in the process above some index is now out, me should analise if it's being used for anything. If so, we can easisily re-add it (now it should be easy because we are sharing versions and so on and the new XMLDB DDL function allow us to detect if one index exists programatically. Anyway, we can leave this for the end of the comparison.

6) I think I review the uses of the "shared" field (and perhaps I asked?) and it seemed to by one simple flag, so int(10) was, definetively, too much.

7) all the rest of changes, after cleaning the above points, will be the critical ones.ok?

Hi,

This is a more readable version after getting rid of everything we don't need to worry about. Most of the differences left are extra indexes, but some could potentially be mistakes (e.g. different length). In some cases I am not sure the unsigned is justified, so please review them too. As per our discussion yesterday there some fields coming with default values for the fresh install.

We have a good internal script that helps in this... we used it a lot for the 1.6 release. Here

http://moodle.cvs.sourceforge.net/moodle/contrib/devtools/sqlcleanup.pl?revision=1.1&view=markup

what we do

• install 1.6 and upgrade to 1.7 on db A

• install 1.7 in db B

• run mysqldump --schema-only on both

• sqlcleanup.pl < schema-a.sql > schema-a-clean.sql
  sqlcleanup.pl < schema-b.sql > schema-b-clean.sql

• diff -u schema-a.sql schema-b.sql

Luke - do you think you can have a go at trying this with MySQL & Pg with sqlcleanup.pl?

Ok. I've done one more iteration over the diff file:

I've checked all the differences between indexes and ALL them are logic "foreign key indexes". I added to the XMLDB stuff because it was quite obvious that they were missing.

So, I've spplited the diff file into two files:

• diff_indexes.txt: Where all those new indexes differences reside. Somewhere in HEAD we should try to create all those missing indexes. I've created MDL-7357 to solve that problem. Feel free to watch/comment it. It isn't a problem for 1.7 IMHO

• diff.txt: One new file only with the remaining differences. Going to analyse them right now.

(Martín L: We have performed 2-3 iterations over these diff files until now, reducing them a lot. Right now it seems that we have the "important" list of differences well detected. If your script can be used to confirm/find more information, great (absolutely)!! Until then, we'll continue working with current diff file as source).

Ok. And one more iteration performed (the last one, I hope).

Now differences are organised in this way:

• diff_default.txt: Where all the differences about default values reside.
• diff_indexes.txt: Where all those new indexes differences reside
• diff_unsigned: Where all the differences about signs reside.
• diff.txt: One new file only with the remaining differences.

1-About DEFAULTS:

All the occurrences found are about integer fields (and 1 varchar) not following the Moodle standards, where every integer field has one default value. In the new schema those defaults have been, simply, enabled. If code was working before 1.7 this shouldn't do any difference. So I propose to leave them unmodified in the XMLDB schema (because are more correct there). NO ACTIONS TO PERFORM.

2-About UNSIGNED:

Practically all (but one) the occurrences found are integer fields that hadn't the unsigned property enabled in previous Moodle versions. If code was working, this shouldn't do any difference UNLESS they used to store negative values (although I think I checked that some months ago). So, I would propose to CHECK IF ALL THOSE FIELDS ALWAYA CONTAIN POSITIVE NUMBERS. If some NEGATIVE is found, then we'll need to undo the conversion to unsigned. Else, nothing to do.

The only exception to all these fields are those in the chat module. They should be, clearly, UNISIGNED and current XMLDB doesn't do that. I think it isn't critical at all (is difficult to raise limit in the chatid, userid, groupid fields!) and I would propose to make the change only in HEAD.

3-About INDEXES:

As said before, the proposal is to solve this properly somewhere in HEAD. MDL-7357 is the task for this issue. So NO ACTIONS TO PERFORM.

4-About REMAINING DIFFS:

There are only 4. Lets see them:

a)- shared: It's a flag value and it hasn't sense to store it in one int(10) at all. IMO, XMLDB is correct.
b)- defaultrole: Seems to be a MISTAKE in XMLDB although it shouldn't be critical because the default value ensures the same contents for the field, no matter if it's NULL or NOT NULL.
c)entrycomment: Currently glossary comments allow empty values to be entered. I think this is the cause for this field transformation although I forgot to apply such change in 1-6>1.7
d)-teachergraded: In the SQL files, it's declared as int(3) while somewhere in the upgrade it was transformed to int(4). I played with it and, if I'm not wrong it was just one 0/1 flag....

So, from the 4 remaining diffs above I'll propose to apply b) to XMLDB schema (17_STABLE), leaving a), c) and d) unmodified.

And that's all. Any comment about anything (related with this issue, better :-P) will be welcome. Tomorrow will be the fix day for this.

Ciao

4/ remaining diffs

a) rssclient/shared - I agree it is a flag
b) looking at the code the defaultrole should work be ok in any case due to failsafe code in get_default_course_role()
c) glossary - I have already made entrycomment required form field in 1.8 last week (it is checked both client and server-side), I am not sure about proper solution for 1.7
d) workshop/teachergraded - this is IMO flag

Great!

So then, for 4-About REMAINING DIFFS plans should be:

Apply b) to 17_STABLE and HEAD (only to XML files, not need to upgrade only, only a few NEW Beta servers will be affected)

Apply c) to HEAD (proper upgrade + XML) as such field is mandatory in that release.

Hi there. MartinL has put me to work on this one.
So far, using postgres, the upgrade process is failing to complete. It's getting stuck on the first screen of the upgrade process. I've got more work to do on finding out why, but I thought I'd better report this anyway.
As soon as I get the upgrade to complete, I'll look at our (Catalyst) diff script.

Patches infinite loop occuring on upgrade from 1.6 stable

Now, perhaps I'm working with the wrong versions, I wonder.. However, I found the cause of the infinite loop on upgrade from 1.6 clean to 1.7. I've attached the patch to this issue. I'm sorry if this is the wrong place to deal with this – I'm new to the Moodle community.
Now to get to work with the diff script....

Wow (I've upgrade my test - really small - server here without problems). If necessary, plz, fill one new bug to track it properly.

Hi Luke,

it seems that you aren't running 1.7 latest. If I'm not wrong such $rs->MoveNext(); is in CVS since time ago....

Thanks for your report, anyway, and welcome to the crazy & amazing world of the Moodle Community! :-P

I'm glad to hear that's fixed already. I must speak with MartinL when he returns from Argentina, I must have mistaken which of our internal branches corresponds with the correct 1.7 version.
Cheers,

• Luke

Summary:

Differences are organised in this way:

• diff_default.txt: Where all the differences about default values reside.
• diff_indexes.txt: Where all those new indexes differences reside
• diff_unsigned: Where all the differences about signs reside.
• diff.txt: One new file only with the remaining differences.

1-About DEFAULTS:

All the occurrences found are about integer fields (and 1 varchar) not following the Moodle standards, where every integer field has one default value. In the new schema those defaults have been, simply, enabled. If code was working before 1.7 this shouldn't do any difference. So I propose to leave them unmodified in the XMLDB schema (because are more correct there). NO ACTIONS TO PERFORM.

2-About UNSIGNED:

Practically all (but one) the occurrences found are integer fields that hadn't the unsigned property enabled in previous Moodle versions. If code was working, this shouldn't do any difference UNLESS they used to store negative values (although I think I checked that some months ago). So, I would propose to CHECK IF ALL THOSE FIELDS ALWAYA CONTAIN POSITIVE NUMBERS. If some NEGATIVE is found, then we'll need to undo the conversion to unsigned. Else, nothing to do.

The only exception to all these fields are those in the chat module. They should be, clearly, UNISIGNED and current XMLDB doesn't do that. I think it isn't critical at all (is difficult to raise limit in the chatid, userid, groupid fields!) and I would propose to make the change only in HEAD.

3-About INDEXES:

As said before, the proposal is to solve this properly somewhere in HEAD. MDL-7357 is the task for this issue. So NO ACTIONS TO PERFORM.

4-About REMAINING DIFFS:

There are only 4. Lets see them:

a)- shared: It's a flag value and it hasn't sense to store it in one int(10) at all. IMO, XMLDB is correct.
b)- defaultrole: Seems to be a MISTAKE in XMLDB although it shouldn't be critical because the default value ensures the same contents for the field, no matter if it's NULL or NOT NULL.
c)entrycomment: Currently glossary comments allow empty values to be entered. I think this is the cause for this field transformation although I forgot to apply such change in 1-6>1.7
d)-teachergraded: In the SQL files, it's declared as int(3) while somewhere in the upgrade it was transformed to int(4). I played with it and, if I'm not wrong it was just one 0/1 flag....

Proposal:
Apply b) to 17_STABLE and HEAD (only to XML files, not need to upgrade only, only a few NEW Beta servers will be affected)
Apply c) to HEAD (proper upgrade + XML) as such field is mandatory in that release.

Yes/nop? Any objection?

Uhm.

just talking with Penny about some problem in the journal upgrade.....this question arrived to my mind:

The "entrycomment" field above (4.c) is about WHAT module? I assumed it was the glossary one, but that seems to be one correct NOT NULL? So, it should be another "entrycomment", Yu?

Eloy,

thanks for the comment. I threw my notes and script in case it was useful – if it's not useful right now, then bah! the diffs look reasonable (though they have no context :-/ diff -u?).

In the meantime, I am trying to suss out why Luke got off to a bad start with a bogus checkout :-/

Oki, after some more checks, it seems that the infamous "entrycomment" field in the diff file, definitively was the journal one.

The matter is that such field was NULL in 1.6 (and called "comment"). In the 1.6 --> 1.7 upgrade, I renamed it to "entrycomment" (reserved word), but, in the corresponding table_column() call I forgot to specify the "null" parameter, so the field was created as NOT NULL.

Obviously this is breaking the upgrade of the module with null comments.

It has been really difficult to find it because the "direction" of the < and > simbols in the diff file seemed to indicate that the field was originally NOT NULL and get transformed to NULL, when the problem is exactly the opposite.

Right now Penny and Luke are patching both 1.7 and HEAD to in order to get that table_column() function call called properly, with the "null" parameter, and testing the migration on one big site that was breaking before.

So, once they fix that in stable and head, the only remaining points to check/do will be:

2-About UNSIGNED. Check all the fields that now have the unsigned flag are, always, positive.
4.b-Decide if we change the field to NOT NULL (only in XML files).

That's all.

Bad Penny, I should have written something in my cvs commit message about this bug.
Anyway that bad table_column call for journal is patched now in 1.7 and HEAD.

So.. the pg upgrade completed successfully

Luke making another diff (unified!) this diff is upgradeddb -> cleandb

This is the diff file created after exporting db's and using the sqlcleanup script.
So far, all the diffs seem to be covered by Eloy's comments.

Hi ML,

aren't you in Argentina? Please, avoid posting silly messages in serious sites! :-D :-P :-D

Thanks for all, absolutely, you know! B-)

Closing this now. Thanks to everybody.

I checked out 1.7.2+ and 1.6.5+ from CVS just now, and then upgraded the 1.6.5+ to 1.7.2+ also. Then I dumped both databases (MySQL) and produced a diff between them. In my case none of the column types had been "upgraded" to the bigger equivalents in the upgraded (migrated from 1.6.5+) 1.7.2+, but in the fresh installation they had been. I'm re-opening this bug because of that. BTW, the same thing happens if I upgrade to 1.8.1+, but as this bug was about 1.7 and was marked as fixed, here is the latest diff between the fresh and upgraded databases.

The diff produced based on 1.6.5+ upgraded to 1.7.2+ and a fresh 1.7.2+.

Dissecting the diff file a bit, for a future reference (I'm not saying that all of these should be cleaned up). The updated 1.7.2+ has the following differences (versus a fresh installation):

• the int --> bigint (and similar) conversion is not done

• on purpose I believe, but could it be added to avoid confusion and to give the upgraded 1.7 the same max limits than a fresh installation?

• lot of missing 'unsigned' definitions

• also mostly just lowers (about halves) the max limit

• lot of missing table comments

• trivial

• most index names have been changed but remain the same in the updated version

• most probably a won't-fix, just cosmetic

• updated db is missing column adodb_logsql.id (PK) and adodb_logsql table comments

• missing indexes for the columns:

• mdl_backup_log.courseid
• mdl_block_instance.blockid
• mdl_block_pinned.blockid
• mdl_course_categories.parent
• mdl_data_ratings.recordid
• mdl_data_records.dataid
• mdl_forum_queue.discussionid
• mdl_lesson_attempts.answerid
• mdl_lesson_branch.userid
• mdl_lesson_branch.lessonid
• mdl_lesson_branch.pageid
• mdl_lesson_high_scores.userid
• mdl_lesson_high_scores.lessonid
• mdl_lesson_timer.userid
• mdl_lesson_timer.lessonid
• mdl_question.parent
• mdl_question_categories.parent
• mdl_question_datasets.question
• mdl_question_datasets.datasetdefinition
• the above two are as a joined index in both fresh and updated, the single ones are missing from the updated one
• mdl_question_rqp_servers.typeid
• mdl_question_sessions.attemptid
• mdl_question_sessions.questionid
• mdl_question_sessions.newest
• mdl_question_sessions.newgraded
• mdl_quiz_question_versions.quiz
• mdl_quiz_question_versions.oldquestion
• mdl_quiz_question_versions.newquestion
• mdl_quiz_question_versions.originalquestion
• mdl_role_capabilities.capability
• mdl_wiki_locks.wikiid (in MySQL at least it's an extra one in 1.7 fresh anyway as there is index ('wikiid', 'pagename') also
• mdl_wiki_pages.wiki
• mdl_workshop_rubrics.workshopid
• mdl_workshop_stockcomments.workshopid

• index for the 'id' column defined twice in:

• mdl_course_categories
• mdl_course_display
• mdl_course_modules
• mdl_event
• mdl_forum
• mdl_forum_subscriptions
• mdl_groups
• mdl_groups_members
• mdl_resource
• mdl_scorm
• mdl_scorm_scoes
• mdl_survey_analysis
• mdl_survey_answers
• mdl_user
• mdl_user_preferences

• mdl_role has two indexes for 'sortorder'

• mdl_sessions2.expireref has a default of NULL (updated) vs. '' (fresh)

• in mdl_stats_daily, mdl_stats_monthly and mdl_stats_weekly tables some columns have defaults as '0' in the fresh one but no defaults in the updated one

A quick scan would show (didn't investigate every bit) that these differences are in 1.8.1+ fresh vs. 1.8.1+ upgraded from 1.6.5+ also. Furthermore, the updated 1.8.1+ has two extra tables (mdl_groups_members_temp and mdl_groups_temp, required by the upgrade only?) and COLLATE information that the fresh 1.8.1+ doesn't have (1.7.2+ had the COLLATE info both for fresh and updated). I also missed a duplicated 'id' column for mdl_modules.

Sorry but I don't know why this is reopen.

The diff file sent by Samul (thanks!) is, exactly, the same sort of work we did originally.

We closed this bug because some minor "TYPE" differences weren't important at all.

And the missing indexes thing, that shouldn't be critical (as long as such missing indexes belongs to non-critical things existing BEFORE 1.7) are going to be fixed by MDL-7357

I've tried to explain in http://moodle.org/mod/forum/discuss.php?d=74041 but, for sure, I haven't been able to do it properly. Summarizing, there are two problems:

1) Changes of TYPE (INT-->BIGINT). We decided time ago about that. The change was performed only to accommodate theoretical maximums better.

2) Missing indexes (shouldn't be critical because only affect places where they didn't exist). We decided to create one utility to detect and to create them.

Any comment? Ciao

I re-opened it, a bit hastily I admit. The reason was the original bug info "compare ... that every table/field is exactly equal" and I just saw that they are far from equal and was too hasty to think that it's not fixed. Sorry! Close this bug again if needed!

But, still, about the theoretical maximums: now the upgraded and fresh installations of 1.7 and later have different theoretical maximums, is that intentional? I think that they should, for clarity, have the same maximum limits (upgraded/migrated or fresh, all the same).

I have to run the sql clean up script, that fix a lot of that for us.

About the missing indexes, I might misunderstand something, but: yes, things were working before 1.7 just fine without those. But now both the upgraded and fresh installations are in the 1.7 land, and the new stuff might/will be pouring in. Now the difference is that the upgraded one is not prepared for that but the fresh installation is?

The double indexes for the id columns generate (only a little bit) some extra data in the indexes and slows down (not much I believe) inserts to those tables.

I would somehow be more comfortable not having such a big number of db differences between the upgraded one and a fresh one... If the differences (like the new indexes) make no difference in the functionality at all then they feel like a bit redundant?

Hi,

no problem. Always it's better to discuss this sort of things and have a clear execution path. So, let's go:

• About fields with different specs:

There are a lot of fields that, before 1.7 were declared as INT(10) and this was a bit inexact, because hard limit of INT fields doesn't allow all the 10 digit numbers to be stored. And all the Primary Keys, Time fields and related were using such slightly incorrect INT(10) declaration. So we decided to make the new ones (from 1.7) more correct, so we declared them as BIGINT(10) that is the correct type able to store any 10 digit numbers. (http://docs.moodle.org/en/Development:XMLDB_column_types)

But we decided not to upgrade old declarations because:

1) The maximum allowed with the old declaration is 4294967295, one number enough big difficult to be raised by PKs and times.
2) Changing the PK declaration is really complex under some DBs (it implies droping the key, moving the column, copying and recreating).

So, the different declaration of INT <-->BIGINT shouldn't be a problem at all. It was implemented the new way because it's the correct one. Nothing else.

• About missing (and duplicated) indexes:

Also, while building all the new XMLDB files we detected that there were a lot of "logic" (under the relational model) indexes that were missing previously. So we decided to declare them in the new schema. This way any fresh Moodle >= 1.7 installation will be 100% ok from an index perspective.

This election, obviously, left old Moodle < 1.7 installlations running with some missing indexes. And that was the cause to create MDL-7357 . Once that script is available, every Moodle site will be able to run it, looking for missing indexes. And they will end running with exactly the same indexes than a fresh install. We simply delegated that task to a lower priority because it shouldn't be critical for performance at all.

About duplicated indexes I've seen them in some servers but I'm confident they aren't an effect of Moodle 1.7. I think they were due to some duplication in previous upgrades and so on. But nothing related with 1.7 or 1.8, IMO. I agree that they will slow inserts and others a bit but shouldn't be a big problem.

• Conclusions:

So, I would propose, numbering tasks to do this:

1) Close this bug because the INT<-->BIGINT differences seem to be controlled and free of problems (I agree I would be better to have everything 100% the same, absolutely, but I don't recommend to try it).
2) Start work on MDL-7357 to allow every site to check and fix its indexes. (personally I think I'll be back in some days - have been 99% out last weeks - so I will be able to start with that).
3) About duplicated indexes, perhaps they could be detected too, informing users about them. But I would suggest manual deletion (and only under MySQL).
4) Test one "alternative path" to allow any site running "old" definitions to be migrated easily over a "new" structure. I think that something like upgrade to 1.8, export, create new 1.8, import should be able to produce a new fresh site with everything in order and old data working perfectly. Only for skilled admins...

Please approve/discuss every point above... thanks and ciao

Hi, any comment here? I've just marked as resolved MDL-7357 both under 18_STABLE and HEAD. Any feedback will be really welcome. After that, the proposed roadmap for closing this is:

1) Close this bug because the INT<-->BIGINT differences seem to be controlled and free of problems (I agree I would be better to have everything 100% the same, absolutely, but I don't recommend to try it).

2) Done. Missing indexes. MDL-7357

3) About duplicated indexes, perhaps they could be detected too, informing users about them. But I would suggest manual deletion (and only under MySQL).

4) Test one "alternative path" to allow any site running "old" definitions to be migrated easily over a "new" structure. I think that something like upgrade to 1.8, export, create new 1.8, import should be able to produce a new fresh site with everything in order and old data working perfectly. Only for skilled admins...

Please approve/discuss every point above... thanks and ciao

Technically I think you can always use 4) as an option but how do we "test" and what do we use to test this?

Well,

as said before, I really think that the INT<-->BIGINT differences aren't going to be meaningful at all (really difficult yo raise the INT limits).

But some people could want to have their old, upgraded, DBs running with all those BIGINTs in order.

So the proposal should be to test if the following path woks:

1) Create one new Moodle 1.8 installation (BIGINTs will be created)
2) Export the data in the old, upgraded installation (that contains INTs).
3) Import all that data into the new installation created in 1)
4) Check that everything works (specially the autonumeric options).

If looks like a viable path to follow, document it somewhere in Moodle Docs, remarking that it's not mandatory and only for skilled admins wanting to be 100% on sync with the latest DB schema.

Something like that is what I was thinking when wrote 4) originally.

Anyway, I'm not sure if all this is necessary at all and, perhaps, we could close this definitively without performing 4), it's just a proposal, not critical at all IMO, so you decide, HQ!

Ciao

This is an SQL script that will alter the database of a freshly installed Moodle 1.6 that just upgraded to 1.7. The resulting database will be identical in structure to a fresh install of 1.7.

Shall I close this now? Loads of diffs have been submitted, but nothing much seems to happen about it. I assume this task has outlived its usefulness.

Perhaps this task is done, however I think the idea is still an important one. One of my primary functions as the maintainer of Moodle for our business, is to ensure database consistency between upgrades. Every time I find small but potentially significant differences: null vs not null, defaults, field sizes, etc. Until database consistency is completely automated, developers need a reminder to check the upgrade process.