Moodle
  1. Moodle
  2. MDL-25321

sqlsrv_native's limit_to_top_n function destroys queries completely!

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.0
    • Fix Version/s: 2.0.2
    • Component/s: Database SQL/XMLDB
    • Labels:
    • Database:
      Microsoft SQL
    • Difficulty:
      Moderate
    • Affected Branches:
      MOODLE_20_STABLE
    • Fixed Branches:
      MOODLE_20_STABLE
    • Rank:
      205

      Description

      Hi guys,

      I picked this up the other day, the sqlsrv_native limit_to_top_n function is just destroying SQL queries and just presenting mangled crap.
      A simple way to reproduce this is to create a tet.php file with the following contents:

      Unable to find source-code formatter for language: php. Available languages are: actionscript, html, java, javascript, none, sql, xhtml, xml
      require_once('config.php');
      
      $sql = 'SELECT c.id, c.shortname
              FROM {course} c
              WHERE c.id NOT IN (
                  SELECT cm.course 
                  FROM {course_modules} cm 
                  JOIN {modules} m ON m.id = cm.module
                  WHERE m.name = \'wiki\'
              ) ORDER BY c.shortname ASC';
      $courses = $DB->get_records_sql($sql, null, 5, 5);
      
      echo "<pre>";
      print_r($courses);
      echo "</pre>";
      

      I've also attached a screenshot of what is happening with two queries one that selects the course id and shortname for all courses with limit 5, offset 5 and the second that does that same thing for all courses without a wiki activity.
      The function is returning all columns for the orginal from table and drop all where, group by, and order by statement segments.

      Let me know if there is any more information I can provide.

      Cheers
      Sam

        Issue Links

          Activity

          Sam Hemelryk created issue -
          Sam Hemelryk made changes -
          Field Original Value New Value
          Attachment mssql.test.png [ 22302 ]
          Sam Hemelryk made changes -
          Attachment block_icons.tgz [ 22301 ]
          Hide
          Sam Hemelryk added a comment - - edited

          A little bit more information running the following query through the limit_to_top_n function with limit 5, offset five.
          When entering the method:

          SELECT c.id, c.shortname
          FROM course c
          WHERE c.id NOT IN (
          SELECT cm.course
          FROM course_modules cm
          JOIN modules m ON m.id = cm.id
          WHERE m.name = 'wiki'
          ) ORDER BY c.shortname ASC

          And then when returned by the function:

          SELECT *
          FROM (
          SELECT sub2.*, ROW_NUMBER() OVER(ORDER BY sub2.line2) AS line3
          FROM (
          SELECT 1 AS line2, sub1.*
          FROM course AS sub1
          ) AS sub2
          ) AS sub3
          WHERE line3 BETWEEN 6 AND 10

          Show
          Sam Hemelryk added a comment - - edited A little bit more information running the following query through the limit_to_top_n function with limit 5, offset five. When entering the method: SELECT c.id, c.shortname FROM course c WHERE c.id NOT IN ( SELECT cm.course FROM course_modules cm JOIN modules m ON m.id = cm.id WHERE m.name = 'wiki' ) ORDER BY c.shortname ASC And then when returned by the function: SELECT * FROM ( SELECT sub2.*, ROW_NUMBER() OVER(ORDER BY sub2.line2) AS line3 FROM ( SELECT 1 AS line2, sub1.* FROM course AS sub1 ) AS sub2 ) AS sub3 WHERE line3 BETWEEN 6 AND 10
          Aparup Banerjee made changes -
          Link This issue has been marked as being related by MDL-24862 [ MDL-24862 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Raising this to blocker, IMO we must fix it ASAP, or any query having subqueries and limits will break badly.

          Sam, could you, please, add one test to lib/dml/simpletest/testdml.php, surely to the test_get_records_sql_complicated() method.

          Also, note we have already one test with subqueries and limits @ line 1055, would be great to know if that test is working under sqlsrv driver right now or if we need to modify/add some more there too.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Raising this to blocker, IMO we must fix it ASAP, or any query having subqueries and limits will break badly. Sam, could you, please, add one test to lib/dml/simpletest/testdml.php, surely to the test_get_records_sql_complicated() method. Also, note we have already one test with subqueries and limits @ line 1055, would be great to know if that test is working under sqlsrv driver right now or if we need to modify/add some more there too. Ciao
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.1 [ 10420 ]
          Priority Critical [ 2 ] Blocker [ 1 ]
          Difficulty Moderate
          Database [Microsoft SQL]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.x STABLE backlog [ 10463 ]
          Fix Version/s 2.0.1 [ 10420 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.1 [ 10420 ]
          Fix Version/s 2.0.1 sprint 1 [ 10461 ]
          Fix Version/s 2.0.x STABLE backlog [ 10463 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.x STABLE backlog [ 10463 ]
          Fix Version/s 2.0.1 [ 10420 ]
          Fix Version/s 2.0.1 sprint 1 [ 10461 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.1 [ 10420 ]
          Fix Version/s 2.0.1 sprint 1 [ 10461 ]
          Fix Version/s 2.0.x STABLE backlog [ 10463 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s 2.0.x STABLE backlog [ 10463 ]
          Fix Version/s 2.0.1 sprint 1 [ 10461 ]
          Fix Version/s 2.0.1 [ 10420 ]
          Filter Manager made changes -
          Labels triaged
          Martin Dougiamas made changes -
          Fix Version/s 2.0.1 sprint 1 [ 10461 ]
          Fix Version/s STABLE backlog [ 10463 ]
          Martin Dougiamas made changes -
          Fix Version/s 2.0.1 [ 10420 ]
          Martin Dougiamas made changes -
          Assignee Michael Ketcham [ msketcham ] Sam Hemelryk [ samhemelryk ]
          Sam Hemelryk made changes -
          Status Open [ 1 ] In Progress [ 3 ]
          Hide
          Sam Hemelryk added a comment -

          Hi guys,

          I think I've got a patch for this now, I've got to do a little more testing to find out whether I can find any queries that break it.
          Eloy could you please have a look at this and see what you think, I've also asked Andrew to peer review it in the mean time.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi guys, I think I've got a patch for this now, I've got to do a little more testing to find out whether I can find any queries that break it. Eloy could you please have a look at this and see what you think, I've also asked Andrew to peer review it in the mean time. Cheers Sam
          Show
          Sam Hemelryk added a comment - Sorry forgot the relevant links: Repo - https://github.com/samhemelryk/moodle/tree/wip-MDL-25321 Diff - https://github.com/samhemelryk/moodle/compare/master...wip-MDL-25321
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Thanks Sam,

          I've failed miserably here trying to get + LAMP + sqlsrv driver working here, crap, what a waste of time. So I din't looked your code either, grrr, grrr, sorry!

          Will try tomorrow... ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Thanks Sam, I've failed miserably here trying to get + LAMP + sqlsrv driver working here, crap, what a waste of time. So I din't looked your code either, grrr, grrr, sorry! Will try tomorrow... ciao
          Martin Dougiamas made changes -
          Fix Version/s STABLE sprint 1 [ 10480 ]
          Fix Version/s 2.0.1 [ 10420 ]
          Fix Version/s STABLE sprint 2010-12 [ 10461 ]
          Martin Dougiamas made changes -
          Fix Version/s STABLE sprint A [ 10481 ]
          Fix Version/s STABLE sprint 1 [ 10480 ]
          Hide
          Andrew Davis added a comment - - edited

          re this comment
          // If limit is 0 set it to BITINT - 1
          I assume you mean the sql server BIGINT (with a G). may want to expand the comment to be more clear and maybe include a link such as http://msdn.microsoft.com/en-us/library/ms187745.aspx to give more explanation for where this magic number comes from.

          Youve used that same number again later on.
          $sql = "SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY $orderby) AS sqlsrvrownumber, $columnnames FROM ($sql) AS q";
          Put it in a constant or at least a variable rather than copying and pasting it around. Otherwise you have to copy and paste the explanation as well otherwise, upon seeing "select top (long random looking number)" someone in the future will come along, not understand its meaning and break it.

          I cant see anything further wrong but this NEEDS unit tests. Given its complexity the chances of this being entirely correct are pretty slim and the potential impact of bugs in the DML code is pretty catastrophic. If the testing infrastructure in this area needs enhancing then that should be done. Just my opinion of course but personally I'd like to see a battery of unit tests that verify this thing is correct.

          Show
          Andrew Davis added a comment - - edited re this comment // If limit is 0 set it to BITINT - 1 I assume you mean the sql server BIGINT (with a G). may want to expand the comment to be more clear and maybe include a link such as http://msdn.microsoft.com/en-us/library/ms187745.aspx to give more explanation for where this magic number comes from. Youve used that same number again later on. $sql = "SELECT TOP 9223372036854775806 ROW_NUMBER() OVER(ORDER BY $orderby) AS sqlsrvrownumber, $columnnames FROM ($sql) AS q"; Put it in a constant or at least a variable rather than copying and pasting it around. Otherwise you have to copy and paste the explanation as well otherwise, upon seeing "select top (long random looking number)" someone in the future will come along, not understand its meaning and break it. I cant see anything further wrong but this NEEDS unit tests. Given its complexity the chances of this being entirely correct are pretty slim and the potential impact of bugs in the DML code is pretty catastrophic. If the testing infrastructure in this area needs enhancing then that should be done. Just my opinion of course but personally I'd like to see a battery of unit tests that verify this thing is correct.
          Hide
          Sam Hemelryk added a comment -

          Hi Andrew,

          Thanks for looking at that, I will indeed put the big int into a var within that function.
          Regarding the unit tests I was talking to Eloy about that yesterday, the unit tests that area already there cover the angles of that function fantastically it just appears either no one tested them on sql server or forgot to report the mass failure

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Andrew, Thanks for looking at that, I will indeed put the big int into a var within that function. Regarding the unit tests I was talking to Eloy about that yesterday, the unit tests that area already there cover the angles of that function fantastically it just appears either no one tested them on sql server or forgot to report the mass failure Cheers Sam
          Martin Dougiamas made changes -
          Workflow jira [ 40362 ] MDL Workflow [ 47604 ]
          Sam Hemelryk made changes -
          Link This issue will help resolve PULL-27 [ PULL-27 ]
          Hide
          Sam Hemelryk added a comment -

          Pull request created PULL-27

          Show
          Sam Hemelryk added a comment - Pull request created PULL-27
          Sam Hemelryk made changes -
          Status In Progress [ 3 ] Ready for review [ 10010 ]
          Resolution Fixed [ 1 ]
          Martin Dougiamas made changes -
          Fix Version/s STABLE Sprint 2 [ 10482 ]
          Fix Version/s STABLE sprint 1 [ 10481 ]
          Hide
          Sam Hemelryk added a comment -

          Reopening as it failed the pull request, the bigint var probably needs to be a string rather than an int as Petr mentions.

          Show
          Sam Hemelryk added a comment - Reopening as it failed the pull request, the bigint var probably needs to be a string rather than an int as Petr mentions.
          Sam Hemelryk made changes -
          Resolution Fixed [ 1 ]
          Status Ready for review [ 10010 ] Reopened [ 4 ]
          Sam Hemelryk made changes -
          Status Reopened [ 4 ] In Progress [ 3 ]
          Sam Hemelryk made changes -
          Link This issue will help resolve PULL-46 [ PULL-46 ]
          Hide
          Sam Hemelryk added a comment -

          PULL-46 has been created for this issue.

          Show
          Sam Hemelryk added a comment - PULL-46 has been created for this issue.
          Sam Hemelryk made changes -
          Status In Progress [ 3 ] Ready for review [ 10010 ]
          Resolution Fixed [ 1 ]
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Closing as fixed. Will land to upstream in hours.

          I'd happy with time to try to break this with some crazy queries but haven't been able to so, based in code already passing DML tests... this is done.

          Thanks Sam! Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Closing as fixed. Will land to upstream in hours. I'd happy with time to try to break this with some crazy queries but haven't been able to so, based in code already passing DML tests... this is done. Thanks Sam! Ciao
          Eloy Lafuente (stronk7) made changes -
          Status Ready for review [ 10010 ] Closed [ 6 ]
          Fix Version/s 2.0.2 [ 10421 ]
          Hide
          Gregor McNish added a comment -

          Would this change have caused the issues in MDL-26025?

          Show
          Gregor McNish added a comment - Would this change have caused the issues in MDL-26025 ?
          Eloy Lafuente (stronk7) made changes -
          Link This issue is duplicated by MDL-26325 [ MDL-26325 ]
          Martin Dougiamas made changes -
          Workflow MDL Workflow [ 47604 ] MDL Full Workflow [ 95617 ]
          Michael de Raadt made changes -
          Component/s Database SQL/XMLDB [ 10131 ]
          Component/s Database MS SQL [ 10633 ]
          Eloy Lafuente (stronk7) made changes -
          Fix Version/s STABLE Sprint 2 [ 10482 ]

            People

            • Votes:
              3 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: