Moodle

Uhm... that field for user preferences is supposed to be used to store simple values and not serialised info, that can easily grow over 255cc.

Asigning to Nico (and addressing to 1.9.6) as seems that Gradebook is the responsible for this... couldn't we split that info into smaller (ideally individual) parts or so?

Ciao

hmm, I can imagine putting a lot more than 255 there, maybe we could change it in 2.0?

Hey, I just got a report of this at the OU, and luckily I was a good boy and searched before creating a duplicate.

I think that value should be TEXT, like in config and config_plugins.

I also thing that using serialise PHP for this particular pref sucks badly. I think that the gradebook user prefs should be stored in human-readable key-value pairs.

I suppose that means that this bug should probably be split into two. One for changing the column type, which I think should happen in 2.0, and another for fixing gradebook user prefs later.

this issue needs to be fixed, it causes failed sql. We are running the latest 1.9, but we also see that Moodle 2.0 has this issue as well.

Raising this to critical, setting affected versions to 1.9.x and 2.0.x and sending to stable backlog. Needs fix in both branches IMO.

Ciao

If I'm reading the code right there's just the two subfields, so splitting them out shouldn't be a big deal. https://github.com/mackensen/moodle/commit/b7a9176d8ef4374cfb0ccdce8078ec9120c50807 I think does this in 2.0.2+ and applies fairly cleanly against 1.9 as well. Tested on MySQL only but should be db-neutral.

Note, grepping the repository didn't reveal any other obvious cases of serialized preferences.

A triggering event is a click on the category aggregates only icon (box with minus sign) in the grader report (Course > Settings > Grades). When the field is full (has 255 characters) the user receives a 'Error writing to database'.

Presumably the field fills as users switch grade categories to aggregate only. In our system only two faculty out of approximately five hundred have encountered this problem, though. It's possible that another bug is contributing to the problem. Or it's possible that most faculty don't hide grade categories. Or maybe I don't understand how this field is populated.

I've worked around this problem by setting the user_preferences.value field to type 'text'. This sets the field limit to 65k characters.

Hello,

I´ve just realized that this issue also affects 2.1.* versions so I´ve voted for it,

Meanwhile, what can I do to solve it? Can I just extend the column size or this can make some other things fail?

Thanks in advance.

master branch (2.2dev) now contains support for varchar fields up to 1333 chars long

Thanks Petr!

Until 2.2 version comes out could I resize this column manually in the database (up to 1000 chars long, for example) or should I wait?

Hello again,

I'm being pressed to solve this issue ASAP, so I've tried to resize this column manually (up to 500) and realized that this change fixes the issue, but I'm afraid of this manual change causing problems in further moodle upgrades.

Do you suggest me to apply this change on the real moodle site or should I wait?

Thanks.

Ive closed the duplicate bug reports for this issue.

For what it's worth, this happens on MySQL-based systems too (at least on mine, with Moodle 2.1.1+ (20110817) running; I'll be upgrading soon). Is there a safe way to fix the problem without doing an upgrade?

I am having this same problem in several courses. The Moodle version is 2.1.2+ (build 20111012) using MySQL.

Since the problem returns an error to the user and prevents the gradebook from collapsing I have adjusted the mdl_user_preferences.value size to varchar(512). This prevents the error for now. I am also voting for this to be solved.

I'm adding branch information that contains a potential fix for this. I've changed the user_preferences column "value" from varchar to text as is the case in the config table.

If you have manually increased the size of the value column that should be fine. Assuming the fix I have proposed gets accepted you can safely increase the size of the column.

Do not apply the fix I have provided on production servers until after it has been through our review and testing process.

I will raise a separate issue to change the grader report to not store serialized data in the user_preferences table.

I've raised MDL-30668 to look at not storing serialized data in the user_preferences table.

First of all small text is deprecated, new fields should use only big text. Also we should not change varchar to text like this because sql that joins into preference values would break badly (such as our cron and contrib)!

Anyway why do we still use 255 in 2.2 stable?

Ok, so in your opinion am I better off simply increasing the size of the column?

Theres no 2.2 branch yet because it hasnt been through peer review

Hi Andrew,

You asked me to take a look at this yesterday, in fact I've done the lazy thing and had a discussion with Eloy about it (thank you Eloy).
We walked through the different options that we could see and we've ended up settling on a completely different idea to anything purposed here.
There were three options we considered:

1. Changing to text, we didn't get very far with that, it's not bc compatible, surrounding code would need review + upgrade (as Petr mentioned cron for instance), and we'd need to ensure all code used the user pref API. Easiest solution certainly, good for future code, but not great for performance and bc kill it.
2. A two part solution, review the gradebook usage and shrink it's storage in some way. At the same time increase the field size to 1333 for 2.2 and master. < 2.2 max size is 255 still so really the gradebook review + changes are the only leg this stands on. At the same time it doesn't actually solve the problem, that developers will still in the future store large values and even at 1333 chars it'd only be a matter of time before we hit the next issue.
3. Third solution (credit to Eloy here) introduce a new text field in the user pref table for storing large values (lets call it valueoversized) and then altering the uesr prefs API to deal with that. Essentially the getter would look for the first not null value, and the setter would decide where to store the value depending upon the size of it, e.g. value > 255 ? valueoversized : value. With this solution the API handles the madness and code can store anything it wants there (future proof). It's completely backwards compatible (nothing old requires changing) and its safe to join on the table with all fields except the new one. The downsides, there is a new field so performance, however all user prefs are cached in the session after first load so that won't be bad, and the other downside is that as part of the setter we will next textlib so that we can get the size of the value reliably. Of course in 2.2 and master we would still increase value to 1333 so that there was even less chance of using the text field.

Anyway the conclusion that we came to was that introducing the new field and using that for large values as described is the least evil and most compatible solution.
What are everyones thoughts on that?

Cheers
Sam

Ill have to have a think about this...

Option 3 seems like a very invasive change Vs just increasing the size of the existing varchar field however you make an annoyingly good case Adding the new field certainly seems like it would resolve this once and for all.

Just recording some notes on how the valueoversized system could work to make sure I understand.

We increase the length of user_preferences->value to 1333.

check_user_preferences_loaded() currently uses $DB->get_records_menu() which can only handle 2 columns (name, value). It would switch to $DB->get_records() to get 3 columns (name, value, valueoversized). Then we would iterate over the returned array and compress it down to name and value by doing something like...

$user->preference[$name] = !empty($value) ? $value : $valueoversized;

Is there a way to do that that doesn't involve looping over every preference?

set_user_preference() would need to check the length of the value using textlib::strlen(). If its above 1333 (define a new constant for this I guess) it goes into valueoversized and value is set to null. This section of code would need to be altered to deal with both value and valueoversized

if ($preference = $DB->get_record('user_preferences', array('userid'=>$user->id, 'name'=>$name))) {
        if ($preference->value === $value and isset($user->preference[$name]) and $user->preference[$name] === $value) {
            // preference already set to this value
            return true;
        }
        $DB->set_field('user_preferences', 'value', $value, array('id'=>$preference->id));

    } else {
        $preference = new stdClass();
        $preference->userid = $user->id;
        $preference->name   = $name;
        $preference->value  = $value;
        $DB->insert_record('user_preferences', $preference);
    }

Hi Andrew,

That is about the gist of it.
Just noting the field size can only be increased for 2.2+ and master.
I wouldn't use a define for it, I'd just hard code it and comment. Should we ever increase it again we'd need to review use and hopefully we'd find the comment.
As for the iteration in check_user_preferences, I cannot think of any smart way to avoid that, it'd just have to be done.

Perhaps worth waiting for Petr to have a read of these changes and get his thoughts on it. There may be something we have overlooked.

Also worth knowing I just dreamed up the name for the new field, it wasn't discussed, feel free to come up with something better

Cheers
Sam

The valueoversized thing is horrific. That can't possibly be a good idea.

Just changing value to TEXT seems so obvious to me.

What backwards-compability issues are there? They must be less than your complex value + valueoversized change.

And user_pref values are cached in session. I don't believe that there would be performance problems, unless you can show me measurements. Also, if there are performance problems, then there are probably worse performance problems with config_plugins.

I would be happy to just make value CHAR(1333), and then add code to set_user_preferences to throw a clear coding_exception explaining the problem if users try to set a value that is too long. That is simple, and deals with any potential performance issues.

We should probably consider doing the same for config and config_plugins, we can can convince ourselves that is sufficient.

Anyway, -1000000 from my for the valueoversized suggestion. It is totally ridiculous. It has maintenance headache written all over it.

David pointed out that the alleged BC concerns were the ones Petr raised in this comment: http://tracker.moodle.org/browse/MDL-20245?focusedCommentId=133543&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-133543

Searching the code for {user_preferences}, the only match I find is https://github.com/moodle/moodle/blob/master/lib/cronlib.php#L207

That query would, indeed, need one trivial change if we changed the value column to TEXT.

Grepping for user_preferences (without the {}) finds a some more occurrences, but I don't think there are any more causes for concern.

(would be so cool to avoid adjectives like "ridiculous" and avoid to ridiculously abs(vote) > 1)

Regarding the valueoversized (textvalue) idea, it was entirely my idea (but the name, lol), not Sam's or Andrew's one, just to clarify it. And mainly trying to avoid big (current and future) slowdowns in queries handling the "value" column in any where/join clause.

Perhaps, after all, the solution for this should be to keep that serialized information out from user_preferences, and only accept simple/small values there.

ridiculously yours, ciao

Sorry, yes. Inappropriate language. And, since user_prefs are cached in session, probably a good idea to limit field size. I think CHAR(1333) + coding exception is the way to go.

I have implemented the value column max length increase and added a throw new coding_exception() to set_user_preference(). Diff url and branch updated. So is this the solution we're going with?

Are we sure that textlib::strlen($value) > 1333 and 'value too big to fit in CHAR(1333)' mean exactly the same thing in all DBs? (due to the complexities of UTF-8)

... if they are not the same, does it really matter?

Also, are we going to make any attempt to make the grader_report user preferences value more compact - rather than the current, slightly verbose, serialise PHP array format?

Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account.

I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.

Doing some research now on how exactly the various databases interpret "varchar(1333)". Hopefully they all interpret 1333 as the number of characters, not the number of bytes, and correctly take multibyte characters into account.

I've already raised MDL-30668 to deal with the storage of serialized php arrays in the user preferences table. Trying to deal with them as two discrete pieces of work.

That sounds like a good plan. Thanks.

MySql pre 4.1 (released 2004) apparently treats VARCHAR(N) as a number of bytes. All subsequent versions treat it as a number of characters and apparently it copes with UTF-8.

Postgres has apparently treated VARCHAR(N) as number of characters and dealt with utf-8 since forever.

In Oracle you can do either VARCHAR2(N BYTE) or VARCHAR2(N CHAR). Provided we're using CHAR all is well. If not Oracle users will potentially run into problems with a long utf-8 string passing the PHP check but being too long for the column.

SQL Server is complicated and I didn't find a simple answer.

All of the above goes out the window if the database simply isn't configured to use utf-8. You would always get into trouble if you did something like have PHP supply a UTF-8 string to a database configured to be ASCII. A long utf-8 string could pass the check in PHP, be interpreted as single byte characters by the DB and become too long to go into the value column. Not sure if we can do much about that.

This involves a database change so cannot be included in the 2.2 and 2.1 branches. I considered back porting just the addition of the safety check in PHP but then anyone wanting to increase the size of the value column themselves would have both increase the column size and modify the code to match their increased column size.

This cannot be done in 2.1, because varchar(1333) support was only added in 2.2

"And thirdly, the code is more what you'd call 'guidelines' than actual rules." - it is safe to make exactly the same DB change on the stable branch and master, especially when it is an idempotent change like increasing the column width to 1333, so I think we should bend our own rules here and fix this in 2.2 .

I've produced a 2.2 stable version of the fix. Ultimately it's up to the integrators whether they want to integrate the 2.2 version as well as the version for master.

I'm submitting this for integration. Although it doesn't officially have a peer reviewer its certainly had plenty of eyes on it.

Thanks Andrew.

Oki so this is really one "workaround / alleviator" that, exclusively under 22_STABLE and master will allow bigger (1333) values there, leading to "more cases" working, but not all (I mean, if there are, say 200 categories, it will continue breaking, correct? Because 1333 will be exceeded too.

So, the "real" fix will arrive once MDL-30668 is done (changing storage format and/or place). And that will be applied to all supported stable branches. Does that mean that we should "tidy" a bit the subject of this issue to something like "Allow bigger values in user preferences, alleviating some problems with grader preferences" or so.

Finally, note the we are still in the initial "on-sync" weeks between 22_STABLE and master so we cannot apply the patches on their current form (they cause versions to diverge). To solve this, there are 2 possibilities, you pick the preferred one:

1) Amend your patches so both are using the same 2011120500.02 + .01 (right now) version.
2) Send this back to the "awaiting integration" status, so it gets fixed next week, when we are already allowing to diverge versions. Note that this *requires amending too" because the current versions will be outdated.

I'll keep this halted until hearing from you...ciao

Oki, I'm going to:

• add extra commit putting versions on-sync (2011120500.03)
• modify the title of the issue an point everybody to the "real" fix: MDL-30668
• push it to integration, so testing can happen.

Ciao

Two new commits have been added about:

• keep 22_STABLE and master versions on-sync (2011120500.03)
• added 2 assertions to test_set_user_preference() so both the 1333 and 1334 cases are covered. They have been tested agains the 4 DBs

Tested and passed thanks all.

Whoever decided one week was worth 14 days had really one bad idea. Anyway, the nightmare is over, so thanks for your, once again, amazing contributions. Many, many thanks!

Now... disconnect, relax and enjoy the next days, yay!

Closing...ciao