oops the regexp got a little mangled in the report - the r and n should be \r and \n (hope it works this time)

From Eloy Lafuente (stronk7 at moodle.org) Monday, 7 August 2006, 04:51 PM:

Well and wow,

the list of columns seems to be really high (>120 columns). I never have understood why all those columns are NOT NULL and have one '' default applied to them. More yet, I'm pretty sure that, under Oracle, empty strings are considered NULL so, such schema won't work at all for the incoming 1.7 (note that this can be applied also to char/varchar columns.

The quick solution seems to be to unset all those defaults from such columns. To do it, me must be absolutely sure that the code doesn't rely on that defaults at all. Tim?

But we need to a long and definitive solution. IMO We have one excess of NOT NULL and DEFAULTS in DB. Only true NOT NULL fields should be defined as NOT NULL and only true DEFAULTS (non-empty) should be defined.

What if we implement the quick solution for 1.6 and I, in my XMLDB schema simply DELETE all those char/varchar/clob/blob DEFAULTS, leaving the field nullable? Any potential drawback with this approach?

(I'm beginning to build XMLDB files today so we should solve this ASAP, please).

Ciao

From Eloy Lafuente (stronk7 at moodle.org) Monday, 7 August 2006, 05:49 PM:

Well, it seems that all those:

char/varchar/text/blob NOT NULL default ''

columns shouldn't be there at all. If something is defined as not null it shouldn't contain an empty string at all but be defined as NULLABLE.

Trying to imagine a good reason for all this excess of NOT NULL columns, perhaps we added the horrible DEFAULT clause to them in order to make the insert_record() function to work properly or something similar (I remember me asking Yu about that).

Anyway, if we are going to continue using that horrible approach, perhaps this could be a good path:

1.- Don't modify 1.6 at all. Everything should continue working properly.

2.- In the XMLDB files, for 1.7, continue defining all those fields with the current form (i.e. NOT NULL DEFAULT '').

3a.- In the XMLDB generator (create table), modify the DEFAULT clause to use ' ' (whitespace) instead of '' (empty string) for Oracle and, potentially MSSQL.

3b.- In the XMLDB generator (create table), avoid the NOT NULL and DEFAULT clause for all those fields for Oracle and, potentially MSSQL.

4.- Add some check in the installation script to look for that MySQL modes being problematic.

About the decision (3a or 3b), 3a seems easy and the created DB will be more similar to the current MySQL and PostgreSQL but I really think that it's a wrong habit completely. 3b will generate a slightly different DB, but a more realistic one (where NOT NULLs are real NOT NULLs rules. This alternative, potentially will break some places (or not if the code is ok and don't rely on that '' default values.

With the time (Moodle 2.0), we could modify MySQL and PostgreSQL columns to be also NULLABLE, deleting the awful default field.

Obviously, I prefer 3b. Ciao

From Tim Hunt (T.J.Hunt at open.ac.uk) Monday, 7 August 2006, 06:14 PM:

The Moodle coding guidelines say that every column must have a default and be not-null. http://docs.moodle.org/en/Coding. Actually, no it doesn't. I sure it used to require not-null. Now it only says must have a default.

I think that '' and NULL are very different values, and in some situations it is helpful to distinguish them.

I thought you must be wrong about Oracle, but you are right: http://www.adp-gmbh.ch/ora/misc/null.html. How very strange.

I agree that we don't touch 1.6.

And I am not a big fan of all those default not-null columns, so if we could get away from them, I would be happy. But since I don't really understand why they were like that in the first place, I can't answer your question about whether it is safe to change them.

From Eloy Lafuente (stronk7 at moodle.org) Monday, 7 August 2006, 08:04 PM:

Well, I've documented this problem and the solution to implement here:

http://docs.moodle.org/en/XMLDB_Problems#NOT_NULL_fields_using_a_DEFAULT_.27.27_clause

Feel free to discuss it because it's being implemented (XML files) now.

From Martin Langhoff (martin at catalyst.net.nz) Tuesday, 8 August 2006, 04:49 AM:

IIRC, the strategy of using not-null columns with a default that the app layer effectively considers null is (I think) an artifact of apps that used early MySQL releases. Before v4.x, MySQL could not put null entries in its indexes.

This change is going to cause a barrage of warnings because there will be nulls in the arrayish objects we fetch from the DB, and we are not checking empty($rec->foo) before use. I think that will be a lot of work and a large delta (lots of tiny changes all over the codebase) that will make merges hard.

And still I think we should do it

I've reviewed all the install.xml files plus all the upgrade.php and, since 1.7, there isn't any TEXT field containing DEFAULT clauses in Moodle, that was the original cause for this bug.

Going to perform one more test setting my MySQL server in strict mode but I think it'll work (unless another strict thing was causing problems).

I've enabled STRICT_ALL_TABLES in my server and then I've upgraded from 1.7 to 1.8 without problems at all. Also I've installed one 1.8 version from scratch and everything ran ok.

Edited: The upgrade from 1.7 to 1.8 has worked perfectly but the new install no because there is one remaining change in 18_STABLE (I had it applied in my local server). I'll backport it tomorrow.

Also, as part of the migration to 1.9, the DEFAULT will be dropped for sites having it.

Feedback will be appreciated. Closing in some days... thanks Samuli and ciao

So I've patched a bit that xmldb class so, any field created (both at install and upgrade) will be checked against this combination. Also, a debugging message will be showed and the problems will be auto-fixed (by setting the default for that field to null).

Ciao