Talk:DB layer 2.0

Various Notes (before 20080501)

• AdoDB, AdoDB over PDO, PDO. (my initial +1 to continue using underlying ADOdb).
• Placeholder types: :named or ? (will require PHP parsing under some DBs).
• Multiple connections supported (auto-contained DB object).
• PERF debugging available.
• Logging available (to detect wrong uses).

• Total breakage (all contrib uses)
• Total breakage (current dmllib 1.0 functions definition).
• Development mode (branch with radical replacement, some compatibility layer to minimize breakage - current dmllib instantiating new dmllib + stripping slashes ?)

• Mahara experience. ( it's also worthwhile to note that a long long time ago I also did this for elgg which involved doing the addslashes/stripslashes audit through the code base - we didn't have to do this with mahara as we had placeholders and no slashes right from the start - Penny )
• Interface example.
• dmllib 2.0 tests since the beginning.
• dmllib 2.0 documentation since the beginning.
• Classes implementation order (PG, MySQL, MSSQL, Oracle).

• More ideas.

• Next meeting. TODO: document MDM20080501, AdoDb/PDO decision, Interface + PHP documentation + Docs documentation, define tests to perform. ADDRESS IT for... 20080508 ?

Eloy Lafuente (stronk7) 19:00, 29 April 2008 (CDT)

PDO/ADODB

I think it's interesting to discuss the potential of moving to PDO instead of ADODB but I'm not convinced it needs to be in the same conversation - we should be able to switch out ADODB and use PDO instead with completely no change outside dml - surely nowhere in the code is using $db directly - should always be using execute_sql

Penny Leach

Yup 100% agree, underlying stuff will be completely hidden, so it's a non-priority decision. We'll be able "replace" it more or less easily or, alternatively, create new "Moodle drivers" using different internal stuff. Just prospecting your opinion about PDO. I've done some (basic) research and it seems not to be 100% ready for production under some DBs. But wanted to know if you have some experience with it (or at least you initial feeling).

Eloy Lafuente (stronk7)

Various Notes (before 20080508)

Agenda

• From previous meeting + docs comments:
  • multiple connections supported
  • unit testing intro
  • exercise and lams
  • $mdb or $db, global or static. (see "default $DB object" below)
  • empty array returned instead of false
  • $CFG->dbfamily, 100% out, possible? $CFG->dbtype out?
  • performance profiling
  • logging
  • error/exception handling

• To explain/review/discuss/decide:
  • placeholders research and decision (named vs ?)
  • OOP architecture (see "Interface proposal" below)
  • mdllib 2.0 own testing
  • first timeframe estimation

• Other:
  • review problems (see "Problems" below)
  • ideas and comments
  • next meeting (OOP architecture finished, mysql and postgresql working, XX tests against them (self testing), start collecting changes to search and destroy... with script, implementation details agreement)

Interface proposal

Sample implementation with basic API definition is in MDL-14679

All classes are stored in lib/dml/ directory. The naming convention is dbtype_dblibrary_moodle_database for database classes. For example oci8po_adodb_moodle_database and oci_pdo_moodle_database. There is a new configuration option $CFG->dblibrary which can be adodb, pdo or anything else in case of 3rd party modifications. The $CFG->dbtype is expected to contain internal driver name.

Each database class must fully abstract all operations with database including API for XMLDB editor.

To minimise conversion costs all function names are kept, though the prepared statements need a bit different method parameters:

• $sql parameters need $params array with values, $sql query must not contain any user submitted data - instead use ? or :name parameters, both types are supported, but not both in the same query
• $select parameters must be accompanied by $params too
• $field1, $value1 are replaced by $conditions array - you can have more than one condition in get_records() and more than three in get_record() now

The use of recordsets was changed substantially - they can be used in foreach($rs as $record) directly. Recordset closing should be mandatory now, this might help with performance and memory consumption later. Each library abstraction or database class must define own recorset class with moodle_recordset interface.

While I like the foreach iterators here... this makes me think if that won't cause people to get confused between get_record() and get_recordset() functions. First ones doesn't need closing but second ones yes. Also, while thinking on this... do you think we could profile how many open recordsets are left open per request, debugging error if necessary? That will help developers, for sure. Eloy Lafuente (stronk7) 19:27, 6 May 2008 (CDT)

get_records() and similar functions that return arrays now return empty array if nothing found and false only if error occured, this should not cause any major regressions, all code should be verified during the conversion (proposed by Donal).

Database classes should not depend on $CFG settings - instead supplly them in constructors or use set_property() methods, this will allow us to use the same library for enrolment and auth plugins.

default $DB object

Martin proposed to use global $DB instead of function returning $mdb instance or factory methods. Nicolas confirmed that it should be suitable for unit testing purposes.

sample code:

function xyz($userid, $courseid) {
    global $DB, $CFG;
    if ($records = DB->get_records_sql('SELECT * FROM {$CFG->prefix}abc WHERE userid=? AND courseid=?', array($userid, $courseid))) {
        //...
    }
}

Things like $db->debug = true; are changed to

$DB->set_debug(true);  // sets the database debug var

Implementation steps proposal

done:

1. import latest adodb for PHP5

in patch MDL-14679:

1. implement basic API as abstract moodle_database class and moodle_recordset interface
2. tweak lib/setup.php and related files - place all $DB setup code into one function setup_DB()
3. keep old global $db until everything is converted to new global $DB
4. all dmllib functions that do not accept $sql or $select can be rewritten to use new $DB - move those classes to lib/dmllib_deprecated.php for now - this allows us to run moodle in hybrid mode for testing purposes
5. move all unused dmllib functions to lib/dmllib_removed.php and remove body - this could be useful during the migration phase of contrib
6. implement basic mysql and postgresql classes

todo:

1. test, refactor, rewrite, improve, etc. the new dmllib classes
2. implement classes for all supported backends - oracle, mssql
3. majority of code can be converted to new classes while keeping magic quotes on (we are dealing with numbers mostly), the formslib can already return unslashed data and we can add new parameter to data_submitted() too
4. kill the magic quotes and convert the rest

We could also prepare an experimental mysql pdo driver in parallel - this could help to uncover potential problems in API design or implementation.

What parts of adodb do we use/need

• connect to database, configure and set encoding
• execute sql without result with bound parameters
• get results as recorset with bound parameters
• get results as array with bound parameters (can be emulated)
• transactions - begin, end, rollback
• get table columns information - needed for insert and update data validation (can be partially emulated, in fact this could be nice perf boost to use the data we already have in install.xml's)
• get list of tables
• debugging, logging , version info, etc.

PDO and native drivers in PHP5 offer the same level of functionality (except the legacy mysql driver).

Questions & problems

• Do we still need record cache?
• Unknown parameter types in query parameters - we can find out the correct type of each param for inserts and updates, but in case of other queries it might be a problem :-( we could use optional type hints in parameter names like :numberI, :numberD, :nameS where IDS means int, double, string (like in mysqli driver)

Wouldn't some params->setInt(), setString(), setNull() do the trick? Once more the possible substitution of the plain array of values to one array of objects seems to appear in the discussion... Eloy Lafuente (stronk7) 19:35, 6 May 2008 (CDT)

• casting to (int) in isert_record() - what happens if db returns string with number greater than max int?

What means casting to int? Why we need that? IMO a more serious problem are some big sites raising the max int(10) sequence value (logs tables, backup and restore...). Eloy Lafuente (stronk7) 19:35, 6 May 2008 (CDT)

well, some code (like modedit) uses is_int() on results from insert_report() which was was breaking badly when I thied to remove the (int) casting from insert_record() Petr Škoda (škoďák)

• Do we have to use array_values() on $params array in order to re-key the array when using "?" in query? It looks like pdo needs that [1]
• reading the comments in php manual it seems like we need a big battery of unit tests to verify everything works as expected

100% agree. For XMLDB stuff, for example... I've more that "custom" tests built in the XMLDBEditor. And it only covers a few DDL commands. I guess we'll need at least.... 100 tests for good dmllib testing. Eloy Lafuente (stronk7) 19:39, 7 May 2008 (CDT)

• should we limit persistent connections only for simple scripts like file.php and theme stuff? maybe this could make persistent connections more reliable. it could be implemented as some define before require config.php

I would vote to include that define (CAN_USE_PERSISTENT_CONNECTION) in some files, agree. But will create one $CFG->experimentwithpersistentconnections (config-dist.php) to be able to play with it in the safe side. I remember tons of headaches with persistent connections in Moodle early days (due to drivers implementation, mainly). Eloy Lafuente (stronk7) 19:39, 7 May 2008 (CDT)