Moodle
  1. Moodle
  2. MDL-28080

Error on Oracle DB operations caused by placeholders longer than 28 chars

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.3, 2.1.1, 2.2
    • Fix Version/s: 2.0.5, 2.1.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Environment:
      Oracle 11
    • Database:
      Oracle
    • Testing Instructions:
      Hide

      TEST1:

      1) Using Oracle and 21_STABLE, create some multiple choice question, verify that "partially correct feedback" is created and updated ok.
      2) Using Oracle, import questions from another course. Must work without any "identifier too long" error.

      TEST2:

      1) Install Moodle 2.0.x under Oracle. Create at least one course with some multiple choice questions.
      2) Upgrade to Moodle 2.1.x. Must work without error.

      TEST3:

      1) Run the DB functional test against mysql, pgsql, mssql, sqlsrv and oracle. Confirm that no failures happen in test_tweak_param_names() or test_fix_sql_params().

      TEST4:

      1. Using Oracle, Create a database table with a column whose name is 30 char, as allowed by our coding guidelines.
      2. Use $DB->insert_record to try to insert a row. Should work without error.

      Show
      TEST1: 1) Using Oracle and 21_STABLE, create some multiple choice question, verify that "partially correct feedback" is created and updated ok. 2) Using Oracle, import questions from another course. Must work without any "identifier too long" error. TEST2: 1) Install Moodle 2.0.x under Oracle. Create at least one course with some multiple choice questions. 2) Upgrade to Moodle 2.1.x. Must work without error. TEST3: 1) Run the DB functional test against mysql, pgsql, mssql, sqlsrv and oracle. Confirm that no failures happen in test_tweak_param_names() or test_fix_sql_params(). TEST4: 1. Using Oracle, Create a database table with a column whose name is 30 char, as allowed by our coding guidelines. 2. Use $DB->insert_record to try to insert a row. Should work without error.
    • Workaround:
      Hide

      Our Moodle admin suspects that the identifier labeled "o_partiallycorrectfeedbackformat" is too long for Oracle. He commented out the following line:

      //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback['format'];

      within the file:

      /question/type/multichoice/questiontype.php

      And this allows us to create and import multiple choice questions in our Moodle 2.0.3 installation.

      The side effect is that the field titled "For any partially correct response" is ignored. For now we are patching our 2.0.3 installation with that line commented out and informing our faculty participants not to use that field when creating quiz questions.

      Show
      Our Moodle admin suspects that the identifier labeled "o_partiallycorrectfeedbackformat" is too long for Oracle. He commented out the following line: //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback ['format'] ; within the file: /question/type/multichoice/questiontype.php And this allows us to create and import multiple choice questions in our Moodle 2.0.3 installation. The side effect is that the field titled "For any partially correct response" is ignored. For now we are patching our 2.0.3 installation with that line commented out and informing our faculty participants not to use that field when creating quiz questions.
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_22_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE
    • Pull from Repository:
    • Pull Master Branch:
    • Rank:
      17764

      Description

      On our local install of Moodle 2.0.3, with Oracle 11 as the database server, creating a simple multiple choice question leads to a critical database/PHP error, leaving the question in a broken state. This same error occurs when trying to import questions from another course (e.g. from a 2.0.2 installation), or when trying to publish questions using Respondus.

      According to the stack trace, the error results from a dml_write_exception thrown from /lib/dml/moodle_database.php. In addition, the debug info seems to implicate an identifier as being too long – but it's not clear which identifier is causing the problem. See testing instructions below, stack trace is as follows:

      Debug info: ORA-00972: identifier is too long
      UPDATE m_question_multichoice SET question = :o_question,correctfeedback = :o_correctfeedback,partiallycorrectfeedback = :o_partiallycorrectfeedback,incorrectfeedback = :o_incorrectfeedback,answers = :o_answers,single = :o_single,answernumbering = :o_answernumbering,shuffleanswers = :o_shuffleanswers,correctfeedbackformat = :o_correctfeedbackformat,partiallycorrectfeedbackformat = :o_partiallycorrectfeedbackformat,incorrectfeedbackformat = :o_incorrectfeedbackformat WHERE id=:o_id
      [array (
      'o_question' => 729,
      'o_correctfeedback' => '',
      'o_partiallycorrectfeedback' => '',
      'o_incorrectfeedback' => '',
      'o_answers' => '2232,2233',
      'o_single' => '1',
      'o_answernumbering' => 'abc',
      'o_shuffleanswers' => '1',
      'o_correctfeedbackformat' => '1',
      'o_partiallycorrectfeedbackformat' => '1',
      'o_incorrectfeedbackformat' => '1',
      'o_id' => 508,
      )]
      Stack trace:
      line 394 of /lib/dml/moodle_database.php: dml_write_exception thrown
      line 268 of /lib/dml/oci_native_moodle_database.php: call to moodle_database->query_end()
      line 1285 of /lib/dml/oci_native_moodle_database.php: call to oci_native_moodle_database->query_end()
      line 1318 of /lib/dml/oci_native_moodle_database.php: call to oci_native_moodle_database->update_record_raw()
      line 134 of /question/type/multichoice/questiontype.php: call to oci_native_moodle_database->update_record()
      line 384 of /question/type/questiontype.php: call to question_multichoice_qtype->save_question_options()
      line 251 of /question/question.php: call to default_questiontype->save_question()

        Issue Links

          Activity

          Hide
          Tim Hunt added a comment -

          Yes, Oracle has a limit of 30 chars on identifies. Still. In the 21st Century. And people pay good money for that sort or rubbish. Why can't the whole world just use Postgres?

          Anyway, we are aware of this limit (http://docs.moodle.org/dev/XMLDB_defining_an_XML_structure#Conventions) and have long had a rule that column names can be at most 30 chars.

          Unfortunately, whoever implemented insert_record for Oracle forgot about this when they decided to prefix the column name with o_ to create the placeholder name. I guess we need to find an alternate strategy for creating placeholder names.

          Sounds like a job for super-Eloy to me.

          Show
          Tim Hunt added a comment - Yes, Oracle has a limit of 30 chars on identifies. Still. In the 21st Century. And people pay good money for that sort or rubbish. Why can't the whole world just use Postgres? Anyway, we are aware of this limit ( http://docs.moodle.org/dev/XMLDB_defining_an_XML_structure#Conventions ) and have long had a rule that column names can be at most 30 chars. Unfortunately, whoever implemented insert_record for Oracle forgot about this when they decided to prefix the column name with o_ to create the placeholder name. I guess we need to find an alternate strategy for creating placeholder names. Sounds like a job for super-Eloy to me.
          Hide
          Longfei Yu added a comment -

          Hey, Tim,

          Thanks for your reply. The weird thing is there was no problem for version 2.0.2. I checked the source code, both of them use the lang "partiallycorrectfeedbackformat", which is really long, thanks.

          Longfei

          Show
          Longfei Yu added a comment - Hey, Tim, Thanks for your reply. The weird thing is there was no problem for version 2.0.2. I checked the source code, both of them use the lang "partiallycorrectfeedbackformat", which is really long, thanks. Longfei
          Hide
          Longfei Yu added a comment -

          Tim,

          I think maybe when it is 28, it also causes problem, since there is prefix "o_" for that.
          Thanks.

          Longfei

          Show
          Longfei Yu added a comment - Tim, I think maybe when it is 28, it also causes problem, since there is prefix "o_" for that. Thanks. Longfei
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ho, just re-updated the title to better represent the problem.

          In the mean time all we can do is to short the placeholder names where this problem is reproducible (I guess there won't be too many).

          Later, perhaps we can:

          1) add some unit tests to reproduce the problem constantly.
          2) try to fix it in some imaginative way.

          Although I'm not 100% sure as far as the same problem will always happen with > 30 chars (no way to change that easily). And current implementation "just" reduce it to > 28 chars (not bad at all).

          Show
          Eloy Lafuente (stronk7) added a comment - Ho, just re-updated the title to better represent the problem. In the mean time all we can do is to short the placeholder names where this problem is reproducible (I guess there won't be too many). Later, perhaps we can: 1) add some unit tests to reproduce the problem constantly. 2) try to fix it in some imaginative way. Although I'm not 100% sure as far as the same problem will always happen with > 30 chars (no way to change that easily). And current implementation "just" reduce it to > 28 chars (not bad at all).
          Hide
          Longfei Yu added a comment -

          Gosh, I just checked, there are 140 tables having the table name length larger than 27 (42 tables have the 30 length table name), even if we are using "M_" for the prefix. That's horrible!

          I also checked, only PARTIALLYCORRECTFEEDBACKFORMAT has the length larger than 25, in our M_QUESTION_MULTICHOICE and M_QUESTION_CALCULATED_OPTIONS.

          Currently, I just commented the line for PARTIALLYCORRECTFEEDBACKFORMAT, so whatever the teacher puts for that field, the code just ignores it. Any better solution for that so far?

          Thanks.
          Longfei

          Show
          Longfei Yu added a comment - Gosh, I just checked, there are 140 tables having the table name length larger than 27 (42 tables have the 30 length table name), even if we are using "M_" for the prefix. That's horrible! I also checked, only PARTIALLYCORRECTFEEDBACKFORMAT has the length larger than 25, in our M_QUESTION_MULTICHOICE and M_QUESTION_CALCULATED_OPTIONS. Currently, I just commented the line for PARTIALLYCORRECTFEEDBACKFORMAT, so whatever the teacher puts for that field, the code just ignores it. Any better solution for that so far? Thanks. Longfei
          Hide
          Tim Hunt added a comment -

          Nothing horrible at all (other than Oracle ). We have good self-documenting names. Do you think we should all still be writing FORTRAN with 6 character variable names?

          Show
          Tim Hunt added a comment - Nothing horrible at all (other than Oracle ). We have good self-documenting names. Do you think we should all still be writing FORTRAN with 6 character variable names?
          Hide
          Longfei Yu added a comment -

          OK, the good thing is nothing is over 30 so far.

          This bug also broke the Respondus question bank import, which is where we intially found this problem.

          Show
          Longfei Yu added a comment - OK, the good thing is nothing is over 30 so far. This bug also broke the Respondus question bank import, which is where we intially found this problem.
          Hide
          Petr Škoda added a comment -

          Oracle is the craziest DB I ever saw that ignores any standards and the rest of the world. But I agree with Eloy that the 28 limit for placeholders should be enforced - we can do strlen in debug mode and throw fatal error in 2.2 and warning in earlier branches.

          Show
          Petr Škoda added a comment - Oracle is the craziest DB I ever saw that ignores any standards and the rest of the world. But I agree with Eloy that the 28 limit for placeholders should be enforced - we can do strlen in debug mode and throw fatal error in 2.2 and warning in earlier branches.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          yes, petr, problem are "automatic" placeholders (in update/insert...)

          Longfei, it's normal there is nothing (table/columns/indexes) > 30 it's enforced since ages ago. Problem appeared recently (only with columns > 28) because we started using 2 extra chars for placeholders. And 29 (or 30) + 2 > 30, hence the problem.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - yes, petr, problem are "automatic" placeholders (in update/insert...) Longfei, it's normal there is nothing (table/columns/indexes) > 30 it's enforced since ages ago. Problem appeared recently (only with columns > 28) because we started using 2 extra chars for placeholders. And 29 (or 30) + 2 > 30, hence the problem. Ciao
          Hide
          Longfei Yu added a comment -

          Hey, Eloy, Petr and Tim,

          Thanks all of your reply! Really appreciate it!

          Eloy, one question for you, you said "Problem appeared recently....", which means after v2.0.2, things changed? I am asking is because there is no problem with v2.0.2. Thanks, please confirm that.

          Longfei

          Show
          Longfei Yu added a comment - Hey, Eloy, Petr and Tim, Thanks all of your reply! Really appreciate it! Eloy, one question for you, you said "Problem appeared recently....", which means after v2.0.2, things changed? I am asking is because there is no problem with v2.0.2. Thanks, please confirm that. Longfei
          Hide
          Tim Hunt added a comment -

          I think there was a different problem with 2.0.2 that this change was meant to fix.

          Show
          Tim Hunt added a comment - I think there was a different problem with 2.0.2 that this change was meant to fix.
          Hide
          Tim Hunt added a comment -

          git history finds MDL-26842 as the cause.

          Show
          Tim Hunt added a comment - git history finds MDL-26842 as the cause.
          Hide
          Longfei Yu added a comment -

          I also fixed some Oracle reserved word issues, like "date", etc, for 2.0 version.

          I remember that's related to the Quiz part.

          Show
          Longfei Yu added a comment - I also fixed some Oracle reserved word issues, like "date", etc, for 2.0 version. I remember that's related to the Quiz part.
          Hide
          Longfei Yu added a comment -

          After I changed the source code, faculty also found that she still got the same error when she updated a multiple choice question. I finally found the logic behind the scene, and here is the updated code in question/type/multichoice/questiontype.php:
          .....line: 107
          $options = $DB->get_record('question_multichoice', array('question' => $question->id));
          if (!$options)

          { $options = new stdClass; $options->question = $question->id; $options->correctfeedback = ''; $options->partiallycorrectfeedback = ''; $options->incorrectfeedback = ''; $options->id = $DB->insert_record('question_multichoice', $options); //}

          $options->answers = implode(',', $answers);
          $options->single = $question->single;
          if (isset($question->layout))

          { $options->layout = $question->layout; }

          $options->answernumbering = $question->answernumbering;
          $options->shuffleanswers = $question->shuffleanswers;
          $options->correctfeedback = $this->import_or_save_files($question->correctfeedback,
          $context, 'qtype_multichoice', 'correctfeedback', $question->id);
          $options->correctfeedbackformat = $question->correctfeedback['format'];
          $options->partiallycorrectfeedback = $this->import_or_save_files($question->partiallycorrectfeedback,
          $context, 'qtype_multichoice', 'partiallycorrectfeedback', $question->id);
          //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback['format'];
          $options->incorrectfeedback = $this->import_or_save_files($question->incorrectfeedback,
          $context, 'qtype_multichoice', 'incorrectfeedback', $question->id);
          $options->incorrectfeedbackformat = $question->incorrectfeedback['format'];

          $DB->update_record('question_multichoice', $options);
          }else{
          $options2 = new stdClass;
          $options2->id = $options->id;
          $options2->question = $question->id;
          $options2->answers = implode(',', $answers);
          $options2->single = $question->single;
          if(isset($question->layout))

          { $options2->layout = $question->layout; }

          ;
          $options2->answernumbering = $question->answernumbering;
          $options2->shuffleanswers = $question->shuffleanswers;
          $options2->correctfeedback = $this->import_or_save_files($question->correctfeedback,
          $context, 'qtype_multichoice', 'correctfeedback', $question->id);
          $options2->correctfeedbackformat = $question->correctfeedback['format'];
          $options2->partiallycorrectfeedback = $this->import_or_save_files($question->partiallycorrectfeedback,
          $context, 'qtype_multichoice', 'partiallycorrectfeedback', $question->id);
          //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback['format'];
          $options2->incorrectfeedback = $this->import_or_save_files($question->incorrectfeedback,
          $context, 'qtype_multichoice', 'incorrectfeedback', $question->id);
          $options2->incorrectfeedbackformat = $question->incorrectfeedback['format'];

          $DB->update_record('question_multichoice', $options2);
          }
          /// Perform sanity checks on fractional grades
          .....

          Also, the "calculated" question type also has this issue. Going to work on it later.

          Show
          Longfei Yu added a comment - After I changed the source code, faculty also found that she still got the same error when she updated a multiple choice question. I finally found the logic behind the scene, and here is the updated code in question/type/multichoice/questiontype.php: .....line: 107 $options = $DB->get_record('question_multichoice', array('question' => $question->id)); if (!$options) { $options = new stdClass; $options->question = $question->id; $options->correctfeedback = ''; $options->partiallycorrectfeedback = ''; $options->incorrectfeedback = ''; $options->id = $DB->insert_record('question_multichoice', $options); //} $options->answers = implode(',', $answers); $options->single = $question->single; if (isset($question->layout)) { $options->layout = $question->layout; } $options->answernumbering = $question->answernumbering; $options->shuffleanswers = $question->shuffleanswers; $options->correctfeedback = $this->import_or_save_files($question->correctfeedback, $context, 'qtype_multichoice', 'correctfeedback', $question->id); $options->correctfeedbackformat = $question->correctfeedback ['format'] ; $options->partiallycorrectfeedback = $this->import_or_save_files($question->partiallycorrectfeedback, $context, 'qtype_multichoice', 'partiallycorrectfeedback', $question->id); //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback ['format'] ; $options->incorrectfeedback = $this->import_or_save_files($question->incorrectfeedback, $context, 'qtype_multichoice', 'incorrectfeedback', $question->id); $options->incorrectfeedbackformat = $question->incorrectfeedback ['format'] ; $DB->update_record('question_multichoice', $options); }else{ $options2 = new stdClass; $options2->id = $options->id; $options2->question = $question->id; $options2->answers = implode(',', $answers); $options2->single = $question->single; if(isset($question->layout)) { $options2->layout = $question->layout; } ; $options2->answernumbering = $question->answernumbering; $options2->shuffleanswers = $question->shuffleanswers; $options2->correctfeedback = $this->import_or_save_files($question->correctfeedback, $context, 'qtype_multichoice', 'correctfeedback', $question->id); $options2->correctfeedbackformat = $question->correctfeedback ['format'] ; $options2->partiallycorrectfeedback = $this->import_or_save_files($question->partiallycorrectfeedback, $context, 'qtype_multichoice', 'partiallycorrectfeedback', $question->id); //$options->partiallycorrectfeedbackformat = $question->partiallycorrectfeedback ['format'] ; $options2->incorrectfeedback = $this->import_or_save_files($question->incorrectfeedback, $context, 'qtype_multichoice', 'incorrectfeedback', $question->id); $options2->incorrectfeedbackformat = $question->incorrectfeedback ['format'] ; $DB->update_record('question_multichoice', $options2); } /// Perform sanity checks on fractional grades ..... Also, the "calculated" question type also has this issue. Going to work on it later.
          Hide
          Noel Sultana added a comment -

          After I changed the source code as above, when trying to upgrade to latest Moodle 2.1+ or 2.0.3+ I still got same Oracle ORA-00972 errors.

          Eventually, I got both upgrades to finish successfully and also tested all questions functionality fully works (both "multichoice" and "calculated" questions) by changing code in one file:

          /lib/dml/oci_native_moodle_database.php

          Basically removing reference to o_ prefix for bind variable ensuring size is 30 characters or less so Oracle does not error.

          Here is the updated code the updated code I used in /lib/dml/oci_native_moodle_database.php:

          ORIGINAL Lines
          Line:
          369: $newparams['o_'.$name] = $value;
          371: $sql = preg_replace('/[a-z0-9_-]+[$a-z0-9_-])/', ':o_$1', $sql);
          870: if ($key == 'o_newfieldtoset') { // found case where column and key diverge, handle that
          1172: oci_bind_by_name($stmt, ":o_oracle_id", $id, 10, SQLT_INT); // :o_ prefix added in tweak_param_names()

          MODIFIED Lines
          Line:
          369: $newparams[$name] = $value;
          371: $sql = preg_replace('/[a-z0-9_-]+[$a-z0-9_-])/', ':$1', $sql);
          870: if ($key == 'newfieldtoset') { // found case where column and key diverge, handle that
          1172: oci_bind_by_name($stmt, ":oracle_id", $id, 10, SQLT_INT);

          Show
          Noel Sultana added a comment - After I changed the source code as above, when trying to upgrade to latest Moodle 2.1+ or 2.0.3+ I still got same Oracle ORA-00972 errors. Eventually, I got both upgrades to finish successfully and also tested all questions functionality fully works (both "multichoice" and "calculated" questions) by changing code in one file: /lib/dml/oci_native_moodle_database.php Basically removing reference to o_ prefix for bind variable ensuring size is 30 characters or less so Oracle does not error. Here is the updated code the updated code I used in /lib/dml/oci_native_moodle_database.php: ORIGINAL Lines Line: 369: $newparams ['o_'.$name] = $value; 371: $sql = preg_replace('/ [a-z0-9_-] + [$a-z0-9_-] )/', ':o_$1', $sql); 870: if ($key == 'o_newfieldtoset') { // found case where column and key diverge, handle that 1172: oci_bind_by_name($stmt, ":o_oracle_id", $id, 10, SQLT_INT); // :o_ prefix added in tweak_param_names() MODIFIED Lines Line: 369: $newparams [$name] = $value; 371: $sql = preg_replace('/ [a-z0-9_-] + [$a-z0-9_-] )/', ':$1', $sql); 870: if ($key == 'newfieldtoset') { // found case where column and key diverge, handle that 1172: oci_bind_by_name($stmt, ":oracle_id", $id, 10, SQLT_INT);
          Hide
          Tim Hunt added a comment -

          Noel, your change is dangerous because Oracle will also complain if the placeholder name is the same as a database reserved word - and that might happen in some places in Moodle.

          Show
          Tim Hunt added a comment - Noel, your change is dangerous because Oracle will also complain if the placeholder name is the same as a database reserved word - and that might happen in some places in Moodle.
          Hide
          Yanfei Lu added a comment -

          When I upgrade moodle from 1.9 to 2.0.3, I change the code like Nole said. After upgrade, I change code back. Tim, Is this solution OK?

          Show
          Yanfei Lu added a comment - When I upgrade moodle from 1.9 to 2.0.3, I change the code like Nole said. After upgrade, I change code back. Tim, Is this solution OK?
          Hide
          Noel Sultana added a comment -

          When you put the code back the code and a user try to insert or update multichoice questions or calculated questions, you will get the same database write error messages.

          Show
          Noel Sultana added a comment - When you put the code back the code and a user try to insert or update multichoice questions or calculated questions, you will get the same database write error messages.
          Hide
          Yanfei Lu added a comment -

          There's only one way to solve the problem now, change this table's field name. I use the script below to change all "partiallycorrectfeedbackformat" to "partiallycorrectfbformat". I cann't get another good name for this. This solution has one problem, if I want upgrade moodle in the future, I have to change the source code all the time. That's distressing.

          grep -rl 'partiallycorrectfeedbackformat' /var/www/html/moodle | xargs -n1 sed -i 's/partiallycorrectfeedbackformat/partiallycorrectfbformat/'

          Show
          Yanfei Lu added a comment - There's only one way to solve the problem now, change this table's field name. I use the script below to change all "partiallycorrectfeedbackformat" to "partiallycorrectfbformat". I cann't get another good name for this. This solution has one problem, if I want upgrade moodle in the future, I have to change the source code all the time. That's distressing. grep -rl 'partiallycorrectfeedbackformat' /var/www/html/moodle | xargs -n1 sed -i 's/partiallycorrectfeedbackformat/partiallycorrectfbformat/'
          Hide
          Tim Hunt added a comment -

          No, that is a really bad solution.

          The only good solution is to change oci_native_moodle_database::tweak_param_names.

          Try this as a proof-of-concept:

          https://github.com/timhunt/moodle/commit/261091c7ddb94533835180ca4a08f2e4935f8f8b

          (I guess that patch will apply to any 2.x branch)

          Show
          Tim Hunt added a comment - No, that is a really bad solution. The only good solution is to change oci_native_moodle_database::tweak_param_names. Try this as a proof-of-concept: https://github.com/timhunt/moodle/commit/261091c7ddb94533835180ca4a08f2e4935f8f8b (I guess that patch will apply to any 2.x branch)
          Hide
          Longfei Yu added a comment -

          I don't think it is a good idea either, because, if you change the scripts, how about the database? That will be a disaster.

          For our local solution, I just unset the "partiallycorrectfeedbackformat" for all of the "Options" object. However, if the instructor puts something for the "partiallycorrectfeedback", it will lose the format, the HTML tag will display.

          Wish Moodle would be tested on Oracle before they release. Ok, just a wish!

          Longfei

          Show
          Longfei Yu added a comment - I don't think it is a good idea either, because, if you change the scripts, how about the database? That will be a disaster. For our local solution, I just unset the "partiallycorrectfeedbackformat" for all of the "Options" object. However, if the instructor puts something for the "partiallycorrectfeedback", it will lose the format, the HTML tag will display. Wish Moodle would be tested on Oracle before they release. Ok, just a wish! Longfei
          Hide
          Yanfei Lu added a comment -

          Tim, I got error when I test your code:
          PHP Fatal error: Class 'oracle_sql_generator' not found in /var/www/moodle/lib/dml/oci_native_moodle_database.php on line 221
          It seems that 'oracle_sql_generator' is not in released code.

          Show
          Yanfei Lu added a comment - Tim, I got error when I test your code: PHP Fatal error: Class 'oracle_sql_generator' not found in /var/www/moodle/lib/dml/oci_native_moodle_database.php on line 221 It seems that 'oracle_sql_generator' is not in released code.
          Hide
          Yanfei Lu added a comment - - edited

          I add this line to oci_native_moodle_database.php

          require_once($CFG->libdir.'/ddl/oracle_sql_generator.php');

          upgrade can be started, but I have other problem.
          qeupgradehelper seems not work in oracle, I always get error.

          Tim, If I don't want keep the quiz data, and I cann't use qeupgradehelper, and the count of m_question_sessions is above 10000, what can I do for upgrade?

          Show
          Yanfei Lu added a comment - - edited I add this line to oci_native_moodle_database.php require_once($CFG->libdir.'/ddl/oracle_sql_generator.php'); upgrade can be started, but I have other problem. qeupgradehelper seems not work in oracle, I always get error. Tim, If I don't want keep the quiz data, and I cann't use qeupgradehelper, and the count of m_question_sessions is above 10000, what can I do for upgrade?
          Hide
          Tim Hunt added a comment -

          Sorry, I should have made it clear, I did not test that code (I don't have an Oracle install) I just typed it to give a suggestion of how it could work. Sorry about missing the require_once.

          I don't know why that DB query fails on Oracle (hey, I just noticed, you deleted the query and the error message!)

          Debug info: ORA-00600: internal error code, arguments: [19004], [], [], [], [], [], [], [], [], [], [], []

          SELECT *
          FROM m_quiz_attempts quiza
          WHERE uniqueid IN (
          SELECT DISTINCT qst.attempt
          FROM m_question_states qst
          LEFT JOIN m_question_sessions qsess ON
          qst.question = qsess.questionid AND qst.attempt = qsess.attemptid
          WHERE qsess.id IS NULL
          )

          • line 562 of /mod/quiz/db/upgrade.php: call to oci_native_moodle_database->get_records_sql()

          Nothing in that query strikes my as particularly advanced or data-base specific SQL, and the error message from Oracle seems to be completely useless. If you can work out what that error message actually means, or which bit of the query is causing the problem, we can probably fix it. (By the way, it has nothing to do with qeupgradehelper).

          Show
          Tim Hunt added a comment - Sorry, I should have made it clear, I did not test that code (I don't have an Oracle install) I just typed it to give a suggestion of how it could work. Sorry about missing the require_once. I don't know why that DB query fails on Oracle (hey, I just noticed, you deleted the query and the error message!) Debug info: ORA-00600: internal error code, arguments: [19004] , [], [], [], [], [], [], [], [], [], [], [] SELECT * FROM m_quiz_attempts quiza WHERE uniqueid IN ( SELECT DISTINCT qst.attempt FROM m_question_states qst LEFT JOIN m_question_sessions qsess ON qst.question = qsess.questionid AND qst.attempt = qsess.attemptid WHERE qsess.id IS NULL ) line 562 of /mod/quiz/db/upgrade.php: call to oci_native_moodle_database->get_records_sql() Nothing in that query strikes my as particularly advanced or data-base specific SQL, and the error message from Oracle seems to be completely useless. If you can work out what that error message actually means, or which bit of the query is causing the problem, we can probably fix it. (By the way, it has nothing to do with qeupgradehelper).
          Hide
          Yanfei Lu added a comment - - edited

          I delete the error message for no other reason than that I found the error has nothing to do with moodle, it's all about Oracle. In order to upgrade moodle 1.9 to 2.x, I tried about 50 times. Tables have been transported many times, and I also use flashback many times. Something broken for these tables( I guess), that cause the error. I mentioned qeupgradehelper, that's because I get the same ORA-00600[19004] error. Now I think I can try it one more time.

          Tim, after use you code, I upgrade moodle 1.9 to 2.1 successfully! Thanks a lot!

          By the way, I proposed MDL-28422. I think there's some typo in the code, I cann't find out where it is. Can you take a look at it?

          Show
          Yanfei Lu added a comment - - edited I delete the error message for no other reason than that I found the error has nothing to do with moodle, it's all about Oracle. In order to upgrade moodle 1.9 to 2.x, I tried about 50 times. Tables have been transported many times, and I also use flashback many times. Something broken for these tables( I guess), that cause the error. I mentioned qeupgradehelper, that's because I get the same ORA-00600 [19004] error. Now I think I can try it one more time. Tim, after use you code, I upgrade moodle 1.9 to 2.1 successfully! Thanks a lot! By the way, I proposed MDL-28422 . I think there's some typo in the code, I cann't find out where it is. Can you take a look at it?
          Hide
          Tim Hunt added a comment -

          Ah, sorry.

          So, it looks like my patch is possibly a good change. The one bit I don't like about it is having to do require_once($CFG->libdir.'/ddl/oracle_sql_generator.php'); to get the list of reserved words.

          Anyway, the person best placed to review this is Eloy, who has just started a three-week holiday. Oh well. I will submit this for peer-review, and hopefully he will see it when he gets back.

          Show
          Tim Hunt added a comment - Ah, sorry. So, it looks like my patch is possibly a good change. The one bit I don't like about it is having to do require_once($CFG->libdir.'/ddl/oracle_sql_generator.php'); to get the list of reserved words. Anyway, the person best placed to review this is Eloy, who has just started a three-week holiday. Oh well. I will submit this for peer-review, and hopefully he will see it when he gets back.
          Hide
          Petr Škoda added a comment -

          Am I missing something? There is no strlen() in the patch, next time somebody introduces long parameter name there will be exactly the same problem for oracle only, right?

          Show
          Petr Škoda added a comment - Am I missing something? There is no strlen() in the patch, next time somebody introduces long parameter name there will be exactly the same problem for oracle only, right?
          Hide
          Tim Hunt added a comment -

          Thanks for taking a look Petr, but I think what you are asking for is a separate issue.

          The issue I have fixed if that Moodle was indescriminately adding 2 chars to placeholder names, which, in the case of Oracle turned valid 30-char column names into invalid 32-char placeholder names.

          There is a separate issue: developers should no use placeholder names longer than 30 chars because it breaks Oracle. We ought to have a coding exception for that, but it should be in the base class, not in oracle-specific code, so all developers becomes aware of this. I will do another commit for that.

          https://github.com/timhunt/moodle/commit/f867742f75ed900b4e59425f4bcd3b1343e3cf98

          again, unteseted, but it should work. Really, we should add some unit tests for this new code too. However, this is not really my area of code, and I should be working on other things, so I will probably leave this here, and let someone else finish it off.

          Show
          Tim Hunt added a comment - Thanks for taking a look Petr, but I think what you are asking for is a separate issue. The issue I have fixed if that Moodle was indescriminately adding 2 chars to placeholder names, which, in the case of Oracle turned valid 30-char column names into invalid 32-char placeholder names. There is a separate issue: developers should no use placeholder names longer than 30 chars because it breaks Oracle. We ought to have a coding exception for that, but it should be in the base class, not in oracle-specific code, so all developers becomes aware of this. I will do another commit for that. https://github.com/timhunt/moodle/commit/f867742f75ed900b4e59425f4bcd3b1343e3cf98 again, unteseted, but it should work. Really, we should add some unit tests for this new code too. However, this is not really my area of code, and I should be working on other things, so I will probably leave this here, and let someone else finish it off.
          Hide
          Petr Škoda added a comment -

          Separate issue is fine for me, I guess it will be a trivial change on top of your commit, thanks.

          Show
          Petr Škoda added a comment - Separate issue is fine for me, I guess it will be a trivial change on top of your commit, thanks.
          Hide
          Tim Hunt added a comment -

          I already changed it on top of my commit!

          Show
          Tim Hunt added a comment - I already changed it on top of my commit!
          Hide
          Tim Hunt added a comment -

          I just rebased my branch. Any chance of getting this integrated?

          Show
          Tim Hunt added a comment - I just rebased my branch. Any chance of getting this integrated?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, it's for sure better than current implementation of tweak_params() for sure. In fact my primary idea, before the function arrived, was exactly to, centrally, for all DBs, load the list of reserved words and perform the checks always, throwing exception if reserved use was found. Surely under developer mode only.

          Also, as part of the check_db_syntax.php report, I had all the alias (AS xxxx sentences) checked and we have only a few using reserved words.

          So I've divided feelings about how to fix this. IMO there are 2 alternatives:

          1) Your fix to tweak_params() exclusively for Oracle.

          2) Revert the tweak params completely and try to implement the "exception on development" and fix current bad uses.

          Note that both approaches solve both the reserved names and the max length issues. The difference is that 1) does it only for Oracle automatically but makes it (yet) slower 2) does it globally (does not make things slower but for developers) but is not "automatic".

          +0.1 for 1) right now, but 2) is something that should be considered in the long term.

          Any thoughts? Ciao

          PS: Note I've not checked current proposed code 100%. If we go that route, I'll pick them and add some unittests to have it covered ok.

          Show
          Eloy Lafuente (stronk7) added a comment - Well, it's for sure better than current implementation of tweak_params() for sure. In fact my primary idea, before the function arrived, was exactly to, centrally, for all DBs, load the list of reserved words and perform the checks always, throwing exception if reserved use was found. Surely under developer mode only. Also, as part of the check_db_syntax.php report, I had all the alias (AS xxxx sentences) checked and we have only a few using reserved words. So I've divided feelings about how to fix this. IMO there are 2 alternatives: 1) Your fix to tweak_params() exclusively for Oracle. 2) Revert the tweak params completely and try to implement the "exception on development" and fix current bad uses. Note that both approaches solve both the reserved names and the max length issues. The difference is that 1) does it only for Oracle automatically but makes it (yet) slower 2) does it globally (does not make things slower but for developers) but is not "automatic". +0.1 for 1) right now, but 2) is something that should be considered in the long term. Any thoughts? Ciao PS: Note I've not checked current proposed code 100%. If we go that route, I'll pick them and add some unittests to have it covered ok.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          And too 3) instead of loading the reserved words on each request, simply ignore them and change the tweak_params() algorithm to, instead of prefixing with "o_", replace the last 4 chars with "_pXX", so length won't increase.

          Show
          Eloy Lafuente (stronk7) added a comment - And too 3) instead of loading the reserved words on each request, simply ignore them and change the tweak_params() algorithm to, instead of prefixing with "o_", replace the last 4 chars with "_pXX", so length won't increase.
          Hide
          Tim Hunt added a comment -

          The problem with 3) is get_in_or_equal. That could well cause XX to get longer than 2 chars.

          How about 1) in the stable branches, and 2) in master, with warning in the release notes.

          2) is the only way to get best performance in production.

          Perhaps also add developer debug warning on the stable branches, so people can start fixing their third-party plugins now, without fatal exceptions.

          Show
          Tim Hunt added a comment - The problem with 3) is get_in_or_equal. That could well cause XX to get longer than 2 chars. How about 1) in the stable branches, and 2) in master, with warning in the release notes. 2) is the only way to get best performance in production. Perhaps also add developer debug warning on the stable branches, so people can start fixing their third-party plugins now, without fatal exceptions.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Uhm, one more iteration. Some facts (IMO):

          1) Only oracle (for now) uses named params for bindings.

          2) the tweak_params() method is NOT necessary in non xxx_sql() methods (insert/update/delete/set_field...) at all. Why? because the original implementation uses column names as params, and we know that all our column names are correct (non reserved words and < 30).

          3) the tweak_params() method is necessary in all xxx_select/sql() methods, because there is where we can introduce "custom" (offending) params (reserved words and > 30).

          4) This is only about params, it does not cover column aliases (AS xxx) that need to be fixed in another way (check_db_syntax.php executions or similar).

          So, for example, this issue is clearly one 2) case, one update statement not needing the tweak at all (column names are safe).

          Also, 3) is always code (param named queries) introduced by developers and knowing how lazy we are, the chances of doing that using 30 chars for params is really, really minor. In fact roughly all historic problems are with params like 'user', 'join', 'group'...

          So here it's another proposal:

          A) For 20, 21 and master, right now, on Oracle:

          1) stop calling tweak_params() in all non xxx_sql/select() methods. Not needed.
          2) continue calling tweak_params() in all xxx_sql/select() methods, and add the bloody 'o_' prefix if length <= 28 or replace the 2 first chars by 'o_' if length > 28.

          B) For 20, 21 and master, right now:

          3) throw exception if any param name length is > 30 in fix_sql_params() (your second commit). We could relax it to debugging() under stable branches to avoid regressions.

          C) (optional) master only:

          4) Create new followup issue so, when running in developer mode, we load ALL the reserved words and check for them in fix_sql_params(), debugging if found. In the long term (say 2.4) that will make A2) above unnecessary (or we can keep it for safety). To decide/implement in separate bug.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Uhm, one more iteration. Some facts (IMO): 1) Only oracle (for now) uses named params for bindings. 2) the tweak_params() method is NOT necessary in non xxx_sql() methods (insert/update/delete/set_field...) at all. Why? because the original implementation uses column names as params, and we know that all our column names are correct (non reserved words and < 30). 3) the tweak_params() method is necessary in all xxx_select/sql() methods, because there is where we can introduce "custom" (offending) params (reserved words and > 30). 4) This is only about params, it does not cover column aliases (AS xxx) that need to be fixed in another way (check_db_syntax.php executions or similar). So, for example, this issue is clearly one 2) case, one update statement not needing the tweak at all (column names are safe). Also, 3) is always code (param named queries) introduced by developers and knowing how lazy we are, the chances of doing that using 30 chars for params is really, really minor. In fact roughly all historic problems are with params like 'user', 'join', 'group'... So here it's another proposal: A) For 20, 21 and master, right now, on Oracle: 1) stop calling tweak_params() in all non xxx_sql/select() methods. Not needed. 2) continue calling tweak_params() in all xxx_sql/select() methods, and add the bloody 'o_' prefix if length <= 28 or replace the 2 first chars by 'o_' if length > 28. B) For 20, 21 and master, right now: 3) throw exception if any param name length is > 30 in fix_sql_params() (your second commit). We could relax it to debugging() under stable branches to avoid regressions. C) (optional) master only: 4) Create new followup issue so, when running in developer mode, we load ALL the reserved words and check for them in fix_sql_params(), debugging if found. In the long term (say 2.4) that will make A2) above unnecessary (or we can keep it for safety). To decide/implement in separate bug. Ciao
          Hide
          Tim Hunt added a comment -

          Regarding 3). No point changing throw to debugging there. If we don't throw, a more cryptic Oracle error will be thrown.

          Other than that, good analysis. Go for it. +1

          Show
          Tim Hunt added a comment - Regarding 3). No point changing throw to debugging there. If we don't throw, a more cryptic Oracle error will be thrown. Other than that, good analysis. Go for it. +1
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Updated git repo source with proposed changes, it does (using numbering above):

          1) stop calling tweak_params() in insert/update. Not needed.
          2) continue calling tweak_params() in the rest of methods. Needed.
          3) throw coding exception if placeholder > 30 is found.

          And:

          0) Unit tests covering all the stuff above. Passing under mysqli, pgsql, mssql and oracle, yay.

          Everything available @:

          TODO:

          T1) coding exception or dml exception. Not 100% sure

          Any comment will be welcome. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Updated git repo source with proposed changes, it does (using numbering above): 1) stop calling tweak_params() in insert/update. Not needed. 2) continue calling tweak_params() in the rest of methods. Needed. 3) throw coding exception if placeholder > 30 is found. And: 0) Unit tests covering all the stuff above. Passing under mysqli, pgsql, mssql and oracle, yay. Everything available @: master: https://github.com/stronk7/moodle/compare/master...MDL-28080 21_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_21_STABLE...MDL-28080_21 20_STABLE: https://github.com/stronk7/moodle/compare/MOODLE_20_STABLE...MDL-28080_20 TODO: T1) coding exception or dml exception. Not 100% sure Any comment will be welcome. Ciao
          Hide
          Tim Hunt added a comment -

          On a quick inspection, looks good.

          And now, I am on holiday!

          Show
          Tim Hunt added a comment - On a quick inspection, looks good. And now, I am on holiday!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (added complete testing instructions)

          Show
          Eloy Lafuente (stronk7) added a comment - (added complete testing instructions)
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          MDL-29217 has been created to discuss the implementation of developer checks in order to better pre-detect the use of reserved words.

          So this is the TODO:

          T1) coding exception or dml exception. Not 100% sure

          Show
          Eloy Lafuente (stronk7) added a comment - - edited MDL-29217 has been created to discuss the implementation of developer checks in order to better pre-detect the use of reserved words. So this is the TODO: T1) coding exception or dml exception. Not 100% sure
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Submitting for integration, I've left the coding exception instead of dml one. Not critical, I think.

          Show
          Eloy Lafuente (stronk7) added a comment - Submitting for integration, I've left the coding exception instead of dml one. Not critical, I think.
          Hide
          Sam Hemelryk added a comment -

          Thanks guys, this has been integrated now

          Show
          Sam Hemelryk added a comment - Thanks guys, this has been integrated now
          Hide
          Aparup Banerjee added a comment -

          finally passed for me. what a test!

          Show
          Aparup Banerjee added a comment - finally passed for me. what a test!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          git & cvs repositories updated with your gorgeous code. Many thanks!

          Closing and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - git & cvs repositories updated with your gorgeous code. Many thanks! Closing and ciao
          Hide
          Elizabeth Dalton added a comment -

          I think a change made in this code to NAME_MAX_LENGTH may be affecting sites not using Oracle. I have a large number of plugins that won't install on a fresh 2.3 server and kick out the dreaded "Coding error detected, it must be fixed by a programmer: Invalid table name

          {plagiarism_crot_submission_pair}

          : name is too long. Limit is 28 chars" message. I checked the workaround at https://moodle.org/mod/forum/discuss.php?d=116512#p908383 (which I found via a link in http://tracker.moodle.org/browse/CONTRIB-3770 ). I manually edited NAME_MAX_LENGTH in /whome/moodle/lib/xmldb/xmldb_table.php and now I am able to install the plugins. While researching this I found a large number of reports of various plugins not installing with this error. Is there supposed to be a check to ensure that the site is using Oracle before setting this constant so low?

          Show
          Elizabeth Dalton added a comment - I think a change made in this code to NAME_MAX_LENGTH may be affecting sites not using Oracle. I have a large number of plugins that won't install on a fresh 2.3 server and kick out the dreaded "Coding error detected, it must be fixed by a programmer: Invalid table name {plagiarism_crot_submission_pair} : name is too long. Limit is 28 chars" message. I checked the workaround at https://moodle.org/mod/forum/discuss.php?d=116512#p908383 (which I found via a link in http://tracker.moodle.org/browse/CONTRIB-3770 ). I manually edited NAME_MAX_LENGTH in /whome/moodle/lib/xmldb/xmldb_table.php and now I am able to install the plugins. While researching this I found a large number of reports of various plugins not installing with this error. Is there supposed to be a check to ensure that the site is using Oracle before setting this constant so low?
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Elizabeth,
          This change was done to enable easier cross DB compatibility for any single distribution of plugin. I don't think there is any easy logic to shorten table names for all plugins based on what DB(oracle) is being used. any tweaking to table names could break plugins and their code.

          There may be a table name mapping that could be implemented in some intermediate storage layer (enter MUC with much lesser limitations) to separate these oracle type limitations from developers. I've reported MDL-36739 to consider this.

          Show
          Aparup Banerjee added a comment - - edited Hi Elizabeth, This change was done to enable easier cross DB compatibility for any single distribution of plugin. I don't think there is any easy logic to shorten table names for all plugins based on what DB(oracle) is being used. any tweaking to table names could break plugins and their code. There may be a table name mapping that could be implemented in some intermediate storage layer (enter MUC with much lesser limitations) to separate these oracle type limitations from developers. I've reported MDL-36739 to consider this.
          Hide
          Elizabeth Dalton added a comment -

          Aparup, I understand that, but the change has broken a large number of existing plugins for systems that don't use Oracle. Even if there is no way to automate the shortening of names or using an intermediate storage layer, I don't think it makes sense to break the functionality of all the systems that aren't using Oracle. Is there no way to test for which database is being used and adjust the limit only for Oracle systems, with an alert to the system administrator indicating that many plugins may not work as a result? Improving the error message to make it clear what causes the problem and what changes could help fix it (e.g. using a database other than Oracle) would also be more helpful.

          Show
          Elizabeth Dalton added a comment - Aparup, I understand that, but the change has broken a large number of existing plugins for systems that don't use Oracle. Even if there is no way to automate the shortening of names or using an intermediate storage layer, I don't think it makes sense to break the functionality of all the systems that aren't using Oracle. Is there no way to test for which database is being used and adjust the limit only for Oracle systems, with an alert to the system administrator indicating that many plugins may not work as a result? Improving the error message to make it clear what causes the problem and what changes could help fix it (e.g. using a database other than Oracle) would also be more helpful.
          Hide
          Petr Škoda added a comment -

          I personally vote for dropping of Oracle support, it never worked properly with Moodle and it only creates problems for everybody else.

          Show
          Petr Škoda added a comment - I personally vote for dropping of Oracle support, it never worked properly with Moodle and it only creates problems for everybody else.
          Hide
          Bo Mack added a comment -

          I hope Moodle.org takes a very deliberate and deliberative approach before dropping Oracle as a supported platform. My institution would be very disappointed if support for Oracle were dropped casually. Although I'm definitely not a cheerleader for Oracle Corporation or products, I know that many organizations have reasons to run the Oracle database, despite its limitations and shortcomings. And I think many of those organizations' reasons are quite legitimate. I certainly vote to keep supporting Oracle db at this time, because I'm not convinced that limitations like the one described in this issue are so onerous that they can't be resolved reasonably.

          Show
          Bo Mack added a comment - I hope Moodle.org takes a very deliberate and deliberative approach before dropping Oracle as a supported platform. My institution would be very disappointed if support for Oracle were dropped casually. Although I'm definitely not a cheerleader for Oracle Corporation or products, I know that many organizations have reasons to run the Oracle database, despite its limitations and shortcomings. And I think many of those organizations' reasons are quite legitimate. I certainly vote to keep supporting Oracle db at this time, because I'm not convinced that limitations like the one described in this issue are so onerous that they can't be resolved reasonably.
          Hide
          Tim Hunt added a comment -

          Moodle.org has no plans to drop Oracle support.

          Petr hates Oracle (a very understandable position) but his statements on the subject are no indication of official moodle.org policy.

          Show
          Tim Hunt added a comment - Moodle.org has no plans to drop Oracle support. Petr hates Oracle (a very understandable position) but his statements on the subject are no indication of official moodle.org policy.
          Hide
          Petr Škoda added a comment -

          The current Oracle support helps a few sites initially but in the end they often migrate to other databases because it is either too slow (thanks to buggy Oracle PHP drivers) or users hit too many incompatibilities in core or add-on code. I do mean it seriously, if we did not support Oracle we would not have problems with table lengths, varchar fields could be much bigger for full multilang support, there would be no silly broken empty string hacks all over the place, etc.

          Tim: If you disagree please start fixing all Oracle bugs and resolving all the problems the Oracle support brings. Otherwise could you please stop talking for other developers and stop promising miracles?

          Show
          Petr Škoda added a comment - The current Oracle support helps a few sites initially but in the end they often migrate to other databases because it is either too slow (thanks to buggy Oracle PHP drivers) or users hit too many incompatibilities in core or add-on code. I do mean it seriously, if we did not support Oracle we would not have problems with table lengths, varchar fields could be much bigger for full multilang support, there would be no silly broken empty string hacks all over the place, etc. Tim: If you disagree please start fixing all Oracle bugs and resolving all the problems the Oracle support brings. Otherwise could you please stop talking for other developers and stop promising miracles?
          Hide
          Elizabeth Dalton added a comment -

          I am not asking for Moodle to drop Oracle support, but I don't think it's right that limitations in Oracle are added to the Moodle core in a way that breaks a lot of existing plugins. That just seems to me to be a very bad policy, unless the overwhelming majority of Moodle sites are running Oracle and support for non-Oracle databases will be dropped. It's a major API change, effectively. Again, is there no way to test for which database is being used, and make the limit only apply to Oracle systems? Or are the rest of us simply going to have to make a manual edit in core code from now on to get plugins in the repository to work?

          Show
          Elizabeth Dalton added a comment - I am not asking for Moodle to drop Oracle support, but I don't think it's right that limitations in Oracle are added to the Moodle core in a way that breaks a lot of existing plugins. That just seems to me to be a very bad policy, unless the overwhelming majority of Moodle sites are running Oracle and support for non-Oracle databases will be dropped. It's a major API change, effectively. Again, is there no way to test for which database is being used, and make the limit only apply to Oracle systems? Or are the rest of us simply going to have to make a manual edit in core code from now on to get plugins in the repository to work?
          Hide
          Tim Hunt added a comment -

          Petr, I do fix oracle bugs that arise in the quiz/question area.

          I think you should stop making public statements about oracle support. You are just worrying users. You should discuss such matter privately with Martin D / Michael dR, but actually that is none of my business. I was simply trying to reassure Bo Mack, who you had worried, that I had never heard anything official from Moodle.org that suggested that oracle support would be dropped.

          Show
          Tim Hunt added a comment - Petr, I do fix oracle bugs that arise in the quiz/question area. I think you should stop making public statements about oracle support. You are just worrying users. You should discuss such matter privately with Martin D / Michael dR, but actually that is none of my business. I was simply trying to reassure Bo Mack, who you had worried, that I had never heard anything official from Moodle.org that suggested that oracle support would be dropped.
          Hide
          Aparup Banerjee added a comment -

          Please see MDL-36739 which is essentially about 'make the limit only apply to Oracle systems'.

          There is no news officially (even unofficially) about dropping oracle support.

          Show
          Aparup Banerjee added a comment - Please see MDL-36739 which is essentially about 'make the limit only apply to Oracle systems'. There is no news officially (even unofficially) about dropping oracle support.
          Hide
          Martin Dougiamas added a comment -

          We are not dropping Oracle support for the forseeable future, even though it's undoubtedly the least-used database among Moodle users.

          That said, let's not cripple other databases to support Oracle. Let's aim to keep any hacks (that are because of Oracle only) inside our Oracle database driver.

          Show
          Martin Dougiamas added a comment - We are not dropping Oracle support for the forseeable future, even though it's undoubtedly the least-used database among Moodle users. That said, let's not cripple other databases to support Oracle. Let's aim to keep any hacks (that are because of Oracle only) inside our Oracle database driver.
          Hide
          Petr Škoda added a comment - - edited

          Thanks a lot Martin, this is exactly the decision we need because Eloy was apposing most of the "inside Oracle driver" hacks.

          I vote for the following changes:

          1/ allow table names to be 64chars long immediatelly - and later fix Oracle driver to do hashing like this substr($tablename, 0, 23).substr(md5($tablename), 0, 5), we would either need a list of all long table names in moodle (obtained from MUC) or just tweak the

          {tablename}

          in queries which would require full removal of $CFG->prefix usage

          2/ deprecate the confusing sql_empty() and do all the NULL <--> '' hacking in oracle driver

          3/ enlarge the varchar size to 3000 and simply cut off oversized strings in our Oracle driver (mysql does it for ages and nobody cares, please note only strings with many non-ascii characters would be affected, because the legacy Oracle DBA still use bytes)

          I suppose we can do all this in 2.5 relatively easily, the only problem might be resistance from Eloy because he loves Oracle databases.

          Show
          Petr Škoda added a comment - - edited Thanks a lot Martin, this is exactly the decision we need because Eloy was apposing most of the "inside Oracle driver" hacks. I vote for the following changes: 1/ allow table names to be 64chars long immediatelly - and later fix Oracle driver to do hashing like this substr($tablename, 0, 23).substr(md5($tablename), 0, 5), we would either need a list of all long table names in moodle (obtained from MUC) or just tweak the {tablename} in queries which would require full removal of $CFG->prefix usage 2/ deprecate the confusing sql_empty() and do all the NULL <--> '' hacking in oracle driver 3/ enlarge the varchar size to 3000 and simply cut off oversized strings in our Oracle driver (mysql does it for ages and nobody cares, please note only strings with many non-ascii characters would be affected, because the legacy Oracle DBA still use bytes) I suppose we can do all this in 2.5 relatively easily, the only problem might be resistance from Eloy because he loves Oracle databases.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry, guys, but I disagree completely with any unnecessary hack/hash/muc-solutionm like that.

          1) The 28 characters table/column limit/rule has been out there since Moodle 1.7, when the XMLDB stuff was implemented.
          2) The XMLDB editor enforced it since that day 0, and documentation about the limit is here and there.
          3) This issue just enforced it better/on execution time.
          4) Any plugin must adapt to that rule.

          That said, keeping completely apart any discussion about how ALL DBs have their particularities, issues and annoyances. None is free from them, both in the drivers and in conditional coding.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry, guys, but I disagree completely with any unnecessary hack/hash/muc-solutionm like that. 1) The 28 characters table/column limit/rule has been out there since Moodle 1.7, when the XMLDB stuff was implemented. 2) The XMLDB editor enforced it since that day 0, and documentation about the limit is here and there. 3) This issue just enforced it better/on execution time. 4) Any plugin must adapt to that rule. That said, keeping completely apart any discussion about how ALL DBs have their particularities, issues and annoyances. None is free from them, both in the drivers and in conditional coding. Ciao
          Hide
          Elizabeth Dalton added a comment -

          1 - Is there a test suite a plugin has to pass to be accepted in Contrib?

          2 - Is there a way of notifying the maintainers of all plugins that don't limit columns to 28 characters that their code needs fixes to come into compliance?

          3 - Is the limitation imposed by XMLDB due to any other factors besides the Oracle db limitation?

          From the perspective of a system administrator, having a bunch of plugins fail is a problem. It doesn't really matter to me if the plugins are out of compliance with a standard that was stated, but not enforced until now, or never stated, or changed recently, or whatever. What matters at my institution is that if we try to upgrade, we're going to have a bunch of plugins we downloaded from the Moodle.org site fail, unless we want to manually edit core. The issue for us isn't about whether Moodle ought to support Oracle. It's about whether we can rely on Moodle to work with plugins hosted in Contrib that say they are compatible with a specific version.

          Show
          Elizabeth Dalton added a comment - 1 - Is there a test suite a plugin has to pass to be accepted in Contrib? 2 - Is there a way of notifying the maintainers of all plugins that don't limit columns to 28 characters that their code needs fixes to come into compliance? 3 - Is the limitation imposed by XMLDB due to any other factors besides the Oracle db limitation? From the perspective of a system administrator, having a bunch of plugins fail is a problem. It doesn't really matter to me if the plugins are out of compliance with a standard that was stated, but not enforced until now, or never stated, or changed recently, or whatever. What matters at my institution is that if we try to upgrade, we're going to have a bunch of plugins we downloaded from the Moodle.org site fail, unless we want to manually edit core. The issue for us isn't about whether Moodle ought to support Oracle. It's about whether we can rely on Moodle to work with plugins hosted in Contrib that say they are compatible with a specific version.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Elizabeth,

          I'm not aware of any test suite checking for that sort of things right note (note I'm not aware of the checks performed by the plugins database at all, surely Aparup can confirm this) but, for sure, that's the direction to take ASAP.

          Right now we are moving more and more our (core) development process both to Unit Test driven development and to the use of Continuous Integration tools, able to detect/test/check wrong code/habits and, for sure, a lot of those jobs could be applied to contrib plugins easily.

          Those checks include both installing, upgrading and automated testing (apart from other checks) and, for sure are able to detect things like the use of > 28 character long columns/placeholders.

          So, yes, we can start looking for those things (and more), both in core and in plugins. Obviously, it's not something automated, and will require some planning before being available for the masses. Ideally it should be fully automates and, every time one plugin is sent to the database, all the checks are run and the author is immediately informed about any problem found. The author and/or, why not, the whole community, by making the info available @ the plugins database.

          But, back to the reality, right now, the only way to verify if one contrib plugin works is, simply, trying to use it. Install, upgrade and use it (in a testing env). Nothing else will tell you, from an administrator POV, if the plugin is correct and works, or if it upgrades correctly from older versions. In fact, no matter of the future automated-checks to be implemented, that for sure will lead to less and less problems, that testing phase will continue being necessary, specially if the BUMP is big (and 1.9 => 2.x is big). After 2.0.x things are far more stable, IMO.

          So, all I can say is:

          • We need to move and start performing more and more checks against all contrib code. Some of them will lead to automatic rejection and others won't. Unit tests are always welcome bundled with the plugin.
          • Any problem should be reported/voted/informed to the plugin author, here in the Tracker, in the plugins DB and/or in forums.
          • Any upgrade, specially if big, will require a manual testing phase, and any problem found should be routed to the 2 points above (try to catch it automatically next time, and inform the author to fix it).

          Also, yes, the 28 characters limitation is, if I don't remember it wrongly, exclusively imposed by the Oracle limit. And, as far as core officially supports Oracle (and will continue supporting it, unless decided the opposite in the future), that becomes the common limit for all databases (note that there are also other limits imposed by other databases, surely with less impact, and that never has lead to questioning the support of that database).

          So, given the previous statement (Oracle is supported in core), the only 2 solutions for contrib plugins are:

          • or the Oracle support is also applied to the plugin (hard to developers and to admins, but pretty doable at the same time).
          • or we introduce some mechanism to allow contrib developers to specify the supported environments (easier to developers and safer for admins, that will get more ack about their options).

          Personally, I bet 95% of contrib authors won't declare their plugins as Oracle/MSSQL compatible, both because "everybody" seems to be using MySQL and PostgreSQL and the proprietary nature of those databases (harder to get, install and test).

          But, once again, no contrib plugin will be allowed to bypass "core rules" and they specify, right now, that tables/columns/aliases/placeholders cannot be > 28 chars, so in this case, the plugin is incompatible with ALL databases, it's not an Oracle problem. As simple as that. Only option is to fix the plugin (or, edited, change the core rules globally).

          Ciao

          Edited: added "or change the core rules globally" alternative.

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Elizabeth, I'm not aware of any test suite checking for that sort of things right note (note I'm not aware of the checks performed by the plugins database at all, surely Aparup can confirm this) but, for sure, that's the direction to take ASAP. Right now we are moving more and more our (core) development process both to Unit Test driven development and to the use of Continuous Integration tools, able to detect/test/check wrong code/habits and, for sure, a lot of those jobs could be applied to contrib plugins easily. Those checks include both installing, upgrading and automated testing (apart from other checks) and, for sure are able to detect things like the use of > 28 character long columns/placeholders. So, yes, we can start looking for those things (and more), both in core and in plugins. Obviously, it's not something automated, and will require some planning before being available for the masses. Ideally it should be fully automates and, every time one plugin is sent to the database, all the checks are run and the author is immediately informed about any problem found. The author and/or, why not, the whole community, by making the info available @ the plugins database. But, back to the reality, right now, the only way to verify if one contrib plugin works is, simply, trying to use it. Install, upgrade and use it (in a testing env). Nothing else will tell you, from an administrator POV, if the plugin is correct and works, or if it upgrades correctly from older versions. In fact, no matter of the future automated-checks to be implemented, that for sure will lead to less and less problems, that testing phase will continue being necessary, specially if the BUMP is big (and 1.9 => 2.x is big). After 2.0.x things are far more stable, IMO. So, all I can say is: We need to move and start performing more and more checks against all contrib code. Some of them will lead to automatic rejection and others won't. Unit tests are always welcome bundled with the plugin. Any problem should be reported/voted/informed to the plugin author, here in the Tracker, in the plugins DB and/or in forums. Any upgrade, specially if big, will require a manual testing phase, and any problem found should be routed to the 2 points above (try to catch it automatically next time, and inform the author to fix it). Also, yes, the 28 characters limitation is, if I don't remember it wrongly, exclusively imposed by the Oracle limit. And, as far as core officially supports Oracle (and will continue supporting it, unless decided the opposite in the future), that becomes the common limit for all databases (note that there are also other limits imposed by other databases, surely with less impact, and that never has lead to questioning the support of that database). So, given the previous statement (Oracle is supported in core), the only 2 solutions for contrib plugins are: or the Oracle support is also applied to the plugin (hard to developers and to admins, but pretty doable at the same time). or we introduce some mechanism to allow contrib developers to specify the supported environments (easier to developers and safer for admins, that will get more ack about their options). Personally, I bet 95% of contrib authors won't declare their plugins as Oracle/MSSQL compatible, both because "everybody" seems to be using MySQL and PostgreSQL and the proprietary nature of those databases (harder to get, install and test). But, once again, no contrib plugin will be allowed to bypass "core rules" and they specify, right now, that tables/columns/aliases/placeholders cannot be > 28 chars, so in this case, the plugin is incompatible with ALL databases, it's not an Oracle problem. As simple as that. Only option is to fix the plugin (or, edited, change the core rules globally). Ciao Edited: added "or change the core rules globally" alternative.

            People

            • Votes:
              4 Vote for this issue
              Watchers:
              9 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: