Moodle

Implement native Oracle DML driver

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Minor Minor
  • Resolution: Fixed
  • Affects Version/s: 2.0
  • Fix Version/s: 2.0.1
  • Component/s: Database SQL/XMLDB
  • Labels:
    None
  • Database:
    Oracle
  • Affected Branches:
    MOODLE_20_STABLE
  • Fixed Branches:
    MOODLE_20_STABLE

Issue Links

Activity

Hide
Petr Škoda (skodak) added a comment -

I have created basic native driver, there are still some problems though:

1/ enum support - solution could be to remove all enum support from all drivers and use varchars instead
2/ lob and blob support - there are some unknown problems when binding params, tests are failing for now
3/ sql_bitor() and sql_bitxor() do not support bound params - probably we will have to use user functions
4/ sql_ilike() - again probably solvable by user functions
5/ one space hack handling in text fields seems inconsistent - need detailed review
6/ db session not supported - missing locking code, will probably require http://download.oracle.com/docs/cd/B10501_01/appdev.920/a96612/d_lock2.htm#999576
7/ there are several reports in text=varchar comparison problems - needs code review
8/ only oracle 10 supported - will need several workarounds and testing if we want to support 9.2
9/ somebody has to test Oracle with Moodle 2.0dev regularly - there might be many new regressions in the code added during the last year (empty strings, text comparisons, from dual, etc.)

Known problems (oracle 10 only, no extensive testing) can be probably solved in 14 days. The result should be more reliable than to 1.9.x, provided somebody steps up and starts testing/developing/reporting problems related to Oracle databases.

Show
Petr Škoda (skodak) added a comment - I have created basic native driver, there are still some problems though: 1/ enum support - solution could be to remove all enum support from all drivers and use varchars instead 2/ lob and blob support - there are some unknown problems when binding params, tests are failing for now 3/ sql_bitor() and sql_bitxor() do not support bound params - probably we will have to use user functions 4/ sql_ilike() - again probably solvable by user functions 5/ one space hack handling in text fields seems inconsistent - need detailed review 6/ db session not supported - missing locking code, will probably require http://download.oracle.com/docs/cd/B10501_01/appdev.920/a96612/d_lock2.htm#999576 7/ there are several reports in text=varchar comparison problems - needs code review 8/ only oracle 10 supported - will need several workarounds and testing if we want to support 9.2 9/ somebody has to test Oracle with Moodle 2.0dev regularly - there might be many new regressions in the code added during the last year (empty strings, text comparisons, from dual, etc.) Known problems (oracle 10 only, no extensive testing) can be probably solved in 14 days. The result should be more reliable than to 1.9.x, provided somebody steps up and starts testing/developing/reporting problems related to Oracle databases.
Hide
Eloy Lafuente (stronk7) added a comment - - edited

About DIRTY HACK (1-space hack) before 2.0 and 2.0 onwards
(from comments also in the driver: http://cvs.moodle.org/moodle/lib/dml/oci_native_moodle_database.php?view=markup):

BEFORE Moodle 2.0:

// For Oracle DB, empty strings are converted to NULLs in DB
// and this breaks a lot of NOT NULL columns currenty Moodle. In the future it's
// planned to move some of them to NULL, if they must accept empty values and this
// piece of code will become less and less used. But, for now, we need it.
// What we are going to do is to examine all the data being inserted and if it's
// an empty string (NULL for Oracle) and the field is defined as NOT NULL, we'll modify
// such data in the best form possible ("0" for booleans and numbers and " " for the
// rest of strings. It isn't optimal, but the only way to do so.
// In the oppsite, when retrieving records from Oracle, we'll decode " " back to
// empty strings to allow everything to work properly. DIRTY HACK.

Moodle 2.0 ONWARDS:

// Before Moodle 2.0, we only used to apply this DIRTY HACK to NOT NULL columns, as
// stated above, but it causes one problem in NULL columns where both empty strings
// and real NULLs are stored as NULLs, being impossible to diferentiate them when
// being retrieved from DB.
//
// So, starting with Moodle 2.0, we are going to apply the DIRTY HACK to all the
// CHAR/CLOB columns no matter of their nullability. That way, when retrieving
// NULLABLE fields we'll get proper empties and NULLs diferentiated, so we'll be able
// to rely in NULL/empty/content contents without problems, until now that wasn't
// possible at all.
//
// No breackage with old data is expected as long as at the time of writing this
// (20090922) all the current uses of both sql_empty() and sql_isempty() has been
// revised in 2.0 and all them were being performed against NOT NULL columns,
// where nothing has changed (the DIRTY HACK was already being applied).

Summary:
Before Moodle 2.0 we were applying the dirty hack only to NOT NULL columns, in order to be able to store "empties" in those columns (replacing them by 1-space, converted back to empty when retrieving). We weren't applying the hack to NULL columns at all. This was causing the problem of being impossible to differentiate real NULL and empty values in those columns (all them were stored as NULL).

To solve this problem, starting with Moodle 2.0, we are going to apply the dirty hack to NULL columns too. That way we'll have both values (NULL and empty) properly separated in the DB, making NULL columns to work exactly like NOT NULL ones were working before Moodle 2.0 with the extra of differentiating NULLs correctly (that didn't worked in 1.9.x at all).

Until today all the uses of the sql_empty() and sql_isempty() functions that we have done are against NOT NULL column (their behaviour doesn't change at all in 2.0), so nothing will break with the change. Only NEW uses of those functions against NULL columns will return accurate results, properly differentiating NULLs and empties.

At the same time, this makes the HACK itself and the xxx_empty() functions really simpler, because they will be supporting all the char/clob columns, without NULL/NOT NULL distinctions at all.

Note: I've reviewed all those uses in core, just a dozen, and all them are against NOT NULL columns or properly covered with and extra NOT IS NULL condition.

Show
Eloy Lafuente (stronk7) added a comment - - edited About DIRTY HACK (1-space hack) before 2.0 and 2.0 onwards (from comments also in the driver: http://cvs.moodle.org/moodle/lib/dml/oci_native_moodle_database.php?view=markup): BEFORE Moodle 2.0:
// For Oracle DB, empty strings are converted to NULLs in DB
// and this breaks a lot of NOT NULL columns currenty Moodle. In the future it's
// planned to move some of them to NULL, if they must accept empty values and this
// piece of code will become less and less used. But, for now, we need it.
// What we are going to do is to examine all the data being inserted and if it's
// an empty string (NULL for Oracle) and the field is defined as NOT NULL, we'll modify
// such data in the best form possible ("0" for booleans and numbers and " " for the
// rest of strings. It isn't optimal, but the only way to do so.
// In the oppsite, when retrieving records from Oracle, we'll decode " " back to
// empty strings to allow everything to work properly. DIRTY HACK.
Moodle 2.0 ONWARDS:
// Before Moodle 2.0, we only used to apply this DIRTY HACK to NOT NULL columns, as
// stated above, but it causes one problem in NULL columns where both empty strings
// and real NULLs are stored as NULLs, being impossible to diferentiate them when
// being retrieved from DB.
//
// So, starting with Moodle 2.0, we are going to apply the DIRTY HACK to all the
// CHAR/CLOB columns no matter of their nullability. That way, when retrieving
// NULLABLE fields we'll get proper empties and NULLs diferentiated, so we'll be able
// to rely in NULL/empty/content contents without problems, until now that wasn't
// possible at all.
//
// No breackage with old data is expected as long as at the time of writing this
// (20090922) all the current uses of both sql_empty() and sql_isempty() has been
// revised in 2.0 and all them were being performed against NOT NULL columns,
// where nothing has changed (the DIRTY HACK was already being applied).
Summary: Before Moodle 2.0 we were applying the dirty hack only to NOT NULL columns, in order to be able to store "empties" in those columns (replacing them by 1-space, converted back to empty when retrieving). We weren't applying the hack to NULL columns at all. This was causing the problem of being impossible to differentiate real NULL and empty values in those columns (all them were stored as NULL). To solve this problem, starting with Moodle 2.0, we are going to apply the dirty hack to NULL columns too. That way we'll have both values (NULL and empty) properly separated in the DB, making NULL columns to work exactly like NOT NULL ones were working before Moodle 2.0 with the extra of differentiating NULLs correctly (that didn't worked in 1.9.x at all). Until today all the uses of the sql_empty() and sql_isempty() functions that we have done are against NOT NULL column (their behaviour doesn't change at all in 2.0), so nothing will break with the change. Only NEW uses of those functions against NULL columns will return accurate results, properly differentiating NULLs and empties. At the same time, this makes the HACK itself and the xxx_empty() functions really simpler, because they will be supporting all the char/clob columns, without NULL/NOT NULL distinctions at all. Note: I've reviewed all those uses in core, just a dozen, and all them are against NOT NULL columns or properly covered with and extra NOT IS NULL condition.
Hide
Eloy Lafuente (stronk7) added a comment -

So, possible values will be:

For NOT NULL columns (same behaviour than before 2.0):

  • NULL: Not allowed, the column is NOT NULL
  • ' ' (1-whitespace). To represent ANSI empty strings.
  • any content.

For NULL columns (new behaviour in 2.0):

  • NULL: To represent real NULLs
  • ' ' (1-whitespace). To represent ANSI empty strings.
  • any content.

Comments:

  • All those 1-whitespace values are transparently converted back to ANSI empty strings by all the the get_filed/record/set() functions.
  • If SQL needs to check for empties directly... the sql_empty() and sql_isempty() functions must be used to guarantee cross-db handling.
Show
Eloy Lafuente (stronk7) added a comment - So, possible values will be: For NOT NULL columns (same behaviour than before 2.0):
  • NULL: Not allowed, the column is NOT NULL
  • ' ' (1-whitespace). To represent ANSI empty strings.
  • any content.
For NULL columns (new behaviour in 2.0):
  • NULL: To represent real NULLs
  • ' ' (1-whitespace). To represent ANSI empty strings.
  • any content.
Comments:
  • All those 1-whitespace values are transparently converted back to ANSI empty strings by all the the get_filed/record/set() functions.
  • If SQL needs to check for empties directly... the sql_empty() and sql_isempty() functions must be used to guarantee cross-db handling.
Hide
Tim Hunt added a comment -

+1 from me. Best solution available, short of getting Oracle to fix their database

Show
Tim Hunt added a comment - +1 from me. Best solution available, short of getting Oracle to fix their database
Hide
Eloy Lafuente (stronk7) added a comment -

CLOB/BLOB support added
Transactions support added.

Also have committed the dirty hack change commented above in case anybody wants to take a look to it (easily reversible if needed, I hope no )

Ciao

Show
Eloy Lafuente (stronk7) added a comment - CLOB/BLOB support added Transactions support added. Also have committed the dirty hack change commented above in case anybody wants to take a look to it (easily reversible if needed, I hope no ) Ciao
Hide
Eloy Lafuente (stronk7) added a comment -

Modified the sql_generator (base) so, any empty default passed to xmldb stuff will generate the proper "empty" default to be used, aka, oracle will have proper 1-whitespace defaults when xmldb requires it in field specs. The rest will continue having real empties. Rely on $default_for_char property (same value than $DB->sql_empty(), btw).

Show
Eloy Lafuente (stronk7) added a comment - Modified the sql_generator (base) so, any empty default passed to xmldb stuff will generate the proper "empty" default to be used, aka, oracle will have proper 1-whitespace defaults when xmldb requires it in field specs. The rest will continue having real empties. Rely on $default_for_char property (same value than $DB->sql_empty(), btw).
Hide
Eloy Lafuente (stronk7) added a comment -

Just confirmed Oracle temporary tables are always global while the other big-3 use them in a local way (per session). Need to imagine some workaround for this. Tomorrow (hopefully the moodle_temptables thing will help once more, let's see).

Show
Eloy Lafuente (stronk7) added a comment - Just confirmed Oracle temporary tables are always global while the other big-3 use them in a local way (per session). Need to imagine some workaround for this. Tomorrow (hopefully the moodle_temptables thing will help once more, let's see).
Hide
Tim Hunt added a comment -

Limit temp table names to something shorter than the current limit (say 20 chars, instead of 30), and then have the oracle driver use $prefix = $CFG->prefix . $ninerandomcharsfromsessinonid . '_'; as the prefix for temp tables.

Or something like that.

Show
Tim Hunt added a comment - Limit temp table names to something shorter than the current limit (say 20 chars, instead of 30), and then have the oracle driver use $prefix = $CFG->prefix . $ninerandomcharsfromsessinonid . '_'; as the prefix for temp tables. Or something like that.
Hide
Eloy Lafuente (stronk7) added a comment -

temp tables support added by using the moodle_temptables stuff, where we change their names internally. Seems to be working ok (passing tests at least).

Show
Eloy Lafuente (stronk7) added a comment - temp tables support added by using the moodle_temptables stuff, where we change their names internally. Seems to be working ok (passing tests at least).
Hide
Eloy Lafuente (stronk7) added a comment -

Added support for DB locking (via PL/SQL package).

Show
Eloy Lafuente (stronk7) added a comment - Added support for DB locking (via PL/SQL package).
Hide
Eloy Lafuente (stronk7) added a comment -

Allow multiple connections: - fix connection settings and use oci_new_connect()

Show
Eloy Lafuente (stronk7) added a comment - Allow multiple connections: - fix connection settings and use oci_new_connect()
Hide
Petr Škoda (skodak) added a comment -

I am converting to separate bug in order to close the huge meta DB 2.0 issue now. I suppose anybody who wants full Oracle support will have to fix a few core bugs and solve remaining oci driver issues.
Thanks everybody!

Petr Skoda

Show
Petr Škoda (skodak) added a comment - I am converting to separate bug in order to close the huge meta DB 2.0 issue now. I suppose anybody who wants full Oracle support will have to fix a few core bugs and solve remaining oci driver issues. Thanks everybody! Petr Skoda
Hide
Petr Škoda (skodak) added a comment -

Current unittests pass (if you disable statement cache and install PL/SQL packages) with the exception of sloppy NULL hanlding in test_sql_concat().

I suppose this can be closed now, right?

Show
Petr Škoda (skodak) added a comment - Current unittests pass (if you disable statement cache and install PL/SQL packages) with the exception of sloppy NULL hanlding in test_sql_concat(). I suppose this can be closed now, right?
Hide
Petr Škoda (skodak) added a comment -

oh, and of course the Oracle driver fails badly if your locale uses floating point commas instead of points (such as CZ locale on Windows)...

Show
Petr Škoda (skodak) added a comment - oh, and of course the Oracle driver fails badly if your locale uses floating point commas instead of points (such as CZ locale on Windows)...
Hide
Eloy Lafuente (stronk7) added a comment -

Yay! Saw the diagnostics, cool!

Just guessing if we should also look for NLS defined separators and also report them via diagnostics, so the admin will be able to define the proper env vars correctly.

Or alternatively... perhaps we could force that on each connection... uhm.. not sure if there will be any drawback.

Ciao

Show
Eloy Lafuente (stronk7) added a comment - Yay! Saw the diagnostics, cool! Just guessing if we should also look for NLS defined separators and also report them via diagnostics, so the admin will be able to define the proper env vars correctly. Or alternatively... perhaps we could force that on each connection... uhm.. not sure if there will be any drawback. Ciao
Hide
Eloy Lafuente (stronk7) added a comment -

Ping, should be close this?

Show
Eloy Lafuente (stronk7) added a comment - Ping, should be close this?
Hide
Eloy Lafuente (stronk7) added a comment -

I think this can be safely closed, doing!

Show
Eloy Lafuente (stronk7) added a comment - I think this can be safely closed, doing!

People

Dates

  • Created:
    Updated:
    Resolved: