Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Not a bug
    • Affects Version/s: 1.8.3
    • Fix Version/s: 1.8.4
    • Component/s: Roles / Access
    • Labels:
      None
    • Database:
      Microsoft SQL
    • Affected Branches:
      MOODLE_18_STABLE
    • Fixed Branches:
      MOODLE_18_STABLE

      Description

      One of the SQL queries in gmc is concatenating ctx.path with an id field. MSSQL barfs at the idea of casting the numeric id.

      Credit for the fix to Dennis Rochford <Dennis.Rochford@usq.edu.au>

      [I initially misinterpreted the patch - thinking that the implicit CAST caused Oracle to complain. It is MSSQL that complains about it.]

        Gliffy Diagrams

          Activity

          Hide
          Matthew Davidson added a comment -

          I believe this fix just broke ours. No My Courses block. The profile doesn't show courses either. We are running MySQL. Our dev 1.9 works fine.....but our live 1.8 is broken now.

          Show
          Matthew Davidson added a comment - I believe this fix just broke ours. No My Courses block. The profile doesn't show courses either. We are running MySQL. Our dev 1.9 works fine.....but our live 1.8 is broken now.
          Hide
          Ryan Smith added a comment -

          Yeah, with the updated lib/datalib.php no Courses: section shows up in user profiles. It is completely gone.

          Show
          Ryan Smith added a comment - Yeah, with the updated lib/datalib.php no Courses: section shows up in user profiles. It is completely gone.
          Hide
          Martín Langhoff added a comment -

          Looking at the breakage...

          Show
          Martín Langhoff added a comment - Looking at the breakage...
          Hide
          Martín Langhoff added a comment -

          Diagnosed and fixed. Can you confirm that it works for you now (note that you may have to wait a few hs for the commit to come through - you want datalib rev 1.365.2.31 )

          From the commit msg:
          MySQL does not like CAST()s to VARCHAR. Change it to a CHAR() – note
          the lack of length definition – which will do the cast without any
          padding. That's what we are after, anyway.

          Show
          Martín Langhoff added a comment - Diagnosed and fixed. Can you confirm that it works for you now (note that you may have to wait a few hs for the commit to come through - you want datalib rev 1.365.2.31 ) From the commit msg: MySQL does not like CAST()s to VARCHAR. Change it to a CHAR() – note the lack of length definition – which will do the cast without any padding. That's what we are after, anyway.
          Hide
          Ryan Smith added a comment -

          I just updated from CVS and the latest datalib works great. Thanks for the quick fix Martin!

          Show
          Ryan Smith added a comment - I just updated from CVS and the latest datalib works great. Thanks for the quick fix Martin!
          Hide
          Martín Langhoff added a comment -

          NP! Sorry about the breakage.

          Show
          Martín Langhoff added a comment - NP! Sorry about the breakage.
          Hide
          Martín Langhoff added a comment -

          Reopening - the fix for mysql breaks Oracle again. And adding Eloy - CAST() to CHAR cross-db is the devil!

          Note: this is important wrt Oracle support in 1.9 because the new accesslib uses similar code to this in many functions in the hot path – which is to say, we need this to work fast and reliably on the DB side.

          Let's see

          • Oracle barfs on implicit cast to char as in

          SELECT id || 'foo' FROM m_config;

          it wants an explicit cast, like CAST(id AS CHAR(10)) or as VARCHAR(length). Oracle requires the length there – and will pad to that length. If we provide the length, we end up with

          SELECT CAST(id AS CHAR(10) || 'foo' FROM m_config; // problem - will be space-padded on Pg and Oracle (not on mysql!)
          SELECT CAST(id AS VARCHAR(10) || 'foo' FROM m_config; // problem - will error out on MySQL

          and in either case, we may exceed the 10 chars

          SELECT CAST(id AS CHAR) || 'foo' FROM m_config; // works in MySQL & Pg, barfs on Oracle

          So far, the only cross-db solution I managed to get is

          SELECT TRIM(CAST(id AS CHAR(10))) || 'foo' FROM m_config; // works in MySQL & Pg & Oracle - sems to have a minor perf impact on Pg

          Still untested on MSSQL – which I don't have.

          So for 1.9 I think we need a sql_castaschar($sqlexpr). Was going to add a $pad variable but mysql does not support that at all. See bug http://bugs.mysql.com/bug.php?id=24424 and have a good laugh.

          Show
          Martín Langhoff added a comment - Reopening - the fix for mysql breaks Oracle again. And adding Eloy - CAST() to CHAR cross-db is the devil! Note: this is important wrt Oracle support in 1.9 because the new accesslib uses similar code to this in many functions in the hot path – which is to say, we need this to work fast and reliably on the DB side. Let's see Oracle barfs on implicit cast to char as in SELECT id || 'foo' FROM m_config; it wants an explicit cast, like CAST(id AS CHAR(10)) or as VARCHAR(length). Oracle requires the length there – and will pad to that length. If we provide the length, we end up with SELECT CAST(id AS CHAR(10) || 'foo' FROM m_config; // problem - will be space-padded on Pg and Oracle (not on mysql!) SELECT CAST(id AS VARCHAR(10) || 'foo' FROM m_config; // problem - will error out on MySQL and in either case, we may exceed the 10 chars SELECT CAST(id AS CHAR) || 'foo' FROM m_config; // works in MySQL & Pg, barfs on Oracle So far, the only cross-db solution I managed to get is SELECT TRIM(CAST(id AS CHAR(10))) || 'foo' FROM m_config; // works in MySQL & Pg & Oracle - sems to have a minor perf impact on Pg Still untested on MSSQL – which I don't have. So for 1.9 I think we need a sql_castaschar($sqlexpr). Was going to add a $pad variable but mysql does not support that at all. See bug http://bugs.mysql.com/bug.php?id=24424 and have a good laugh.
          Hide
          Martín Langhoff added a comment -

          Now clarified with Dennis – the implicit cast works Oracle, but breaks on MSSQL.

          So MySQL, Oracle and Pg all like:

          SELECT id || 'foo' FROM m_config;

          MSSQL and Oracle support this:

          SELECT CAST(id AS VARCHAR(10)) || 'foo'

          Show
          Martín Langhoff added a comment - Now clarified with Dennis – the implicit cast works Oracle, but breaks on MSSQL. So MySQL, Oracle and Pg all like: SELECT id || 'foo' FROM m_config; MSSQL and Oracle support this: SELECT CAST(id AS VARCHAR(10)) || 'foo' –
          Hide
          Eloy Lafuente (stronk7) added a comment -

          ah, that has more sense. Anyway, I'm going to build one:

          sql_casttochar() function to provide cross-db, oki? Only MSSQL will do the explicit CAST() oki?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - ah, that has more sense. Anyway, I'm going to build one: sql_casttochar() function to provide cross-db, oki? Only MSSQL will do the explicit CAST() oki? Ciao
          Hide
          Martín Langhoff added a comment -

          Hey - we are trying one more theory with dennis - I just emailed him...

          Dennis wrote -

          With SQL Server the statement comes out as:

          SELECT id,name,value, id +'x' as foo FROM mdl_config

          Which gives the following error:

          Error converting data type varchar to bigint.

          MartinL's response -

          how about this theory?: The problem we are having with MSSQL is that the
          + operand is ambiguous, and is looking at the left-most element as a
          hint for whether is a numeric addition or a string concat. The error
          that you've shown me indicated that 'foo' is getting casted to a bigint.

          I'll bet that MSSQL lets the leftmost operand define the casting - like
          most type-dynamic languages do.

          So

          // this errors out
          SELECT id + 'foo' FROM m_config;

          // this should work
          SELECT 'foo'+id FROM m_config;

          // Therefore this should work
          SELECT '' + id + 'foo' FROM m_config;

          Does MSSQL agree with my crazy theory? If yes, we can just patch
          sql_concat() to add a leading ''+ if the DB is MSSQL.

          Show
          Martín Langhoff added a comment - Hey - we are trying one more theory with dennis - I just emailed him... Dennis wrote - With SQL Server the statement comes out as: SELECT id,name,value, id +'x' as foo FROM mdl_config Which gives the following error: Error converting data type varchar to bigint. MartinL's response - how about this theory?: The problem we are having with MSSQL is that the + operand is ambiguous, and is looking at the left-most element as a hint for whether is a numeric addition or a string concat. The error that you've shown me indicated that 'foo' is getting casted to a bigint. I'll bet that MSSQL lets the leftmost operand define the casting - like most type-dynamic languages do. So // this errors out SELECT id + 'foo' FROM m_config; // this should work SELECT 'foo'+id FROM m_config; // Therefore this should work SELECT '' + id + 'foo' FROM m_config; Does MSSQL agree with my crazy theory? If yes, we can just patch sql_concat() to add a leading ''+ if the DB is MSSQL.
          Hide
          Martín Langhoff added a comment -

          hmmm. Doesn't work - looks like we'll need that sql_casttochar()

          Here is a link that explains what SQL Server does when combining multiple data types:
          http://msdn2.microsoft.com/en-us/library/aa258264(SQL.80).aspx

          Show
          Martín Langhoff added a comment - hmmm. Doesn't work - looks like we'll need that sql_casttochar() Here is a link that explains what SQL Server does when combining multiple data types: http://msdn2.microsoft.com/en-us/library/aa258264(SQL.80).aspx
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry Martín, but here there is something that I cannot understand.

          I was building and testing the sql_casttochar() function when I detected that, for MSSQL drivers, the sql_concat() function itself, that internally calls adodb-mssql.inc.php uses to, AUTOMATICALLY, cast every parameter to varchar.

          So it seems that we don't need the sql_casttochar() function, at least when using the sql_concat() function.

          Any insight ?

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry Martín, but here there is something that I cannot understand. I was building and testing the sql_casttochar() function when I detected that, for MSSQL drivers, the sql_concat() function itself, that internally calls adodb-mssql.inc.php uses to, AUTOMATICALLY, cast every parameter to varchar. So it seems that we don't need the sql_casttochar() function, at least when using the sql_concat() function. Any insight ?
          Hide
          Martín Langhoff added a comment -

          Hmmm, odd. This is a patch that Dennis had in his working tree, and he was saying that MSSQL broke for him without it (on moodle v1.8). I don't have MSSQL so I took the patch and worked it "blind" other than asking Dennis to test a few things.

          Perhaps AdoDB changes between moodle v1.8 and 1.9 change the CAST handling? I did a diff and it didn't seem likely. Perhaps it's all a bit of confusion! I'll ask Dennis to sub to the bug and comment.

          (My worry is that the new accesslib has similar concat/casts everywhere.)

          Show
          Martín Langhoff added a comment - Hmmm, odd. This is a patch that Dennis had in his working tree, and he was saying that MSSQL broke for him without it (on moodle v1.8). I don't have MSSQL so I took the patch and worked it "blind" other than asking Dennis to test a few things. Perhaps AdoDB changes between moodle v1.8 and 1.9 change the CAST handling? I did a diff and it didn't seem likely. Perhaps it's all a bit of confusion! I'll ask Dennis to sub to the bug and comment. (My worry is that the new accesslib has similar concat/casts everywhere.)
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well, I've MMSQL running here and I should be getting errors everywhere (due to the accesslib concat/cast), but I not!

          So perhaps... this is vaporbug?

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Well, I've MMSQL running here and I should be getting errors everywhere (due to the accesslib concat/cast), but I not! So perhaps... this is vaporbug? Ciao
          Hide
          Dennis Rochford added a comment -

          Perhaps this is because I am using odbc_mssql rather than mssql_n as the database driver?

          Show
          Dennis Rochford added a comment - Perhaps this is because I am using odbc_mssql rather than mssql_n as the database driver?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Ah, odbc_mssql, for sure that's the cause!

          the odbc_mssql driver hasn't the Concat function defined, so it uses the default, that's a simple "+". So, for sure, it's the cause of the problem.

          In any case, note that the odbc_mssql is experimental (from Moodle's perspective) and it isn't "gold supported" by ADOdb so it shouldn't be used for production environments at all (nor, from my experience, for testing, due to the lack of a lot of "cross-db" artifacts available in other drivers).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Ah, odbc_mssql, for sure that's the cause! the odbc_mssql driver hasn't the Concat function defined, so it uses the default, that's a simple "+". So, for sure, it's the cause of the problem. In any case, note that the odbc_mssql is experimental (from Moodle's perspective) and it isn't "gold supported" by ADOdb so it shouldn't be used for production environments at all (nor, from my experience, for testing, due to the lack of a lot of "cross-db" artifacts available in other drivers). Ciao
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Important note: I've reverted your patch leaving the SQL as used to be (without the CAST) in 18_STABLE, because it was breaking MySQL too.

          http://moodle.org/mod/forum/discuss.php?d=86238

          I only have performed the change in 18_STABLE because is wasn't ever committed to 19_STABLE and HEAD (i think).

          Also, Martín, could you please, analyze and merge 18_STABLE to 19_STABLE and HEAD? It seems that there are a bunch of unmerged changes there (in the get_my_courses() function) and I don't know at all what's the correct part to leave.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Important note: I've reverted your patch leaving the SQL as used to be (without the CAST) in 18_STABLE, because it was breaking MySQL too. http://moodle.org/mod/forum/discuss.php?d=86238 I only have performed the change in 18_STABLE because is wasn't ever committed to 19_STABLE and HEAD (i think). Also, Martín, could you please, analyze and merge 18_STABLE to 19_STABLE and HEAD? It seems that there are a bunch of unmerged changes there (in the get_my_courses() function) and I don't know at all what's the correct part to leave. Ciao
          Hide
          Dennis Rochford added a comment -

          No problem, sorry about the confusion. I've been using the odbc version because the others give installation errors (unicode collation issues with ntext - using SQL Server 2005). Not to worry, might have to use mysql or postgres for local development instead of mssql.

          Thanks

          Show
          Dennis Rochford added a comment - No problem, sorry about the confusion. I've been using the odbc version because the others give installation errors (unicode collation issues with ntext - using SQL Server 2005). Not to worry, might have to use mysql or postgres for local development instead of mssql. Thanks
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          No problem Dennis,

          in fact I think its good to have people using mssql for local development (will make bugs easier to discover). It's only that the installation is a bit more complex (have to use the mssql_n adodb driver and some of FreeTDS/ODBTP drivers to connect to the DB, in order to have UTF-8 working).

          http://docs.moodle.org/en/Installing_MSSQL_for_PHP

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited No problem Dennis, in fact I think its good to have people using mssql for local development (will make bugs easier to discover). It's only that the installation is a bit more complex (have to use the mssql_n adodb driver and some of FreeTDS/ODBTP drivers to connect to the DB, in order to have UTF-8 working). http://docs.moodle.org/en/Installing_MSSQL_for_PHP Ciao
          Hide
          Martín Langhoff added a comment -

          Hi Eloy! Thanks for your patience – and sorry about the confusion.

          The merged tag was alright, this "fix" (though wrong) only applied to 18_STABLE because get_my_courses() was rewritten bigtime as part of the accesslib rewrite. Right now the only unmerged changes I find for datalib are your reverts, and they don't need merging, so I've moved the merged tag. Seems like the right thing to do, anyway

          Closing - not-a-bug!

          Show
          Martín Langhoff added a comment - Hi Eloy! Thanks for your patience – and sorry about the confusion. The merged tag was alright, this "fix" (though wrong) only applied to 18_STABLE because get_my_courses() was rewritten bigtime as part of the accesslib rewrite. Right now the only unmerged changes I find for datalib are your reverts, and they don't need merging, so I've moved the merged tag. Seems like the right thing to do, anyway Closing - not-a-bug!
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Great! I love no-bug bugs! :-P

          Thanks and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Great! I love no-bug bugs! :-P Thanks and ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: