Moodle

Uhm... in theory all the places using the get_recordXXXX() family of functions include one automatic transformation of whitespaces back to empty strings when information is retrieved to balance the default stored whitespaces.

Perhaps that value is not being retrieved using one of that functions but another one "harcoded" ExecuteQuery() or similar... do you know where exactly is that value retrieved from DB?

Anyway I'll take a look to it. Thanks.

If I've done the analysis right, this is from course/enrol.php (line 30), which uses $course retrieved at line 22 in that same file, using get_record ('course', 'id', $id). This in turn uses get_record_sql (line 395 in lib/dmllib.php) where I can see the hack a few lines below the call to get_recordset_sql().

I don't have direct access to the server where this Moodle/Oracle installation is running, but I'll get in touch with the person in charge and see if we can debug this further.

So far we've used trim() in enrol/enrol.class.php to overcome the issue.

Saludos. Iñaki.

After getting in touch with Mikel Kortabarria, we've finally tracked it down.

We get the error when we call print_my_moodle() in course/lib.php. There is a call to get_my_courses() there, which in turn calls get_recordset(), that calls get_recordset_select() who finally calls get_recordset_sql(). None of theses functions check for the Oracle hack at all, so this is why get the error in enrol/enrol.class.php when calling the factory method.

Saludos. Iñaki.

You got it Iñaki!

We are looking about the best form to save this and to get the "horrible but needed" Oracle hack working in all the places where get_recordsetXXX is being used.

Some alternatives could be (after talking about the issue with MartínL):

• Subclass the ADODB oci8po driver (to perform automatic whitespace2empty conversion. This will leave Moodle code cleaner although we'll need to be carefull when upgrading ADODB.

• Add conditional code (if oracle) to perform OUR whitespace2empty conversion after every $rs->MoveNext() or $rs->FetchNextObj() or $rs->FetchNextObject() function calls.

Will continue......

And... there is one more posibility. To implement and use our own:

• MoveNext($rs), FetchNextObj($rs) and FetchNextObject($rs) to do the job.

Please, vote! (1, 2, 3)

And one more possible solution:

• Create our own MoodleRecordSet class that will contain the retrieved ADOdb resultset and will implement its own MoveNext(), FetchNextObj() and FetchNextObject() functions that, simply, will execute the corresponding ADOdb functions + apply the Oracle whitespace2empty conversion.

So, please, vote (1, 2, 3, 4).

Each one has its pros/cons but we must decide it ASAP, not only because of this exactly bug that could be quickly fixed by (2) but to provide a stable API for recordsets to developers.

I vote Eloy

From my point of view whatever results in the least (or no) changes in module-land. (3?)

Looking forwards, the best solution for a stable future is to make our own MoodleRecordSet class with the API we want.

That probably will look like adodb one, except that no API should contain functions called both FetchNextObj() and FetchNextObject().

To match the moodle coding conventions, I would call the class record_set, with methods move_next (returns true/false), get_record (returns object, or false if past the end) , get_next_record (does move_next, then get_record).

well...

I agree with Tim about the best (and transparent) way to be the 4th alternative, with move_next(), get_record() and get_next_record() available in a natural way, so code like this:

$rs = get_recordset(......);

while ($record = $rs->get_next_record())
....
}

(note that get_next_record() will fetch and advance, instead of advance and fetch)

We need this working both on 1.7 and HEAD to fix the bug. Is it ok? I really think the class will be simple (one mini-wrapper over ADOdb original functions).

Yes/No?

Should I use the third solution instead (create 3 simple functions) without the class? The code will be something like:

$rs = get_recordset(......);

while ($record = get_next_record($rs))
....
}

this is less OOP but also more "independent" (and simpler) than wrapping the whole ADOdb recordset.

Please vote. 2 votes difference will make the decision (with a minimum of 3 votes total).

Ciao Really sleepy now...

1 vote for the functions

+1 here too for that

my +1 for any solution that fixes the problem

Oki. That's enough.

Going for functions and replacing all uses of ADOdb MoveNext(), FetchNextObj() and FetchNextObject().

Functions will be:

move_next($rs) to advance the set.
fetch_record($rs) to get the current record in the set.
fetch_next_record($rs) to get the current record and to advance the set.

(the third function will be the recommended way to iterate, while the 2 first functions will be there for special situations needing to scroll & fetch in different steps)

Ciao

Hi Eloy, sounds good.

Just checked quickly the AdoDB recordset methods. I think we'll also want...

• moveto($rs, $pos) with a couple of shortcuts to go to the first and last of the set. Perhaps we could just have move($rs, $pos) where 'next', 'first' and 'last' are recognised
• countrecords($rs)
• maybe eof($rs) for people used to write loops checking for eof
• maybe a fetch_record_arr() to avoid the (rather costly) conversion to StdClass in situations where we know that we want only arrays. Actually, it can save 2 expensive casts.

Writing thing I realise that we'll take several functionnames that are very usable for other things. Might be a good idea to either prefix them or go back to making it a class so we don't have namespace pollution. So what I am proposing is either

• rs_move_next($rs), rs_countrecords($rs), rs_eof($rs),. etc

• $rs->move_next(), $rs->count() or $rs->countrecords(). $rs->eof()

cheers!

I've been reviewing all uses of recordsets in Moodle and I've detected this:

• $rs->FetchRow()
• $rs->FetchObj()
• $rs->Fields()
• $rs->fields;
• $rs->eof
• $rs->MoveNext()

Then, before coding one line, I've written the simple rstest.php script above with results both for MySQL, PostgreSQL, and MSSQL. All the test return data as objects (casting from array when necessary).

Both FechRow() and direct access to the fields[] array achive maximum performance (2x aprox) both under MySQL and PostgreSQL, while doesn't seem to be really different under MSSQL (note that my MSSQL machine is a really old PC with wireless connection, while the others have been executed in my new Mac locally).

So, my initial conclusions were about to use that speedy functions to use our "moodle resultset functions". But, finally I'm thinking about to use the "slower" FetchNextObj everywhere because:

• It's the recommended way from ADOdb docs.
• It controls the case of the fielnames and it's a must
• By using only the required fields in the recordset declaration instead of the default "*" we can achieve important speed gains, near the speediest functions.

Next step. Define the functions to implement.

The initial list is something like:

• rs_fetch_record($rs) to get the current record in the rs.

• rs_next_record($rs) to advance the rs.

• rs_fetch_next_record($rs) to get the current record and to advance the rs. This should be used all the time!

All those using the slower but recommended FetchObj(), MoveNext() and FetchNextObj() and ADOdb functions.

I'll analyse if these two are really needed or no (I've seen a lot of uses in code and perhaps it could be necessary, or perhaps no, by restricting to the field in the query instead and not here):

• rs_fetch_field($rs, $fieldname) to get the specified field from the current record in the rs.

• rs_fetch_next_field($rs, $fieldname) to get the specified field from the current record in the rs and to advance the rs.

And really I wouldn't implement any of this (although it's open for further discussion):

• rs_count_records($rs) because, both MSSQL and Oracle need to iterate over the whole rs to get the number. And, if we need the count we should use one "SELECT count..." IMO.

• rs_eof($rs), because we don't need it at all. With constructions of this type:

while ($rec = rs_fetch_next_record($rs)) {
....
}

• rs_fetch_record_arr($rs), because the key point here, IMO, is memory saved and no speed. And exposing two different alternatives (three if we count the standard get_records()) to do the same task seems a bit confusing. Also any of the two "array" alternatives seems perfect. Direct handling of $rs->fields could present problems with field-names under some DBs and FetchRow() does MoveNext() scroll always if I'm not wrong.

• moveto($rs, $position), because we don't need them for now and because practically all DB "emulate" the absolute move by scrolling back and forward. Also, sets IMO are to be iterated, so let's maintain that.

And this is all, 02:00 here. Going to bed! Feel free to comment anything. I'll start to implement the THREE decided functions tomorrow, with the other TWO being added if needed and the REST remaining open to discuss.

Functions will go both to 1.7 and HEAD. And then, will be used in both versions (to fix the original bug and others).

Ciao

New test script attached. It measures de use of the 2 new rs_fetchXX() functions.

Latest tests under MySQL

Latest tests under PostgreSQL

Now in CVS there are these functions:

• rs_fetch_record($rs): To fetch current record from the set. Returns false if no record is available.
• rs_next_record($rs): To move the pointer in the set to the next record. Returns false if no next record is available.
• rs_fetch_next_record($rs): To fetch current record and to move the pointer to the next record. Returns false if no record is available.
  -rs_close($rs): To close the set. Frees memory and that sort of things (optional but recommended).

All them support the called Oracle DIRTY HACK in order to fix this bug and their friends...

Also, I've performed the original tests once again and results (you can see them above, executed under MySQL and PostgreSQL with 50.000 records) say that:

• The combination of rs_fetch_record() + rs_next_record() is slightly slowly than the initial one using adodb methods (1-2/10 sec for 50.000 records). I think it's ok.
• The preferred way of iteration, rs_fetch_next_record(), using the quicker FetchRow() adodb function with auto-advance, is really fast, with results near-near of the "raw" access to the $rs->fields array.

So, IMO, that's enough. Now going to apply the new functions everywhere (outside from dmllib!!!) where recordsets are being used.

Perhaps somebody with a better English than me could document them: http://docs.moodle.org/en/Development:DML_functions

Ciao

Hi Iñaki,

after all these tasks, I've finally applied the new rs_fetch_next_record() function to the get_my_courses() function, both in 17_STABLE and HEAD, could you test if now such recordsets use to return correct empty strings under Oracle?

TIA (I continue replacing the rest of occurrences)

Hi all,

while the change to new rs_xxx() functions is unfinished, for now I've replaced all the old uses of:

• $rs->FetchRow()
• $rs->FetchObj()
• $rs->FetchField()
• $rs->Fields()

to use the new functions. To do that I've performed small and silly (like me) changes to some important processes like: auth/db, enrol/db, mnet, notification of wrong login/pass, 1.6 upgrade to roles....

While I finish with the rest of the migration to the new rs_xxx() functions it would be great to have some feedback about things working without problems with new code in that areas).

Please, let's test the areas above. TIA and ciao

Done. Now all the occurrences of:

• $rs->FetchRow()
• $rs->FetchObj()
• $rs->FetchField()
• $rs->Fields()
• $rs->fields;
• $rs->fields[
• $rs->eof
• $rs->MoveNext()
• $rs->Close()

have been replaced to use the new recordsets both under 17_STABLE and HEAD. Some exceptions are:

• dmllib.php internals that use raw access to recordsets + dependent db hacks. no problem.
• setuplib.php internals that use raw access to recordsets + dependent db hacks. no problem.
• The install.php script that access to some DB-dependent internals. no problem.
• all the code under /search! Everything there is based on recordsets (wrongly!) and I've decided not to modify that.
• The multilangupgrade.php script that seems to be MySQL dependent and there is no problem with that.

So, now, test, test, test and feedback please!

I'll close this in some days....ciao

We have just upgraded to 1.7.1+ current as of today and the errors are gone away and everything works as expected (w.r.t our original report). There is still the issue of MDL-8164 (that we have to patch by hand on every upgrade).

Thank you very much for your fixed Eloy

Saludos. Iñaki.

Cool. Thanks for the feedback Iñaki (remember me to have some beer next Moot!). I'm going to fight against MDL-8164 right now!

Closing this now. Everything seems to work (incredible!) :-P