Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 1.9
    • Fix Version/s: 1.9
    • Component/s: Chat
    • Labels:
      None
    • Affected Branches:
      MOODLE_19_STABLE
    • Fixed Branches:
      MOODLE_19_STABLE
    • Rank:
      30103

      Description

      the problem is again:
      You can't specify target table 'mdl_chat_messages' for update in FROM clause - 243 of mod/chat/lib.php

      $sql = "SELECT m.id
      FROM {$CFG->prefix}chat_messages m
      JOIN {$CFG->prefix}chat c
      ON m.chatid = c.id
      WHERE c.keepdays != 0
      AND m.timestamp < ( ".time()." - c.keepdays * 24 * 3600)";

      delete_records_select("chat_messages", "id IN ($sql)");

      return true;

      solution: rewrite in portable SQL

        Issue Links

          Activity

          Hide
          Petr Škoda added a comment - - edited

          /// Delete old messages
          $subselect = "SELECT c.keepdays
          FROM {$CFG->prefix}chat c
          WHERE c.id = {$CFG->prefix}chat_messages.chatid AND c.keepdays > 0";

          $sql = "DELETE
          FROM {$CFG->prefix}chat_messages
          WHERE timestamp < ( ".time()." - COALESCE(($subselect, 999999)) * 24 * 3600)";

          would this work?

          Show
          Petr Škoda added a comment - - edited /// Delete old messages $subselect = "SELECT c.keepdays FROM {$CFG->prefix}chat c WHERE c.id = {$CFG->prefix}chat_messages.chatid AND c.keepdays > 0"; $sql = "DELETE FROM {$CFG->prefix}chat_messages WHERE timestamp < ( ".time()." - COALESCE(($subselect, 999999)) * 24 * 3600)"; would this work?
          Hide
          Petr Škoda added a comment -

          sending patch - works fine for me in mysql...

          Show
          Petr Škoda added a comment - sending patch - works fine for me in mysql...
          Hide
          Petr Škoda added a comment -

          committed, please review and reopen if needed...

          Show
          Petr Škoda added a comment - committed, please review and reopen if needed...
          Hide
          Eric Merrill added a comment -

          On line 214 of mod/chat/lib.php, there is the statement:
          ...COALESCE(($keepdays), 999999) * 24 * 3600)";...
          The coalesce select the first non-null, so if $keepdays only returns null, it selects 999999 and the number of days. This get multiplied out to seconds (86399913600), but that number is bigger than an integer in postgres, so it throws and error and that sql dies.

          A postgres int is 32 bit signed (+-2147483647), so the closest you can get it 2147483647/3600/24 = 24855.134803 ~= 24855.

          So I think that 999999 should be replaced by 24855 on that line, to keep the int from overflowing.

          -eric

          Show
          Eric Merrill added a comment - On line 214 of mod/chat/lib.php, there is the statement: ...COALESCE(($keepdays), 999999) * 24 * 3600)";... The coalesce select the first non-null, so if $keepdays only returns null, it selects 999999 and the number of days. This get multiplied out to seconds (86399913600), but that number is bigger than an integer in postgres, so it throws and error and that sql dies. A postgres int is 32 bit signed (+-2147483647), so the closest you can get it 2147483647/3600/24 = 24855.134803 ~= 24855. So I think that 999999 should be replaced by 24855 on that line, to keep the int from overflowing. -eric
          Hide
          Eric Merrill added a comment -

          I opened MDL-12823 and then saw this one. The patch for this created the new bug.

          Show
          Eric Merrill added a comment - I opened MDL-12823 and then saw this one. The patch for this created the new bug.
          Hide
          Martín Langhoff added a comment -

          It might be better to divide time() instead of using very large ints (and getting into portability problems)... ie:

          WHERE timestamp < ( " . (time() / (24*3600) ) ." - COALESCE(($SUBSELECT, 999999))";

          Show
          Martín Langhoff added a comment - It might be better to divide time() instead of using very large ints (and getting into portability problems)... ie: WHERE timestamp < ( " . (time() / (24*3600) ) ." - COALESCE(($SUBSELECT, 999999))";
          Hide
          Eric Merrill added a comment -

          Maybe my thinking is off here, but I think that will cause use to 'lose accuracy'. Right now were delete chats exactly x days after they occur (to closest next cron). Changing time to number of days, we would delete on the days that x is met (at midnight).

          Also, you would have to multiply the result of the entire statement back out to seconds, because you are compairing against timestamp, which is in seconds.

          Don't get me wrong, my solution feels 'wrong' also. Maybe we need to look at this all from the start again.

          Show
          Eric Merrill added a comment - Maybe my thinking is off here, but I think that will cause use to 'lose accuracy'. Right now were delete chats exactly x days after they occur (to closest next cron). Changing time to number of days, we would delete on the days that x is met (at midnight). Also, you would have to multiply the result of the entire statement back out to seconds, because you are compairing against timestamp, which is in seconds. Don't get me wrong, my solution feels 'wrong' also. Maybe we need to look at this all from the start again.
          Hide
          Petr Škoda added a comment -

          what if we used the subselect twice instead of coalesce?

          Show
          Petr Škoda added a comment - what if we used the subselect twice instead of coalesce?
          Hide
          Petr Škoda added a comment -

          subselect not null and timestamp comparison

          Show
          Petr Škoda added a comment - subselect not null and timestamp comparison
          Hide
          Eric Merrill added a comment -

          Do you mean do the sub and make sure the result is not null, and then if != null then run the full statement, but with 'WHERE timestamp < ( ".time()." -($subselect) * 24 * 3600)'?

          If so, that seems to make sense for me.

          Show
          Eric Merrill added a comment - Do you mean do the sub and make sure the result is not null, and then if != null then run the full statement, but with 'WHERE timestamp < ( ".time()." -($subselect) * 24 * 3600)'? If so, that seems to make sense for me.
          Hide
          Petr Škoda added a comment -

          something like that, subselect results should be cached, so the perf should be ok, right?

          Show
          Petr Škoda added a comment - something like that, subselect results should be cached, so the perf should be ok, right?
          Hide
          Eric Merrill added a comment -

          I think they performance penalty should be pretty light, since it is on mdl_chat which is considerably smaller than mdl_chat_messages, and being run back to back, even if the result isn't cached by the db, all the data should be readily available in short term memory.

          Really the penalty is the general penalty of having 2 query's instead of 1, so whatever overhead that brings to the table. I think since this is a cron function, and we aren't talking an iterative penalty, i would think it's acceptable, and it's not like this is the first place there has had to be a minor (and i really think very minor) performance sacrifice for cross-db compatibility.

          Show
          Eric Merrill added a comment - I think they performance penalty should be pretty light, since it is on mdl_chat which is considerably smaller than mdl_chat_messages, and being run back to back, even if the result isn't cached by the db, all the data should be readily available in short term memory. Really the penalty is the general penalty of having 2 query's instead of 1, so whatever overhead that brings to the table. I think since this is a cron function, and we aren't talking an iterative penalty, i would think it's acceptable, and it's not like this is the first place there has had to be a minor (and i really think very minor) performance sacrifice for cross-db compatibility.
          Hide
          Martín Langhoff added a comment -

          But I don't think 2 queries are ever necessary - the change in the WHERE clause I mentioned above is ok – if we are keeping 30 days, I don't think the lost resolution changes anything – we'd just delete them at midnight UTC which we could fix.

          OTOH, the weird 999999 just means 'don't delete' so we should get rid of it – that's easier:

          WHERE ($subselect) > 0 AND timestamp < ( ".time()." - ($subselect) * 24 * 3600)";

          the ($subselect) > 0 will take care of the nulls that COALESCE() was addressing before. We don't ever need to touch the other records.

          Show
          Martín Langhoff added a comment - But I don't think 2 queries are ever necessary - the change in the WHERE clause I mentioned above is ok – if we are keeping 30 days, I don't think the lost resolution changes anything – we'd just delete them at midnight UTC which we could fix. OTOH, the weird 999999 just means 'don't delete' so we should get rid of it – that's easier: WHERE ($subselect) > 0 AND timestamp < ( ".time()." - ($subselect) * 24 * 3600)"; the ($subselect) > 0 will take care of the nulls that COALESCE() was addressing before. We don't ever need to touch the other records.
          Hide
          Eric Merrill added a comment -

          Well, Im more just saying that it's a change from how it behaves now. But I do see 2 problems with having it happen once per day. First, the behavior will seem pretty arbitrary to the end user, since it happens at 00:00GMT, which means it will vary in time of day depending on time zone. Second, instead of deleting a handful of messages every cron as they expire, you are doing one large delete per day, which could be a problem performance wise.

          WHERE ($subselect) > 0 AND timestamp < ( ".time()." - ($subselect) * 24 * 3600) still wouldn't help where $subselect is no rows (NULL) (ie if you have no chat instances).

          Are SQL AND statements reliably short circuiting? ie could you do
          WHERE ($subselect) IS NOT NULL AND timestamp < ( ".time()." - ($subselect) * 24 * 3600)

          Show
          Eric Merrill added a comment - Well, Im more just saying that it's a change from how it behaves now. But I do see 2 problems with having it happen once per day. First, the behavior will seem pretty arbitrary to the end user, since it happens at 00:00GMT, which means it will vary in time of day depending on time zone. Second, instead of deleting a handful of messages every cron as they expire, you are doing one large delete per day, which could be a problem performance wise. WHERE ($subselect) > 0 AND timestamp < ( ".time()." - ($subselect) * 24 * 3600) still wouldn't help where $subselect is no rows (NULL) (ie if you have no chat instances). Are SQL AND statements reliably short circuiting? ie could you do WHERE ($subselect) IS NOT NULL AND timestamp < ( ".time()." - ($subselect) * 24 * 3600)
          Hide
          Eric Merrill added a comment -

          I tested this on MySQL and pgsql, and it seemed to work, including a no instances case:

           $subselect = "SELECT c.keepdays
                              FROM {$CFG->prefix}chat c
                             WHERE c.id = {$CFG->prefix}chat_messages.chatid";

              $sql = "DELETE
                        FROM {$CFG->prefix}chat_messages
                       WHERE ($subselect) > 0 AND timestamp < ( ".time()." -($subselect) * 24 * 3600)";

          Show
          Eric Merrill added a comment - I tested this on MySQL and pgsql, and it seemed to work, including a no instances case:  $subselect = "SELECT c.keepdays                     FROM {$CFG->prefix}chat c                    WHERE c.id = {$CFG->prefix}chat_messages.chatid";     $sql = "DELETE               FROM {$CFG->prefix}chat_messages              WHERE ($subselect) > 0 AND timestamp < ( ".time()." -($subselect) * 24 * 3600)";
          Hide
          Martín Langhoff added a comment -

          Great - if that tests ok, then we're sorted IMO

          Show
          Martín Langhoff added a comment - Great - if that tests ok, then we're sorted IMO
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Sorry but I think that those type of complex queries aren't the way to do it:

          Why not, simply create one recordset for this:

          SELECT c.id, c.keepdays
          FROM {$CFG->prefix}chat c
          WHERE c.keepdays > 0

          and inside the loop, perform a simple:

          $cutoff = time() - ( $recordset->keepdays * 24 * 3600);

          DELETE FROM {$CFG->prefix}chat_messages cm
          WHERE cm.chatid = $recordset->id AND cm.timestamp < $cutoff

          IMO it's more readable and cross-db. And I don't think it will be significantly slower (perhaps under thousands of chats but...).

          Just my opinion, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Sorry but I think that those type of complex queries aren't the way to do it: Why not, simply create one recordset for this: SELECT c.id, c.keepdays FROM {$CFG->prefix}chat c WHERE c.keepdays > 0 and inside the loop, perform a simple: $cutoff = time() - ( $recordset->keepdays * 24 * 3600); DELETE FROM {$CFG->prefix}chat_messages cm WHERE cm.chatid = $recordset->id AND cm.timestamp < $cutoff IMO it's more readable and cross-db. And I don't think it will be significantly slower (perhaps under thousands of chats but...). Just my opinion, ciao
          Hide
          Petr Škoda added a comment -

          there might be a problem with NULL > 0, maybe ($subselect) NOT NULL would be more compatible - what do you think Eloy?

          Show
          Petr Škoda added a comment - there might be a problem with NULL > 0, maybe ($subselect) NOT NULL would be more compatible - what do you think Eloy?
          Hide
          Eric Merrill added a comment -

          Eloy -

          I agree with you, but there seem to be differing opinions as to if we should be more efficient (Martin L) or more readable/simple (Petr, Eloy).

          Petr -

          I tried it on a table set with with no mdl_chat entries (in both my and pg). Is there a different case where that could be null?

          Show
          Eric Merrill added a comment - Eloy - I agree with you, but there seem to be differing opinions as to if we should be more efficient (Martin L) or more readable/simple (Petr, Eloy). Petr - I tried it on a table set with with no mdl_chat entries (in both my and pg). Is there a different case where that could be null?
          Hide
          Eloy Lafuente (stronk7) added a comment -

          NULL isn't comparable AFAIK so any >, < = against NULL should return FALSE (unless some DB is doing illegal things with NULLs)

          i.e. something like this:

          select * from table where NULL > 0 or NULL < 0 or NULL = 0;

          Should return ZERO records (it's always false).

          Anyway I continue preferring the recordset + simple delete way.

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - NULL isn't comparable AFAIK so any >, < = against NULL should return FALSE (unless some DB is doing illegal things with NULLs) i.e. something like this: select * from table where NULL > 0 or NULL < 0 or NULL = 0; Should return ZERO records (it's always false). Anyway I continue preferring the recordset + simple delete way. Ciao
          Hide
          Martín Langhoff added a comment -

          The NULL thing that Petr mentions has two cases:

          • NULL in a numeric < comparison will always be "smaller". This is safe, and is what we are doing above.
          • String ordering/sorting, where in some LOCALEs NULL sorts first, and in others, it sorts last. This is usually not a DB issue, but an OS issue. OSX sorts differently from most linuxen under the utf8 locales, for example. (This is something to be wary of, and what Petr is thinking about – but we aren't sorting it string-wise)

          Eloy, in terms of performance, what you are proposing is exactly the code I replaced :-/ – on a moderately sized DB it was causing the cron run to do 600 DB queries. See http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=b78f4cbd31b5cfea3cddfea0b39891b785a34156 – it is wasteful to do 600 pointless DB queries for this when we can do 1. And we do need to handle complex SQL to be able to scale.

          Show
          Martín Langhoff added a comment - The NULL thing that Petr mentions has two cases: NULL in a numeric < comparison will always be "smaller". This is safe, and is what we are doing above. String ordering/sorting, where in some LOCALEs NULL sorts first, and in others, it sorts last. This is usually not a DB issue, but an OS issue. OSX sorts differently from most linuxen under the utf8 locales, for example. (This is something to be wary of, and what Petr is thinking about – but we aren't sorting it string-wise) Eloy, in terms of performance, what you are proposing is exactly the code I replaced :-/ – on a moderately sized DB it was causing the cron run to do 600 DB queries. See http://git.catalyst.net.nz/gitweb?p=moodle-r2.git;a=commitdiff;h=b78f4cbd31b5cfea3cddfea0b39891b785a34156 – it is wasteful to do 600 pointless DB queries for this when we can do 1. And we do need to handle complex SQL to be able to scale.
          Hide
          Eric Merrill added a comment - - edited

          Well, I think I'm putting my vote in for the 1 statement fix. It's most efficient, and i think we are pretty confident it will work cross db (and testing on my and pg):

          $subselect = "SELECT c.keepdays
          FROM {$CFG->prefix}chat c
          WHERE c.id = {$CFG->prefix}chat_messages.chatid";

          $sql = "DELETE
          FROM {$CFG->prefix}chat_messages
          WHERE ($subselect) > 0 AND timestamp < ( ".time()." -($subselect) * 24 * 3600)";

          Show
          Eric Merrill added a comment - - edited Well, I think I'm putting my vote in for the 1 statement fix. It's most efficient, and i think we are pretty confident it will work cross db (and testing on my and pg): $subselect = "SELECT c.keepdays FROM {$CFG->prefix}chat c WHERE c.id = {$CFG->prefix}chat_messages.chatid"; $sql = "DELETE FROM {$CFG->prefix}chat_messages WHERE ($subselect) > 0 AND timestamp < ( ".time()." -($subselect) * 24 * 3600)";
          Hide
          Martín Langhoff added a comment -

          Merged the single query - portable SQL fix. Fingers crossed...

          Show
          Martín Langhoff added a comment - Merged the single query - portable SQL fix. Fingers crossed...
          Hide
          Petr Škoda added a comment -

          Forgive my ignorance, but why do we have to use the NULL > 0 test (which evaluates as UNKNOWN) when there is "IS NOT NULL" ?

          Show
          Petr Škoda added a comment - Forgive my ignorance, but why do we have to use the NULL > 0 test (which evaluates as UNKNOWN) when there is "IS NOT NULL" ?
          Hide
          Martín Langhoff added a comment -

          You are 200% right - thanks! Fixed in 19_STABLE and HEAD.

          Show
          Martín Langhoff added a comment - You are 200% right - thanks! Fixed in 19_STABLE and HEAD.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Well fixed, anyway:

          • NULL in a numeric < comparison will always be "smaller".

          I really don't think it's correct. IMO, NULL in ANY comparison (<, =, >) will return false, i.e it's not comparable with anything.

          • String ordering/sorting, agree its' DB/OS dependent.
          • About SUBSELECTS, well AFAIK, some subselects can really produce a lot of temporary stuff and internally cause MORE queries (subquery executed FOREACH record in the main query) than one simple iterator + repeated simpler queries. Also it's far less readable than the iterator. No problem with the solution applied, sorry for sounding pedant, but it isn't necessarily better, IMO.

          Thanks for fixing this, anyway! B-)

          Show
          Eloy Lafuente (stronk7) added a comment - Well fixed, anyway: NULL in a numeric < comparison will always be "smaller". I really don't think it's correct. IMO, NULL in ANY comparison (<, =, >) will return false, i.e it's not comparable with anything. String ordering/sorting, agree its' DB/OS dependent. About SUBSELECTS, well AFAIK, some subselects can really produce a lot of temporary stuff and internally cause MORE queries (subquery executed FOREACH record in the main query) than one simple iterator + repeated simpler queries. Also it's far less readable than the iterator. No problem with the solution applied, sorry for sounding pedant, but it isn't necessarily better, IMO. Thanks for fixing this, anyway! B-)
          Hide
          Martín Langhoff added a comment -

          I agree the way the subselect is done now isn't pretty. The problem we have is that some DBs don't like it when we join into the same table as part of the WHERE clause – if we didn't have that problem, we could get it done with one JOIN. A nice fallback would be SELECTing the candidate rows to a temp-table, but we have plenty of portabilit drama with temp tables.

          So this iteration-in-sql we end up with is less than ideal, specially in the doubled-up subselect – however, it happens purely on the DB side, which makes it a lot cheaper. The DB side is usually quite good at optimising its own "private" temporary stuff, and the memory allocation schemes are smarter. If we transfer things to the PHP side we have higher costs in 'chatting' with the DB on the wire, and memory allocation.

          Show
          Martín Langhoff added a comment - I agree the way the subselect is done now isn't pretty. The problem we have is that some DBs don't like it when we join into the same table as part of the WHERE clause – if we didn't have that problem, we could get it done with one JOIN. A nice fallback would be SELECTing the candidate rows to a temp-table, but we have plenty of portabilit drama with temp tables. So this iteration-in-sql we end up with is less than ideal, specially in the doubled-up subselect – however, it happens purely on the DB side, which makes it a lot cheaper. The DB side is usually quite good at optimising its own "private" temporary stuff, and the memory allocation schemes are smarter. If we transfer things to the PHP side we have higher costs in 'chatting' with the DB on the wire, and memory allocation.
          Hide
          Eric Merrill added a comment -

          NOOOO!!!!!

          Changing that to 'NOT NULL' made it so it will delete all messages in chats that are set to 'keep forever'!!

          Show
          Eric Merrill added a comment - NOOOO!!!!! Changing that to 'NOT NULL' made it so it will delete all messages in chats that are set to 'keep forever'!!
          Hide
          Eric Merrill added a comment -

          The subselect is selecting each row in mdl_chat's keepdays. The greater function go's through each row and needs to eliminate all of them that are 0. If the subsect is null it would still have evaluated to false, but, another solution should be to change the subselect to this and leave the greater function with NOT NULL:

          SELECT c.keepdays
          FROM mdl_chat c
          WHERE c.id = mdl_chat_messages.chatid AND c.keepdays != 0)

          Show
          Eric Merrill added a comment - The subselect is selecting each row in mdl_chat's keepdays. The greater function go's through each row and needs to eliminate all of them that are 0. If the subsect is null it would still have evaluated to false, but, another solution should be to change the subselect to this and leave the greater function with NOT NULL: SELECT c.keepdays FROM mdl_chat c WHERE c.id = mdl_chat_messages.chatid AND c.keepdays != 0)
          Hide
          Eric Merrill added a comment -

          Comment, I changed it back to > 0 in MOODLE_19_STABLE.

          Sorry to do it, but that change could cause dataloss, so I wanted to get it out ASAP.

          Show
          Eric Merrill added a comment - Comment, I changed it back to > 0 in MOODLE_19_STABLE. Sorry to do it, but that change could cause dataloss, so I wanted to get it out ASAP.
          Hide
          Eric Merrill added a comment -

          Also commited change back to HEAD

          Show
          Eric Merrill added a comment - Also commited change back to HEAD
          Hide
          Martín Langhoff added a comment -

          Cool - thanks for that. Good spotting - got distracted and ended mixing up the 2 groups of data: those where it's truly null and those where it's 0. Sorry!

          [And great that you have CVS access too!]

          Show
          Martín Langhoff added a comment - Cool - thanks for that. Good spotting - got distracted and ended mixing up the 2 groups of data: those where it's truly null and those where it's 0. Sorry! [And great that you have CVS access too!]
          Hide
          Eric Merrill added a comment -

          Yeah, that was actually my first use of it. I was 'worried' about using it, but I saw the need

          The current version should work, but if people are worried about NULL > 0, then I think that we should go:

          $subselect = "SELECT c.keepdays
          FROM {$CFG->prefix}chat c
          WHERE c.id = {$CFG->prefix}chat_messages.chatid AND c.keepdays > 0";

          $sql = "DELETE
          FROM {$CFG->prefix}chat_messages
          WHERE ($subselect) IS NOT NULL AND timestamp < ( ".time()." -($subselect) * 24 * 3600)";

          But I haven't tested it throughly yet (but I have preliminarily tested it in postgres)

          -eric

          Show
          Eric Merrill added a comment - Yeah, that was actually my first use of it. I was 'worried' about using it, but I saw the need The current version should work, but if people are worried about NULL > 0, then I think that we should go: $subselect = "SELECT c.keepdays FROM {$CFG->prefix}chat c WHERE c.id = {$CFG->prefix}chat_messages.chatid AND c.keepdays > 0"; $sql = "DELETE FROM {$CFG->prefix}chat_messages WHERE ($subselect) IS NOT NULL AND timestamp < ( ".time()." -($subselect) * 24 * 3600)"; But I haven't tested it throughly yet (but I have preliminarily tested it in postgres) -eric
          Hide
          Martin Dougiamas added a comment -

          Thanks, Eric!

          You also need to set the MERGED tag as the last step to record that the code has been merged between branch and HEAD.

          eg:

          cd moodle/19/mod/chat
          cvs tag -F MOODLE_19_MERGED lib.php

          I've set the flag for you this time, np!

          Is this issue resolved now?

          Show
          Martin Dougiamas added a comment - Thanks, Eric! You also need to set the MERGED tag as the last step to record that the code has been merged between branch and HEAD. eg: cd moodle/19/mod/chat cvs tag -F MOODLE_19_MERGED lib.php I've set the flag for you this time, np! Is this issue resolved now?
          Hide
          Martin Dougiamas added a comment -

          I think it is, so I'll mark this issue as resolved. Please reopen if there's a problem.

          Show
          Martin Dougiamas added a comment - I think it is, so I'll mark this issue as resolved. Please reopen if there's a problem.

            People

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

              Dates

              • Created:
                Updated:
                Resolved: