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
    • Rank:
      29817

      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.]

        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:
            5 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: