Uploaded image for project: 'Moodle'
  1. Moodle
  2. MDL-12427

get_my_courses() broken on MSSQL

    Details

    • Type: Bug
    • Status: Closed
    • Priority: 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

          Attachments

            Activity

            Hide
            syxton 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
            syxton 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
            smithrn 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
            smithrn 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
            martinlanghoff Martín Langhoff added a comment -

            Looking at the breakage...

            Show
            martinlanghoff Martín Langhoff added a comment - Looking at the breakage...
            Hide
            martinlanghoff 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
            martinlanghoff 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
            smithrn Ryan Smith added a comment -

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

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

            NP! Sorry about the breakage.

            Show
            martinlanghoff Martín Langhoff added a comment - NP! Sorry about the breakage.
            Hide
            martinlanghoff 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
            martinlanghoff 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
            martinlanghoff 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
            martinlanghoff 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
            stronk7 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
            stronk7 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
            martinlanghoff 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
            martinlanghoff 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
            martinlanghoff 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
            martinlanghoff 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
            stronk7 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
            stronk7 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
            martinlanghoff 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
            martinlanghoff 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
            stronk7 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
            stronk7 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
            rochford Dennis Rochford added a comment -

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

            Show
            rochford Dennis Rochford added a comment - Perhaps this is because I am using odbc_mssql rather than mssql_n as the database driver?
            Hide
            stronk7 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
            stronk7 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
            stronk7 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
            stronk7 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
            rochford 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
            rochford 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
            stronk7 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
            stronk7 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
            martinlanghoff 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
            martinlanghoff 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
            stronk7 Eloy Lafuente (stronk7) added a comment -

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

            Thanks and ciao

            Show
            stronk7 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:
                  Fix Release Date:
                  11/Jan/08