Moodle
  1. Moodle
  2. MDL-26025

Various problems with range (top) queries in the sqlsrv native driver

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0.1
    • Fix Version/s: 2.0.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Environment:
    • Database:
      Microsoft SQL
    • Difficulty:
      Difficult
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      15984

      Description

      I created a new instance of Moodle 2.0.1+ (Build: 20110112) on my Windows Server 2003 test machine. At the end of the setup I added Site News to my front page and got an error saying "The column 'name' was specified multiple times for 'q'". After turning the Moodle debugging option up to Developer level I see the following output:

      Warning: file_put_contents(D:\MoodleData2/cache/lang/en/core_error.php) [function.file-put-contents]: failed to open stream: Permission denied in D:\Moodle2\lib\moodlelib.php on line 5986

      Error reading from database

      More information about this error

      Debug info: SQLState: 42000<br>
      Error Code: 8156<br>
      Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The column 'name' was specified multiple times for 'q'.<br>

      SELECT , name, timemodified, usermodified, groupid, timestart, timeend, firstname, lastname, email, picture, imagealt FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY modified DESC) AS sqlsrvrownumber, *, name, timemodified, usermodified, groupid, timestart, timeend, firstname, lastname, email, picture, imagealt FROM (SELECT p., d.name, d.timemodified, d.usermodified, d.groupid, d.timestart, d.timeend,
      u.firstname, u.lastname, u.email, u.picture, u.imagealt
      FROM mdl_forum_discussions d
      JOIN mdl_forum_posts p ON p.discussion = d.id
      JOIN mdl_user u ON p.userid = u.id

      WHERE d.forum = '1' AND p.parent = 0

      ) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 1
      [array (
      0 => '1',
      )]
      Stack trace:
      line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown
      line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end()
      line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end()
      line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query()
      line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql()
      line 2596 of \mod\forum\lib.php: call to sqlsrv_native_moodle_database->get_records_sql()
      line 5084 of \mod\forum\lib.php: call to forum_get_discussions()
      line 175 of \index.php: call to forum_print_latest_discussions()

      I've attached the html output with the error.

      1. Error.htm
        151 kB
        Luis de Vasconcelos

        Activity

        Hide
        Gregor McNish added a comment -

        We have the same problem after a recent 2.01 update – I think the permission denied error is separate though; we get the sql error on existing Site News.

        There's a forum post on the issue http://moodle.org/mod/forum/discuss.php?d=166122

        It has other mssql issues in glossary and profile.

        Show
        Gregor McNish added a comment - We have the same problem after a recent 2.01 update – I think the permission denied error is separate though; we get the sql error on existing Site News. There's a forum post on the issue http://moodle.org/mod/forum/discuss.php?d=166122 It has other mssql issues in glossary and profile.
        Hide
        Gregor McNish added a comment -

        The profile issue has also been put in the tracker as MDL-25906

        Show
        Gregor McNish added a comment - The profile issue has also been put in the tracker as MDL-25906
        Hide
        Gregor McNish added a comment -

        I've noted this in MDL-25906 as well. Hope this is correct procedure?
        May have isolated this (though don't know the proper fix)
        In lib/dml/sqlsrv_native_moodle_database.php there's the function

        private function limit_to_top_n

        Replacing this function with it's previous incarnation from our older codebase restored functionality to site news, glossary, profile and logs.

        The function changed in the most recent release (CVS file v1.3 comment is weekly git snapshot 12/30/2010, but it doesn't reference the tracker??).

        Show
        Gregor McNish added a comment - I've noted this in MDL-25906 as well. Hope this is correct procedure? May have isolated this (though don't know the proper fix) In lib/dml/sqlsrv_native_moodle_database.php there's the function private function limit_to_top_n Replacing this function with it's previous incarnation from our older codebase restored functionality to site news, glossary, profile and logs. The function changed in the most recent release (CVS file v1.3 comment is weekly git snapshot 12/30/2010, but it doesn't reference the tracker??).
        Hide
        Gregor McNish added a comment -

        Looked around a bit more – probably the change is the result of MDL-25321?

        Show
        Gregor McNish added a comment - Looked around a bit more – probably the change is the result of MDL-25321 ?
        Hide
        Luis de Vasconcelos added a comment -

        I installed the official final 2.0.1 release from 25th December 2010 and I don't experience the problem on that version. Then I upgraded to MOODLE_20_WEEKLY and the problem reappears - so it looks like a regression in MOODLE_20_WEEKLY.

        Show
        Luis de Vasconcelos added a comment - I installed the official final 2.0.1 release from 25th December 2010 and I don't experience the problem on that version. Then I upgraded to MOODLE_20_WEEKLY and the problem reappears - so it looks like a regression in MOODLE_20_WEEKLY.
        Hide
        Jeremy Bascom added a comment -

        Thanks for tracking this down some. If I can help with testing fixes, let me know.

        Show
        Jeremy Bascom added a comment - Thanks for tracking this down some. If I can help with testing fixes, let me know.
        Hide
        Luis de Vasconcelos added a comment - - edited

        I'm changing the priority of this one as it really is a show-stopper for us...

        To test further I added a Glossary to a course and I get the same "Error reading from database" message:

        Debug info: SQLState: 42000<br>
        Error Code: 8156<br>
        Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The column 'glossarypivot' was specified multiple times for 'q'.<br>

        SELECT , glossarypivot FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY concept) AS sqlsrvrownumber, *, glossarypivot FROM (SELECT ge., ge.concept AS glossarypivot FROM mdl_glossary_entries ge WHERE (ge.glossaryid = '1' or ge.sourceglossaryid = '1') AND
        (ge.approved <> 0 OR ge.userid = '2')
        ) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 10
        [array (
        0 => '1',
        1 => '1',
        2 => '2',
        )]
        Stack trace:
        line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown
        line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end()
        line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end()
        line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query()
        line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql()
        line 265 of \mod\glossary\sql.php: call to sqlsrv_native_moodle_database->get_records_sql()
        line 371 of \mod\glossary\view.php: call to require()

        When I view a Students profile I get this error:

        Debug info: SQLState: 42000<br>
        Error Code: 8155<br>
        Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]No column was specified for column 1 of 'q'.<br>
        SQLState: 42000<br>
        Error Code: 8155<br>
        Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]No column was specified for column 2 of 'q'.<br>

        SELECT 1 FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY 1) AS sqlsrvrownumber, 1 FROM (SELECT 1
        FROM mdl_role_assignments
        WHERE userid = '3' AND roleid IN (3)) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 1
        [array (
        0 => '3',
        )]
        Stack trace:
        line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown
        line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end()
        line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end()
        line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query()
        line 1553 of \lib\dml\moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql()
        line 825 of \lib\accesslib.php: call to moodle_database->record_exists_sql()
        line 3284 of \lib\navigationlib.php: call to has_coursecontact_role()
        line 3208 of \lib\navigationlib.php: call to settings_navigation->generate_user_settings()
        line 2583 of \lib\navigationlib.php: call to settings_navigation->load_user_settings()
        line 583 of \lib\pagelib.php: call to settings_navigation->initialise()
        line 599 of \lib\pagelib.php: call to moodle_page->magic_get_settingsnav()
        line 118 of \user\profile.php: call to moodle_page->__get()

        And when I go into the "Live logs from the past hour" in a course I get this "Error reading from database":

        Debug info: SQLState: 42000<br>
        Error Code: 8156<br>
        Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The column 'firstname' was specified multiple times for 'q'.<br>

        SELECT , firstname, lastname, picture FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY time DESC) AS sqlsrvrownumber, *, firstname, lastname, picture FROM (SELECT l., u.firstname, u.lastname, u.picture
        FROM mdl_log l
        LEFT JOIN mdl_user u ON l.userid = u.id
        WHERE l.course = '2' AND l.time > '1295417679' AND l.time < '1295504079'
        ) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 500
        [array (
        0 => '2',
        1 => 1295417679,
        2 => 1295504079,
        )]
        Stack trace:
        line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown
        line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end()
        line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end()
        line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query()
        line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql()
        line 1825 of \lib\datalib.php: call to sqlsrv_native_moodle_database->get_records_sql()
        line 303 of \course\lib.php: call to get_logs()
        line 315 of \course\lib.php: call to build_logs_array()
        line 43 of \course\report\log\live.php: call to print_log()

        Should the Moodle codebase be using the evil "SELECT *" syntax at all - especially in queries containing JOINS?

        Show
        Luis de Vasconcelos added a comment - - edited I'm changing the priority of this one as it really is a show-stopper for us... To test further I added a Glossary to a course and I get the same "Error reading from database" message: Debug info: SQLState: 42000<br> Error Code: 8156<br> Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] The column 'glossarypivot' was specified multiple times for 'q'.<br> SELECT , glossarypivot FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY concept) AS sqlsrvrownumber, *, glossarypivot FROM (SELECT ge. , ge.concept AS glossarypivot FROM mdl_glossary_entries ge WHERE (ge.glossaryid = '1' or ge.sourceglossaryid = '1') AND (ge.approved <> 0 OR ge.userid = '2') ) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 10 [array ( 0 => '1', 1 => '1', 2 => '2', )] Stack trace: line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end() line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end() line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query() line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql() line 265 of \mod\glossary\sql.php: call to sqlsrv_native_moodle_database->get_records_sql() line 371 of \mod\glossary\view.php: call to require() When I view a Students profile I get this error: Debug info: SQLState: 42000<br> Error Code: 8155<br> Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] No column was specified for column 1 of 'q'.<br> SQLState: 42000<br> Error Code: 8155<br> Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] No column was specified for column 2 of 'q'.<br> SELECT 1 FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY 1) AS sqlsrvrownumber, 1 FROM (SELECT 1 FROM mdl_role_assignments WHERE userid = '3' AND roleid IN (3)) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 1 [array ( 0 => '3', )] Stack trace: line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end() line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end() line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query() line 1553 of \lib\dml\moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql() line 825 of \lib\accesslib.php: call to moodle_database->record_exists_sql() line 3284 of \lib\navigationlib.php: call to has_coursecontact_role() line 3208 of \lib\navigationlib.php: call to settings_navigation->generate_user_settings() line 2583 of \lib\navigationlib.php: call to settings_navigation->load_user_settings() line 583 of \lib\pagelib.php: call to settings_navigation->initialise() line 599 of \lib\pagelib.php: call to moodle_page->magic_get_settingsnav() line 118 of \user\profile.php: call to moodle_page->__get() And when I go into the "Live logs from the past hour" in a course I get this "Error reading from database": Debug info: SQLState: 42000<br> Error Code: 8156<br> Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] The column 'firstname' was specified multiple times for 'q'.<br> SELECT , firstname, lastname, picture FROM (SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY time DESC) AS sqlsrvrownumber, *, firstname, lastname, picture FROM (SELECT l. , u.firstname, u.lastname, u.picture FROM mdl_log l LEFT JOIN mdl_user u ON l.userid = u.id WHERE l.course = '2' AND l.time > '1295417679' AND l.time < '1295504079' ) AS q) AS q WHERE q.sqlsrvrownumber > 0 AND q.sqlsrvrownumber <= 500 [array ( 0 => '2', 1 => 1295417679, 2 => 1295504079, )] Stack trace: line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end() line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end() line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query() line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql() line 1825 of \lib\datalib.php: call to sqlsrv_native_moodle_database->get_records_sql() line 303 of \course\lib.php: call to get_logs() line 315 of \course\lib.php: call to build_logs_array() line 43 of \course\report\log\live.php: call to print_log() Should the Moodle codebase be using the evil "SELECT *" syntax at all - especially in queries containing JOINS?
        Hide
        Luis de Vasconcelos added a comment -

        When I add a Lesson to a Course I also get the "Error reading from database":

        Debug info: SQLState: 42000<br>
        Error Code: 421<br>
        Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The ntext data type cannot be selected as DISTINCT because it is not comparable.<br>

        SELECT DISTINCT u.id, u.*
        FROM mdl_user u,
        mdl_lesson_attempts a
        WHERE a.lessonid = '1' and
        u.id = a.userid
        ORDER BY u.lastname
        [array (
        0 => '1',
        )]
        Stack trace:

        • line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown
        • line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end()
        • line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end()
        • line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query()
        • line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql()
        • line 63 of \mod\lesson\report.php: call to sqlsrv_native_moodle_database->get_records_sql()
        Show
        Luis de Vasconcelos added a comment - When I add a Lesson to a Course I also get the "Error reading from database": Debug info: SQLState: 42000<br> Error Code: 421<br> Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] The ntext data type cannot be selected as DISTINCT because it is not comparable.<br> SELECT DISTINCT u.id, u.* FROM mdl_user u, mdl_lesson_attempts a WHERE a.lessonid = '1' and u.id = a.userid ORDER BY u.lastname [array ( 0 => '1', )] Stack trace: line 391 of \lib\dml\moodle_database.php: dml_read_exception thrown line 252 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database->query_end() line 363 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->query_end() line 761 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->do_query() line 905 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database->get_recordset_sql() line 63 of \mod\lesson\report.php: call to sqlsrv_native_moodle_database->get_records_sql()
        Hide
        Luis de Vasconcelos added a comment -

        I'm using:

        $CFG->dbtype = 'sqlsrv';
        $CFG->dblibrary = 'native';

        in my config.php file

        Show
        Luis de Vasconcelos added a comment - I'm using: $CFG->dbtype = 'sqlsrv'; $CFG->dblibrary = 'native'; in my config.php file
        Hide
        Eloy Lafuente (stronk7) added a comment -

        Thanks for the report and details, guys.

        I guess this is a regression introduced recently when fixing MDL-25321 (the original function was also breaking a lot of queries). Exclusively using the sqlsrv driver (not the mssql one).

        So I'm moving this to the stable backlog and assigning it straight to Sam... with MAXIMUM priority.

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - Thanks for the report and details, guys. I guess this is a regression introduced recently when fixing MDL-25321 (the original function was also breaking a lot of queries). Exclusively using the sqlsrv driver (not the mssql one). So I'm moving this to the stable backlog and assigning it straight to Sam... with MAXIMUM priority. Ciao
        Hide
        Sam Hemelryk added a comment -

        Hi guys,
        I've just spoken to Martin about this, unfortunately it has missed getting into this sprint and by the looks of it will be looked at in the next sprint.
        I've already done a bit of preliminary testing and have isolated two clear issues with the new code from the fantastic examples you have all posted:

        1. The first issue is because of duplicate column names when we nest the original queries to get the row number. This only happen if one of the fields in the original query was x.* in which case you would end up with * + additional comments, guaranteed to cause problems.
        2. The second problem is numeric constants that get used to check a fields existence in some cases.

        The solution to the second problem is pretty clear cut and easily dealt with by improving the regex that identifies constants to include purely numeric constants.
        The second issue is a little more complex unfortunately, the solution is certainly to remove column dependence during nesting. The reason we currently depend upon columns is because we generate a row number field which in Moodle's case would replace the id as the first column. I think it should be possible however to deal with this within our sqlsrv recordset wrapper which is used for all queries. Eloy I'd like to talk to you about this when I start work on this issue.

        Luis I haven't managed to replicate the issue that resulted in the message "Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]The ntext data type cannot be selected as DISTINCT because it is not comparable.". I'll have a deeper look into this one when I commence work on this issue.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Hi guys, I've just spoken to Martin about this, unfortunately it has missed getting into this sprint and by the looks of it will be looked at in the next sprint. I've already done a bit of preliminary testing and have isolated two clear issues with the new code from the fantastic examples you have all posted: The first issue is because of duplicate column names when we nest the original queries to get the row number. This only happen if one of the fields in the original query was x.* in which case you would end up with * + additional comments, guaranteed to cause problems. The second problem is numeric constants that get used to check a fields existence in some cases. The solution to the second problem is pretty clear cut and easily dealt with by improving the regex that identifies constants to include purely numeric constants. The second issue is a little more complex unfortunately, the solution is certainly to remove column dependence during nesting. The reason we currently depend upon columns is because we generate a row number field which in Moodle's case would replace the id as the first column. I think it should be possible however to deal with this within our sqlsrv recordset wrapper which is used for all queries. Eloy I'd like to talk to you about this when I start work on this issue. Luis I haven't managed to replicate the issue that resulted in the message "Message: [Microsoft] [SQL Server Native Client 10.0] [SQL Server] The ntext data type cannot be selected as DISTINCT because it is not comparable.". I'll have a deeper look into this one when I commence work on this issue. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        Eloy if you have a moment could you please have a look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-26025 and let me know what you think?
        I've implement what I was thinking about above and added some unit tests to explicitly catch this sort of thing.

        Cheers
        Sam

        Show
        Sam Hemelryk added a comment - Eloy if you have a moment could you please have a look at https://github.com/samhemelryk/moodle/compare/master...wip-MDL-26025 and let me know what you think? I've implement what I was thinking about above and added some unit tests to explicitly catch this sort of thing. Cheers Sam
        Hide
        Sam Hemelryk added a comment -

        I was talking to Eloy about this issue this morning, the following was our conversation so that it doesn't get forgotten:

        (09:02:14) samhemelryk: I have a purposed fix, but it involves altering the sqlsrv recordset class to remove the row number field when iterating a recordset.... so I'm not sure whether it may be crossing some boundaries or something
        (09:03:18) stronk7: Silly question, Sam. Does the sqlsrv driver has some seek() of similar to quickly advance the recordsets?
        (09:03:38) samhemelryk: Good question.... no clue
        (09:03:42) stronk7: I was thinking about that the other day. Instead of modifying the SQL...
        (09:04:13) samhemelryk: You think just generate a complete recordset and then speed to the records we are interested in
        (09:05:41) stronk7: yup, that's the way the MSSQL emulated the limit clause. It has a quick-scroll-forward: mssql_data_seek()
        (09:06:02) samhemelryk: Ahhh well that may be a much better option!
        (09:06:21) stronk7: yeah, but surely... it doesn't exist.
        (09:07:59) stronk7: uhm... have you take a look to the oracle impl? It also emulates limits with some subqueries and has been working ok so far... and the original $sql is always unmodified.
        (09:08:55) stronk7: Perhaps you could follow that approach, it's a matter of find the equivalent to rownum in mssql, surely some rank function will provide it
        (09:08:57) stronk7: uhm
        (09:09:33) stronk7: I really think the regexp on sql is a dead end and we always will find some query broken.
        (09:09:42) samhemelryk: Year I agree
        (09:11:18) stronk7: one more suggestion... take a look to adodb's sqlsrv implementation. Sure they also have limit emulation there (and MS did it, like ours, so surely will be faulty, but just in case)
        (09:11:40) samhemelryk: Ok cool thanks for the help
        (09:11:41) stronk7: Perhaps the Oracle alternative is the easier to follow
        (09:12:06) stronk7: (at least visually)

        Show
        Sam Hemelryk added a comment - I was talking to Eloy about this issue this morning, the following was our conversation so that it doesn't get forgotten: (09:02:14) samhemelryk: I have a purposed fix, but it involves altering the sqlsrv recordset class to remove the row number field when iterating a recordset.... so I'm not sure whether it may be crossing some boundaries or something (09:03:18) stronk7: Silly question, Sam. Does the sqlsrv driver has some seek() of similar to quickly advance the recordsets? (09:03:38) samhemelryk: Good question.... no clue (09:03:42) stronk7: I was thinking about that the other day. Instead of modifying the SQL... (09:04:13) samhemelryk: You think just generate a complete recordset and then speed to the records we are interested in (09:05:41) stronk7: yup, that's the way the MSSQL emulated the limit clause. It has a quick-scroll-forward: mssql_data_seek() (09:06:02) samhemelryk: Ahhh well that may be a much better option! (09:06:21) stronk7: yeah, but surely... it doesn't exist. (09:07:59) stronk7: uhm... have you take a look to the oracle impl? It also emulates limits with some subqueries and has been working ok so far... and the original $sql is always unmodified. (09:08:55) stronk7: Perhaps you could follow that approach, it's a matter of find the equivalent to rownum in mssql, surely some rank function will provide it (09:08:57) stronk7: uhm (09:09:33) stronk7: I really think the regexp on sql is a dead end and we always will find some query broken. (09:09:42) samhemelryk: Year I agree (09:11:18) stronk7: one more suggestion... take a look to adodb's sqlsrv implementation. Sure they also have limit emulation there (and MS did it, like ours, so surely will be faulty, but just in case) (09:11:40) samhemelryk: Ok cool thanks for the help (09:11:41) stronk7: Perhaps the Oracle alternative is the easier to follow (09:12:06) stronk7: (at least visually)
        Hide
        Eloy Lafuente (stronk7) added a comment -

        I think your iteration above @ github is better than current one, so +1 for it, note I haven't tested it.

        About the recommendation of the Oracle alternative... well, it's pretty much the same you are using, but cannot be implemented so easily mainly because MSSQL doesn't allow ORDER BY clauses in subselects and you need to put it separately in the rank function (row_number() or whatever it's called).

        Also, I've verified that current php driver doesn't offer scrollable (seek) result sets, so perhaps the only way we have it to continue improving the regexp alternative. I don't like it but...

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - I think your iteration above @ github is better than current one, so +1 for it, note I haven't tested it. About the recommendation of the Oracle alternative... well, it's pretty much the same you are using, but cannot be implemented so easily mainly because MSSQL doesn't allow ORDER BY clauses in subselects and you need to put it separately in the rank function (row_number() or whatever it's called). Also, I've verified that current php driver doesn't offer scrollable (seek) result sets, so perhaps the only way we have it to continue improving the regexp alternative. I don't like it but... Ciao
        Hide
        Eloy Lafuente (stronk7) added a comment -

        One more little thing, in the recordset... why not simply, always:

        unset($row['sqlsrvrownumber']);
        

        does it throw anything if not set?

        Ciao

        Show
        Eloy Lafuente (stronk7) added a comment - One more little thing, in the recordset... why not simply, always: unset($row['sqlsrvrownumber']); does it throw anything if not set? Ciao
        Hide
        Sam Hemelryk added a comment -

        PULL-237 created

        Show
        Sam Hemelryk added a comment - PULL-237 created
        Hide
        David Mudrak added a comment -

        Tested. Thanks for the fix Sam!

        Show
        David Mudrak added a comment - Tested. Thanks for the fix Sam!
        Hide
        Luis de Vasconcelos added a comment -

        Excuse the stupid question, but now that this issue is closed and resolved when will the fix be available for me to download i.e. in Stable?

        Alternatively, how can I test this fix?

        Show
        Luis de Vasconcelos added a comment - Excuse the stupid question, but now that this issue is closed and resolved when will the fix be available for me to download i.e. in Stable? Alternatively, how can I test this fix?
        Hide
        David Mudrak added a comment -

        Luis, the fix is now part of Moodle 2.0.1+ package that is built weekly after the testing is finished. You can download the most recent weekly build from http://download.moodle.org/ and it already contains the fix.

        Show
        David Mudrak added a comment - Luis, the fix is now part of Moodle 2.0.1+ package that is built weekly after the testing is finished. You can download the most recent weekly build from http://download.moodle.org/ and it already contains the fix.

          People

          • Votes:
            0 Vote for this issue
            Watchers:
            2 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: