Details

    • Database:
      MySQL
    • Testing Instructions:
      Hide

      Run daily statistics with major log entries for a day.

      With more than 150 000 entries, daily statistics generation can take 2 hours to resolve.

      Also run phpunit tests across 5 DB's

      Show
      Run daily statistics with major log entries for a day. With more than 150 000 entries, daily statistics generation can take 2 hours to resolve. Also run phpunit tests across 5 DB's
    • Affected Branches:
      MOODLE_20_STABLE, MOODLE_21_STABLE, MOODLE_24_STABLE
    • Fixed Branches:
      MOODLE_24_STABLE
    • Pull Master Branch:
      MDL-30643_master-log-speed
    • Rank:
      33457

      Description

      Daily statistics generation is really slow because SQL queries are not optimized properly. The propblem occurs principaly when you have a lots of entries in the log table (150 000 and more per day). We also observed that the server is responding slowly for users in Moodle while the daily statistics generation is running (Every inserts in log table take about 40-50 seconds).

      December 5th cron log from our server

      Running daily statistics gathering, starting at 1322974800:
      ................  finished until 1323061200: lundi 5 décembre 2011, 00:00 (in 6872 s)
      ...completed 1 days of statistics.
      
      1. MDL-30643_pu.log
        25 kB
        Aparup Banerjee
      2. MDL-30643-TZ_phpunit.rtf
        67 kB
        Aparup Banerjee
      3. mssql_phpunit_output.txt
        49 kB
        Aparup Banerjee
      4. mssql_phpunit_output2.txt
        49 kB
        Michael de Raadt
      5. mssql_phpunit_output3.txt
        57 kB
        Michael de Raadt
      6. oracle_phpunit_output.txt
        49 kB
        Aparup Banerjee
      7. oracle_phpunit_output2.txt
        49 kB
        Michael de Raadt
      8. oracle_phpunit_output3.txt
        55 kB
        Michael de Raadt
      9. phpunit_MDL-30643_debug_n.txt
        54 kB
        Aparup Banerjee
      10. stats-v2.patch
        17 kB
        Tyler Bannister

        Issue Links

          Activity

          Hide
          Jean-Philippe Gaudreau added a comment -

          Here's a patch for the bug. The point is to create a temporary table containing the logs per day with the right indexes.

          Queries are taking 40-45 seconds instead of 1h30-2h to run.

          Show
          Jean-Philippe Gaudreau added a comment - Here's a patch for the bug. The point is to create a temporary table containing the logs per day with the right indexes. Queries are taking 40-45 seconds instead of 1h30-2h to run.
          Hide
          Michael de Raadt added a comment -

          Thanks for sharing that solution. This could have a significant impact and it looks like it could resolve a long chain of issues.

          Show
          Michael de Raadt added a comment - Thanks for sharing that solution. This could have a significant impact and it looks like it could resolve a long chain of issues.
          Hide
          Petr Škoda added a comment -

          There are very many problems related to logging and stats, I personally think that we should prioritize this for the next dev cycle.

          The proposed patch is not unfortunately compliant with our coding style, also it is not possible to modify table structure in stable branches, sorry.

          Show
          Petr Škoda added a comment - There are very many problems related to logging and stats, I personally think that we should prioritize this for the next dev cycle. The proposed patch is not unfortunately compliant with our coding style, also it is not possible to modify table structure in stable branches, sorry.
          Hide
          Gerard Caulfield added a comment -

          Disregard my last comment posted here I just though of what I may not have yet tried.

          Show
          Gerard Caulfield added a comment - Disregard my last comment posted here I just though of what I may not have yet tried.
          Hide
          Tyler Bannister added a comment -

          Petr, are you sure about it be contrary to the coding style? I was informed that creating a scratch table was acceptable behavior and has been done in other parts of Moodle where it was useful. On that basis, I don't think the use of a scratch table should disqualify this patch. My testing is showing an approximately 20 times performance improvement from the patch. Specifically, I tested with data from a production site and it took 900 seconds to generate stats before patch and 45 seconds after the patch. I tested the results for discrepancies and the actual results were identical before and after patch.

          Show
          Tyler Bannister added a comment - Petr, are you sure about it be contrary to the coding style? I was informed that creating a scratch table was acceptable behavior and has been done in other parts of Moodle where it was useful. On that basis, I don't think the use of a scratch table should disqualify this patch. My testing is showing an approximately 20 times performance improvement from the patch. Specifically, I tested with data from a production site and it took 900 seconds to generate stats before patch and 45 seconds after the patch. I tested the results for discrepancies and the actual results were identical before and after patch.
          Hide
          Petr Škoda added a comment -

          Yes, changes in database structure are not allowed in stable branches.

          The stats are slow because we are doing crazy things there - the biggest mistake is trying to find out type of operation for each row (read or write) from the action name. It is inaccurate and very slow - the obvious solution is to add new column into the log table and specify the type when adding log entries.

          I believe the current logging/stats is not worth improving, somebody should imo redesign it completely.

          Thanks for your contribution anyway.

          Show
          Petr Škoda added a comment - Yes, changes in database structure are not allowed in stable branches. The stats are slow because we are doing crazy things there - the biggest mistake is trying to find out type of operation for each row (read or write) from the action name. It is inaccurate and very slow - the obvious solution is to add new column into the log table and specify the type when adding log entries. I believe the current logging/stats is not worth improving, somebody should imo redesign it completely. Thanks for your contribution anyway.
          Hide
          Mike Churchward added a comment -

          This could be changed to use "create_temp_table" and "drop_temp_table" if that makes the code compliant. Those functions are used in standard core code. Would that work?

          Show
          Mike Churchward added a comment - This could be changed to use "create_temp_table" and "drop_temp_table" if that makes the code compliant. Those functions are used in standard core code. Would that work?
          Hide
          Jean-Philippe Gaudreau added a comment -

          Hi Mike,

          I thought about this but unfortunately it's not an option with the current queries because of a MySQL limitation

          See http://dev.mysql.com/doc/refman/5.1/en/temporary-table-problems.html :
          "You cannot refer to a TEMPORARY table more than once in the same query."

          Maybe the queries could be modified to only have one reference each but it would need time that I don't have... Feel free to propose a patch if you want base on what I started

          Show
          Jean-Philippe Gaudreau added a comment - Hi Mike, I thought about this but unfortunately it's not an option with the current queries because of a MySQL limitation See http://dev.mysql.com/doc/refman/5.1/en/temporary-table-problems.html : "You cannot refer to a TEMPORARY table more than once in the same query." Maybe the queries could be modified to only have one reference each but it would need time that I don't have... Feel free to propose a patch if you want base on what I started
          Hide
          Michael de Raadt added a comment -

          Shifting this over to the DEV backlog as it will require DB changes.

          Show
          Michael de Raadt added a comment - Shifting this over to the DEV backlog as it will require DB changes.
          Hide
          Michael de Raadt added a comment -

          I think if this change can make an improvement, even only to Master, then we should pursue it for now. I don't think we're going to get a chance to re-write logging in the near future.

          Show
          Michael de Raadt added a comment - I think if this change can make an improvement, even only to Master, then we should pursue it for now. I don't think we're going to get a chance to re-write logging in the near future.
          Hide
          Petr Škoda added a comment -

          Hi Michael, I disagree with proposed workaround because it would be imo a lot better to change log table structure and completely eliminate unnecessary operations in stats processing. Reassigning back, sorry.

          Show
          Petr Škoda added a comment - Hi Michael, I disagree with proposed workaround because it would be imo a lot better to change log table structure and completely eliminate unnecessary operations in stats processing. Reassigning back, sorry.
          Hide
          Mike Churchward added a comment -

          So, how soon can the log table structure be changed, and the stats processing be rewritten?

          In the meantime, large scale Moodle users will lose site functionality whenever they try to use stats functions. The way it is now, the sites hang completely. This is not acceptable. Should we not promote workarounds to fix problems when time will not allow complete rewrites?

          Show
          Mike Churchward added a comment - So, how soon can the log table structure be changed, and the stats processing be rewritten? In the meantime, large scale Moodle users will lose site functionality whenever they try to use stats functions. The way it is now, the sites hang completely. This is not acceptable. Should we not promote workarounds to fix problems when time will not allow complete rewrites?
          Hide
          Petr Škoda added a comment -

          I am sorry I do not have the authority to promise or decide anything.

          Show
          Petr Škoda added a comment - I am sorry I do not have the authority to promise or decide anything.
          Hide
          Martin Dougiamas added a comment -

          I do.

          If someone can come up with a patch for master that

          • adds the new field with the read/write type
          • change logging code to make sure that the field is filled during upgrade, and always kept full after that whenever entries are added (possibly slow) or at least on a cron job.

          then this might solve the problem for now and allow an improvement for 2.3, pending a bigger rewrite at some point in the future.

          Show
          Martin Dougiamas added a comment - I do. If someone can come up with a patch for master that adds the new field with the read/write type change logging code to make sure that the field is filled during upgrade, and always kept full after that whenever entries are added (possibly slow) or at least on a cron job. then this might solve the problem for now and allow an improvement for 2.3, pending a bigger rewrite at some point in the future.
          Hide
          Tyler Bannister added a comment - - edited

          I don't think the new read/write field will improve performance as much as predicted. If I completely remove the subqueries that try "to find out type of operation for each row (read or write) from the action name" then I see an approximately 40% drop in processing time. That's a far cry from the 95% drop achieved using the temporary table. Additionally, all of that drop appears to be related to removing the subqueries rather than the "IN" clause, I have spent some time working out alternative queries that use the IN clauses and produces the same results without the subqueries and I get virtually identical performance improvements with or without the IN clauses.

          Show
          Tyler Bannister added a comment - - edited I don't think the new read/write field will improve performance as much as predicted. If I completely remove the subqueries that try "to find out type of operation for each row (read or write) from the action name" then I see an approximately 40% drop in processing time. That's a far cry from the 95% drop achieved using the temporary table. Additionally, all of that drop appears to be related to removing the subqueries rather than the "IN" clause, I have spent some time working out alternative queries that use the IN clauses and produces the same results without the subqueries and I get virtually identical performance improvements with or without the IN clauses.
          Hide
          Mike Churchward added a comment -

          So, Tyler, you are suggesting that the correct solution is still to use temporary tables? Is that correct?

          Show
          Mike Churchward added a comment - So, Tyler, you are suggesting that the correct solution is still to use temporary tables? Is that correct?
          Hide
          Tyler Bannister added a comment -

          I've attached a new patch that should resolve all currently outstanding issues. This patch uses 2 temporary tables to reach the same 95% reduction in execution time that the original solution had. It also incorporates the improvements to the stats queries themselves (including one that reduces the number of needed temporary tables from 3 to 2) that I was working on.

          Show
          Tyler Bannister added a comment - I've attached a new patch that should resolve all currently outstanding issues. This patch uses 2 temporary tables to reach the same 95% reduction in execution time that the original solution had. It also incorporates the improvements to the stats queries themselves (including one that reduces the number of needed temporary tables from 3 to 2) that I was working on.
          Hide
          Mike Churchward added a comment -

          Martin / Michael / Petr - This new patch looks like a significant improvement. It uses temporary tables solely through the temp tables API. Can this be integrated? We'll happily put the patch into a git branch if that is the preferred way.

          Show
          Mike Churchward added a comment - Martin / Michael / Petr - This new patch looks like a significant improvement. It uses temporary tables solely through the temp tables API. Can this be integrated? We'll happily put the patch into a git branch if that is the preferred way.
          Hide
          Michael de Raadt added a comment -

          Hi. Mike and all.

          We haven't forgotten about this.

          I'm chasing this up this week.

          Show
          Michael de Raadt added a comment - Hi. Mike and all. We haven't forgotten about this. I'm chasing this up this week.
          Hide
          Tony Levi added a comment -

          Whoa, this is amazing, I've just implemented the same thing because we have production systems unable to finish the stats in days.

          It really does work, because instead of scanning either course/user or time index (can't reasonably do both), pre-selecting the right times with course/user indexes allows the DB to scan only the needed bits.

          thoughts on patch above...

          • Don't need all columns. course, userid, action should be enough.
          • Don't need all indexes, one for (course,userid) and one for (userid) worked nicely for me
          • If temptables is a problem for mysql, maybe only 1 perm table first truncated is fine?
          Show
          Tony Levi added a comment - Whoa, this is amazing, I've just implemented the same thing because we have production systems unable to finish the stats in days . It really does work, because instead of scanning either course/user or time index (can't reasonably do both), pre-selecting the right times with course/user indexes allows the DB to scan only the needed bits. thoughts on patch above... Don't need all columns. course, userid, action should be enough. Don't need all indexes, one for (course,userid) and one for (userid) worked nicely for me If temptables is a problem for mysql, maybe only 1 perm table first truncated is fine?
          Hide
          Tony Levi added a comment -

          https://github.com/tlevi/moodle/compare/master...mdl30643

          Ok, here is my contribution rebased to master featuring:

          • Rewrite all queries for speed/scaling and to make only one reference to log_temp, generally simplified etc
          • Pre-aggregate the log_temp table data by action and ignore log.course = 0 (seems bogus)
          • Don't use stats_user_daily in the calculations, as PG is making horrific plans against it

          I've ignored (and removed) the PG 8.2 warnings, as 8.3 is the oldest supported in stable by now AFAIK. These should in theory work against all 4 DBs because other core code uses the same constructs, but obviously this will need testing.

          With the data I'm using - about 120-150k log entries per day - the daily stats are calculated in about 15 seconds consistently which is much nicer than the 'days, until killed' issue we're having at the moment.

          Show
          Tony Levi added a comment - https://github.com/tlevi/moodle/compare/master...mdl30643 Ok, here is my contribution rebased to master featuring: Rewrite all queries for speed/scaling and to make only one reference to log_temp, generally simplified etc Pre-aggregate the log_temp table data by action and ignore log.course = 0 (seems bogus) Don't use stats_user_daily in the calculations, as PG is making horrific plans against it I've ignored (and removed) the PG 8.2 warnings, as 8.3 is the oldest supported in stable by now AFAIK. These should in theory work against all 4 DBs because other core code uses the same constructs, but obviously this will need testing. With the data I'm using - about 120-150k log entries per day - the daily stats are calculated in about 15 seconds consistently which is much nicer than the 'days, until killed' issue we're having at the moment.
          Hide
          Tony Levi added a comment -

          Just updated with a commit to fix a case where the temptable isn't disposed.

          Otherwise, this is working nicely on production so far.

          Show
          Tony Levi added a comment - Just updated with a commit to fix a case where the temptable isn't disposed. Otherwise, this is working nicely on production so far.
          Hide
          Tyler Bannister added a comment -

          Hi Tony, I found a few issues with your changes when I tested it on my test data:

          1. Your changes produced different numbers of results in stats_daily and stats_user_daily when compared to the original stats code.
          2. Some of the rows in stats_daily and stats_user_daily had slightly different totals when compared to the original stats code.
          3. Your patch is a little slower than the last patch I attached here (79 seconds versus 43 seconds).
          4. You have introduced a potential crash bug and the potential to generate incorrect stats. At around line 188 in your patch you conditionally generate the temporary table if there are logs present. However, you reference the temporary logs table at least once (at 249) without also checking that logs were present thus if the first day of results is empty, stats generation will crash because it attempted to access a table which doesn't yet exist. Similarly, if any day has no logs that query will generate results based on the previous day's logs.

          Most of the additional time appears to be consumed by query 11 which calculates the number of guest accesses.

          Show
          Tyler Bannister added a comment - Hi Tony, I found a few issues with your changes when I tested it on my test data: Your changes produced different numbers of results in stats_daily and stats_user_daily when compared to the original stats code. Some of the rows in stats_daily and stats_user_daily had slightly different totals when compared to the original stats code. Your patch is a little slower than the last patch I attached here (79 seconds versus 43 seconds). You have introduced a potential crash bug and the potential to generate incorrect stats. At around line 188 in your patch you conditionally generate the temporary table if there are logs present. However, you reference the temporary logs table at least once (at 249) without also checking that logs were present thus if the first day of results is empty, stats generation will crash because it attempted to access a table which doesn't yet exist. Similarly, if any day has no logs that query will generate results based on the previous day's logs. Most of the additional time appears to be consumed by query 11 which calculates the number of guest accesses.
          Hide
          Tyler Bannister added a comment - - edited

          I've incorporated some of Tony's changes into an updated patch on github for the issue that is a little bit faster than the one I previously added. Strangely, when I changed the order of inserting data and adding indexes, the temporary table creation time tripled. I would have thought it would be faster to insert the data and add the indexes later but testing indicated otherwise. Similarly, I didn't keep the changes to the database queries because it looked to be nearly universally slower with the new temporary table format. I did however eliminate the unused columns and indexes from the temporary table and switch to use microtime instead of time for query tracking.

          Show
          Tyler Bannister added a comment - - edited I've incorporated some of Tony's changes into an updated patch on github for the issue that is a little bit faster than the one I previously added. Strangely, when I changed the order of inserting data and adding indexes, the temporary table creation time tripled. I would have thought it would be faster to insert the data and add the indexes later but testing indicated otherwise. Similarly, I didn't keep the changes to the database queries because it looked to be nearly universally slower with the new temporary table format. I did however eliminate the unused columns and indexes from the temporary table and switch to use microtime instead of time for query tracking.
          Hide
          Tony Levi added a comment -

          Which kind of rows are different? I tested with some weeks of production data so this is surprising.

          Show
          Tony Levi added a comment - Which kind of rows are different? I tested with some weeks of production data so this is surprising.
          Hide
          Dan Marsden added a comment -

          Tyler - any further feedback on this? - Tony have you had a chance to compare the stats that Tylers patch generates against the stats your patch generates?

          Show
          Dan Marsden added a comment - Tyler - any further feedback on this? - Tony have you had a chance to compare the stats that Tylers patch generates against the stats your patch generates?
          Hide
          Dan Marsden added a comment -

          also Tyler - which db types have you tested your patch on? it appears Tony has been running on PG

          Show
          Dan Marsden added a comment - also Tyler - which db types have you tested your patch on? it appears Tony has been running on PG
          Hide
          Tony Levi added a comment -

          I don't have the test setup anymore ... but I did compare my results to the existing core code, so if there is a difference to Tyler's patch it is either some different data my test didn't hit or that patch is bad.

          My patch series may be slower in some conditions but the idea is that it should scale better and avoids bad queries against the stats daily tables, which is just a smaller version of the mdl_log problem (i.e it will sometimes pull things by the course, user or role index, which will get slower and slower as the amount of entries increases over time).

          Switching /everything/ to use log_temp ensures a basically constant run time for years at a given traffic level. IMO, it is fair enough to get a bit slower with more traffic, but not OK to be dramatically slower over time.

          There is one difference in (I think) stats_user_daily for course=0, userid=0 - the row appears to have no purpose so I haven't included it in the new version of the queries. It looks like a hack to make some of the later stats_daily things work but is now obsolete as everything runs though log temp. If any other rows are different I'd really like to know what they are and the data that causes it so it can be fixed...

          I've fixed the only missing $logspresent check to prevent the 'crash' issue and rebased to master again.

          Show
          Tony Levi added a comment - I don't have the test setup anymore ... but I did compare my results to the existing core code, so if there is a difference to Tyler's patch it is either some different data my test didn't hit or that patch is bad. My patch series may be slower in some conditions but the idea is that it should scale better and avoids bad queries against the stats daily tables, which is just a smaller version of the mdl_log problem (i.e it will sometimes pull things by the course, user or role index, which will get slower and slower as the amount of entries increases over time). Switching /everything/ to use log_temp ensures a basically constant run time for years at a given traffic level. IMO, it is fair enough to get a bit slower with more traffic, but not OK to be dramatically slower over time. There is one difference in (I think) stats_user_daily for course=0, userid=0 - the row appears to have no purpose so I haven't included it in the new version of the queries. It looks like a hack to make some of the later stats_daily things work but is now obsolete as everything runs though log temp. If any other rows are different I'd really like to know what they are and the data that causes it so it can be fixed... I've fixed the only missing $logspresent check to prevent the 'crash' issue and rebased to master again.
          Hide
          Matteo Scaramuccia added a comment -

          Hi,
          please, try to consider also a 1.9 backporting to close MDL-18484 too, when a fix will be available for 2.x.
          Even though 1.9 will be EOLed everything about slow and blocking daily stats has been brought to the attention by 1.9 users

          TIA,
          Matteo

          Show
          Matteo Scaramuccia added a comment - Hi, please, try to consider also a 1.9 backporting to close MDL-18484 too, when a fix will be available for 2.x. Even though 1.9 will be EOLed everything about slow and blocking daily stats has been brought to the attention by 1.9 users TIA, Matteo
          Hide
          Tyler Bannister added a comment -

          I'm testing with MySQL. Here are the differences I'm seeing for a month of results:

          mdl_stats_daily:
          Pre-patch: 161,016 records
          Post-patch: 160,964 records

          It looks like I was mistaken about row differences in stats_daily, the tow totals appear to be same when I re-ran Tony's patch (excepting of course the duplicate rows (courseid=siteid,roleid=0) that both version of the stats code create).

          The missing rows are curious, in this case, there appears to be about 50 missing rows all for a specific role (72 - Guest no profile) with courseid 1 or 0. The remaining two missing rows are duplicates that weren't created with Tony's patch.

          mdl_stats_user_daily:
          Pre-patch: 34,312 records
          Post-patch: 34,261 records

          The missing rows for stats_user_daily are activity logs with courseid=0, roleid=0.
          stats_user_daily also has one record which is different for each day (site activity for not logged in users), however, that appears to be a duplicate row that has been created with 0s in it. Thus there are 31 additional rows in Tony's results and 82 missing rows.

          I see the point about avoiding using the stats_daily tables. However, it seems to me there will still see some scaling with the new queries according to number of courses, users, and site activity. It might be a good idea to investigate generating the daily stats directly into temporary tables and copying the results into the permanent tables when the day's stats have been completed. That should also reduce the risk of incomplete or incorrect stats being generated if the cron is interrupted.

          @Matteo - There is already a similar patch attached to MDL-18484 that would reduce log creation time by a factor of 60.

          Show
          Tyler Bannister added a comment - I'm testing with MySQL. Here are the differences I'm seeing for a month of results: mdl_stats_daily: Pre-patch: 161,016 records Post-patch: 160,964 records It looks like I was mistaken about row differences in stats_daily, the tow totals appear to be same when I re-ran Tony's patch (excepting of course the duplicate rows (courseid=siteid,roleid=0) that both version of the stats code create). The missing rows are curious, in this case, there appears to be about 50 missing rows all for a specific role (72 - Guest no profile) with courseid 1 or 0. The remaining two missing rows are duplicates that weren't created with Tony's patch. mdl_stats_user_daily: Pre-patch: 34,312 records Post-patch: 34,261 records The missing rows for stats_user_daily are activity logs with courseid=0, roleid=0. stats_user_daily also has one record which is different for each day (site activity for not logged in users), however, that appears to be a duplicate row that has been created with 0s in it. Thus there are 31 additional rows in Tony's results and 82 missing rows. I see the point about avoiding using the stats_daily tables. However, it seems to me there will still see some scaling with the new queries according to number of courses, users, and site activity. It might be a good idea to investigate generating the daily stats directly into temporary tables and copying the results into the permanent tables when the day's stats have been completed. That should also reduce the risk of incomplete or incorrect stats being generated if the cron is interrupted. @Matteo - There is already a similar patch attached to MDL-18484 that would reduce log creation time by a factor of 60.
          Hide
          Tyler Bannister added a comment -

          I've incorporated my idea about using temporary tables to avoid the potential impact of history growth on the daily tables into my last posted patch. The diff between the updated branch and master is available on git hub (https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed). This new patch is a little slower than the last one, the addition of the extra temporary tables seems to counteract the gains made by removing unnecessary tables and indexes. However, in theory, it should perform better on sites with with many entries in the daily stats tables.

          Show
          Tyler Bannister added a comment - I've incorporated my idea about using temporary tables to avoid the potential impact of history growth on the daily tables into my last posted patch. The diff between the updated branch and master is available on git hub ( https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed ). This new patch is a little slower than the last one, the addition of the extra temporary tables seems to counteract the gains made by removing unnecessary tables and indexes. However, in theory, it should perform better on sites with with many entries in the daily stats tables.
          Hide
          Tony Levi added a comment -

          Still not a fan; those updates against stats daily are the most expensive part because of the time spent in loops against it so this patch wouldn't help our case much.

          Updating several stats for each operation on the site is no way ever going to work in high concurrency, unless I misunderstand?

          Show
          Tony Levi added a comment - Still not a fan; those updates against stats daily are the most expensive part because of the time spent in loops against it so this patch wouldn't help our case much. Updating several stats for each operation on the site is no way ever going to work in high concurrency, unless I misunderstand?
          Hide
          Mike Churchward added a comment -

          Tony / Tyler, can you each summarize the cases you are concerned about, so we can see if there is a common solution?

          Show
          Mike Churchward added a comment - Tony / Tyler, can you each summarize the cases you are concerned about, so we can see if there is a common solution?
          Hide
          Tyler Bannister added a comment -

          Tony, I don't think I understand why it wouldn't help your case much. Could you explain a bit more about what your case is?

          Also, I don't understand what you mean with your high concurrency comment. Since my last patch substitutes a temporary table (mdl_tmp_stats_daily and mdl_tmp_stats_user_daily) for each stats table, it should have the fewest possible concurrency issues. The real tables will only be accessed once for reading, and once for writing. The update queries will operate in isolation on those temporary tables because each process can only access it's own temporary tables.

          Show
          Tyler Bannister added a comment - Tony, I don't think I understand why it wouldn't help your case much. Could you explain a bit more about what your case is? Also, I don't understand what you mean with your high concurrency comment. Since my last patch substitutes a temporary table (mdl_tmp_stats_daily and mdl_tmp_stats_user_daily) for each stats table, it should have the fewest possible concurrency issues. The real tables will only be accessed once for reading, and once for writing. The update queries will operate in isolation on those temporary tables because each process can only access it's own temporary tables.
          Hide
          Dan Marsden added a comment -

          my concern would be the testing across multiple db types (I haven't looked at the patch in detail) - but in the sql performance work I've done in the past I've seen massive differences in the way SQL performs across the different DB types - what might work well for MySql might be really slow on other db types. It would be nice to see some stats across the different db types if poss (I haven't got the time to do the testing myself)

          Show
          Dan Marsden added a comment - my concern would be the testing across multiple db types (I haven't looked at the patch in detail) - but in the sql performance work I've done in the past I've seen massive differences in the way SQL performs across the different DB types - what might work well for MySql might be really slow on other db types. It would be nice to see some stats across the different db types if poss (I haven't got the time to do the testing myself)
          Hide
          Tony Levi added a comment -

          The /worst/ problem for us has been the updates against mdl_stats*_daily for the second column and they get worse over time; using log_temp didn't improve these because of the time sunk into query nestloops on the stats tables. This could run through the night and most of a day, with the worst cases running for 3 days straight.

          It looks like the patches bring the times to a sane level for both PG and MySQL at least; somebody else will have to try MS & oracle.

          Show
          Tony Levi added a comment - The /worst/ problem for us has been the updates against mdl_stats*_daily for the second column and they get worse over time; using log_temp didn't improve these because of the time sunk into query nestloops on the stats tables. This could run through the night and most of a day, with the worst cases running for 3 days straight. It looks like the patches bring the times to a sane level for both PG and MySQL at least; somebody else will have to try MS & oracle.
          Hide
          Tyler Bannister added a comment -

          Tony, if I understand correctly, then if the patch (https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed) changes the updates so that they no longer run against the mdl_stats*_daily tables (which it does) then the patch should be acceptable?

          I don't currently have access to either MSSQL or Oracle either.

          Show
          Tyler Bannister added a comment - Tony, if I understand correctly, then if the patch ( https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed ) changes the updates so that they no longer run against the mdl_stats*_daily tables (which it does) then the patch should be acceptable? I don't currently have access to either MSSQL or Oracle either.
          Hide
          Tony Levi added a comment -

          I think adding another temporary table is not a clean solution; its a workaround for the terrible queries. IMO, proper solution is to rewrite for the more modern DB requirements in 2.x - but maybe upstream is not interested in such a large diff and your patches will be better for them.

          I've just run this over 2 weeks of production data to compare /again/ and there is no difference besides the courseid=0 bogus entries so I really think the integrity concerns require more explanation / evidence.

          Rebased my branch to master again.

          Show
          Tony Levi added a comment - I think adding another temporary table is not a clean solution; its a workaround for the terrible queries. IMO, proper solution is to rewrite for the more modern DB requirements in 2.x - but maybe upstream is not interested in such a large diff and your patches will be better for them. I've just run this over 2 weeks of production data to compare /again/ and there is no difference besides the courseid=0 bogus entries so I really think the integrity concerns require more explanation / evidence. Rebased my branch to master again.
          Hide
          Tyler Bannister added a comment -

          Tony, I don't think it's a good idea to conflate different changes together. An efficiency fix should produce identical results before and after the fix. If you would like to change what results are produced, a separate issue should be opened for that.

          The second log temporary table is simply to work around a MySQL limitation that prevents the table from being referenced multiple times in the same query. With the extra temporary table I see about a 40% reduction in statistics generation time versus the queries that cut the extra table out. Personally, I'm willing to go with "a workaround" that provides a substantial performance improvement.

          Show
          Tyler Bannister added a comment - Tony, I don't think it's a good idea to conflate different changes together. An efficiency fix should produce identical results before and after the fix. If you would like to change what results are produced, a separate issue should be opened for that. The second log temporary table is simply to work around a MySQL limitation that prevents the table from being referenced multiple times in the same query. With the extra temporary table I see about a 40% reduction in statistics generation time versus the queries that cut the extra table out. Personally, I'm willing to go with "a workaround" that provides a substantial performance improvement.
          Hide
          Mike Churchward added a comment -

          Hi Tony / Tyler...

          Trying to get this to some closure.

          Tony - Rewriting the stats was discussed at the top of this issue. Everyone agreed that rewriting is the preferred solution, but that it would not happen any time soon. So, we need a compromise fix to prevent the large users from failing. Or, are you saying you already have a rewrite, when you say "but maybe upstream is not interested in such a large diff"?

          From what I read here, the latest patch provided from Tyler has worked on all the systems tested (MySQL and Postgres), and has proven to be quite faster. There is concern about it using two temporary tables, but again, this improves the performance and should be a good solution until the rewrite can be done.

          There is still an unresolved discrepancy in the alternate solution provided by Tony, where data results were not the same from the current code.

          Please let me know if there are any incorrect statements above.

          If there are not, I would recommend that we implement the latest patch from Tyler to get this issue resolved.

          Show
          Mike Churchward added a comment - Hi Tony / Tyler... Trying to get this to some closure. Tony - Rewriting the stats was discussed at the top of this issue. Everyone agreed that rewriting is the preferred solution, but that it would not happen any time soon. So, we need a compromise fix to prevent the large users from failing. Or, are you saying you already have a rewrite, when you say "but maybe upstream is not interested in such a large diff"? From what I read here, the latest patch provided from Tyler has worked on all the systems tested (MySQL and Postgres), and has proven to be quite faster. There is concern about it using two temporary tables, but again, this improves the performance and should be a good solution until the rewrite can be done. There is still an unresolved discrepancy in the alternate solution provided by Tony, where data results were not the same from the current code. Please let me know if there are any incorrect statements above. If there are not, I would recommend that we implement the latest patch from Tyler to get this issue resolved.
          Hide
          Tony Levi added a comment -

          There is no discrepancy that can be demonstrated so I'd appreciate it if people stopped repeatedly saying that without showing the goods??

          It doesn't impact us much by now but given it /has/ been rewritten I see no reason (yet) that effort can't be integrated. Otherwise... well, we'll keep this piece forked as long as needed.

          Show
          Tony Levi added a comment - There is no discrepancy that can be demonstrated so I'd appreciate it if people stopped repeatedly saying that without showing the goods?? It doesn't impact us much by now but given it /has/ been rewritten I see no reason (yet) that effort can't be integrated. Otherwise... well, we'll keep this piece forked as long as needed.
          Hide
          Dan Marsden added a comment -

          also - there seems to be reference to Tyler and his patch about "duplicate rows" ?

          if the old code or Tylers patch creates incorrect stats that should be fixed at the same time as a patch - it's not important for a new patch to generate exactly the same stats if the old stats are wrong.

          Show
          Dan Marsden added a comment - also - there seems to be reference to Tyler and his patch about "duplicate rows" ? if the old code or Tylers patch creates incorrect stats that should be fixed at the same time as a patch - it's not important for a new patch to generate exactly the same stats if the old stats are wrong.
          Hide
          Mike Churchward added a comment -

          Tony, if I offended you, I apologize. It was not intentional. What I am trying to do is narrow down which patch is appropriate to consider recommending for addition into core. Martin D. indicated he would get an appropriate patch in.

          As I understand it, there are two patches here for consideration. One supplied by Tony and one supplied by Tyler. Tyler's apparently adds another 40% improvement in time over Tony's.

          With respect to the discrepancy, Tyler indicated in one of his comments that he saw data discrepancies with Tony's patch. Tyler - can you confirm this and provide proof? If the discrepancy is really a fix to incorrect data in the original code, as Dan suggests, we can dismiss that issue.

          Tony, you said:
          "It doesn't impact us much by now but given it /has/ been rewritten I see no reason (yet) that effort can't be integrated. Otherwise... well, we'll keep this piece forked as long as needed."

          Can you elaborate on this a bit? What is that has been rewritten? Are you referring to the patches attached, or something else? And what do you mean by "we'll keep this piece forked"? Are you referring to just your organization?

          So, given that Tyler's patch offers even more performance improvements, why would we not recommend that over Tony's? Just looking for the reasons.

          Show
          Mike Churchward added a comment - Tony, if I offended you, I apologize. It was not intentional. What I am trying to do is narrow down which patch is appropriate to consider recommending for addition into core. Martin D. indicated he would get an appropriate patch in. As I understand it, there are two patches here for consideration. One supplied by Tony and one supplied by Tyler. Tyler's apparently adds another 40% improvement in time over Tony's. With respect to the discrepancy, Tyler indicated in one of his comments that he saw data discrepancies with Tony's patch. Tyler - can you confirm this and provide proof? If the discrepancy is really a fix to incorrect data in the original code, as Dan suggests, we can dismiss that issue. Tony, you said: "It doesn't impact us much by now but given it /has/ been rewritten I see no reason (yet) that effort can't be integrated. Otherwise... well, we'll keep this piece forked as long as needed." Can you elaborate on this a bit? What is that has been rewritten? Are you referring to the patches attached, or something else? And what do you mean by "we'll keep this piece forked"? Are you referring to just your organization? So, given that Tyler's patch offers even more performance improvements, why would we not recommend that over Tony's? Just looking for the reasons.
          Hide
          Tony Levi added a comment -

          Well it is late but.. I think the existing queries can have many problem cases where planners are going to fall down, especially related to the excessive use of subqueries, updates against stats tables etc as workarounds to really old and no longer existing DB issues - this is what we hit on different instances, they all have slightly different issues. My patch /series/ builds on Tyler's temptable-only patch, rewriting these poor queries and cleaning up the code a bit too.

          In his particular data set and DB, some of these needlessly complex queries are slightly faster. Ok, but... that doesn't make them correct or desirable. Since we're talking about a 1000x speedup anyway, I hardly think 5 seconds vs 3 seconds seriously matters over simpler codes with less edge cases / problems with other data. Upstream should have a reasonable set of queries that are all-round good, rather than best for only DB X, data Y.

          Hope this explains more.

          Show
          Tony Levi added a comment - Well it is late but.. I think the existing queries can have many problem cases where planners are going to fall down, especially related to the excessive use of subqueries, updates against stats tables etc as workarounds to really old and no longer existing DB issues - this is what we hit on different instances, they all have slightly different issues. My patch /series/ builds on Tyler's temptable-only patch, rewriting these poor queries and cleaning up the code a bit too. In his particular data set and DB, some of these needlessly complex queries are slightly faster. Ok, but... that doesn't make them correct or desirable. Since we're talking about a 1000x speedup anyway, I hardly think 5 seconds vs 3 seconds seriously matters over simpler codes with less edge cases / problems with other data. Upstream should have a reasonable set of queries that are all-round good, rather than best for only DB X, data Y. Hope this explains more.
          Hide
          Tyler Bannister added a comment -

          When I ran Tony's patch over production data from a client, I saw 50 missing rows for a special guest role created by the client in mdl_stats_daily and 2 missing duplicate rows. Additionally, Tony's patch created 31 additional rows and did not create 82 other rows in mdl_stats_user_daily that were created by the original stats code. That's the discrepancy that I see between the output from Tony's patch and the original stats code.

          I don't know if the results produced by the original stats code are correct or incorrect. I think that should be a separate issue because there could be code in core, blocks, modules or other add ons that depends on the current behavior. Additionally, it could be a very long and involved discussion on what those results should be and that could delay implementation of the performance patch. I would move discussion of what the stats should produce to a different issue to isolate the performance issue from the correctness issue. Especially since the correctness issue could easily morph into a re-design of the logging and stats generation systems that could take many more months to implement.

          There are also practical aspects to consider as well, because whoever does the testing could legitimately reject a performance patch that doesn't produce the same results as the unpatched code out of hand.

          As far as performance goes, the unpatched code took 900 seconds on 1 month, my patched code took 41 seconds on the same month, Tony's took 81 seconds. Using 6 months of production data, my patch took 258 seconds and Tony's took 447 seconds. I did not attempt the unpatched code over 6 months, it was obviously going to be much worse.

          Lastly, I examined Tony's patches and I have an additional patch for them (https://github.com/tbannister/moodle/commit/d0f4422e329b07986050adafd8d7989c55ba8398) that produces output identical to the original statslib code.

          Show
          Tyler Bannister added a comment - When I ran Tony's patch over production data from a client, I saw 50 missing rows for a special guest role created by the client in mdl_stats_daily and 2 missing duplicate rows. Additionally, Tony's patch created 31 additional rows and did not create 82 other rows in mdl_stats_user_daily that were created by the original stats code. That's the discrepancy that I see between the output from Tony's patch and the original stats code. I don't know if the results produced by the original stats code are correct or incorrect. I think that should be a separate issue because there could be code in core, blocks, modules or other add ons that depends on the current behavior. Additionally, it could be a very long and involved discussion on what those results should be and that could delay implementation of the performance patch. I would move discussion of what the stats should produce to a different issue to isolate the performance issue from the correctness issue. Especially since the correctness issue could easily morph into a re-design of the logging and stats generation systems that could take many more months to implement. There are also practical aspects to consider as well, because whoever does the testing could legitimately reject a performance patch that doesn't produce the same results as the unpatched code out of hand. As far as performance goes, the unpatched code took 900 seconds on 1 month, my patched code took 41 seconds on the same month, Tony's took 81 seconds. Using 6 months of production data, my patch took 258 seconds and Tony's took 447 seconds. I did not attempt the unpatched code over 6 months, it was obviously going to be much worse. Lastly, I examined Tony's patches and I have an additional patch for them ( https://github.com/tbannister/moodle/commit/d0f4422e329b07986050adafd8d7989c55ba8398 ) that produces output identical to the original statslib code.
          Hide
          Mike Churchward added a comment -

          Okay, I think I understand all of the issues now...

          So, Tony's patch rewrote the queries that were problematic in a more correct way. Tyler's patch substituted temporary tables for corrected queries. If this is the case, then it does make sense to recommend Tony's patch.

          Now I understand that further, Tony may have modified the queries to correct for potential functional issues with the old system. If this is the case, then I think those corrections should be done in another issue, specifically dealing with those bugs. That way, the performance improvement can be verified easily by checking that the same data is output as before the improvement.

          I believe Tyler has proposed that above, and supplied a modified version of Tony's patch, changes to provide the same output (right or wrong) as before the improvement. That should be the one we recommend for this issue.

          Now, if there are other issues with the function (bugs?), let's create a new issue to deal with them and use Tony's patch with the bug fixes as a candidate for that fix.

          Make sense?

          Show
          Mike Churchward added a comment - Okay, I think I understand all of the issues now... So, Tony's patch rewrote the queries that were problematic in a more correct way. Tyler's patch substituted temporary tables for corrected queries. If this is the case, then it does make sense to recommend Tony's patch. Now I understand that further, Tony may have modified the queries to correct for potential functional issues with the old system. If this is the case, then I think those corrections should be done in another issue, specifically dealing with those bugs. That way, the performance improvement can be verified easily by checking that the same data is output as before the improvement. I believe Tyler has proposed that above, and supplied a modified version of Tony's patch, changes to provide the same output (right or wrong) as before the improvement. That should be the one we recommend for this issue. Now, if there are other issues with the function (bugs?), let's create a new issue to deal with them and use Tony's patch with the bug fixes as a candidate for that fix. Make sense?
          Hide
          Dan Marsden added a comment -

          I'm still not sure that replacing incorrect stats calculations with a faster version that still generates incorrect stats is quite right - especially if those incorrect stats were found as part of the patch. This sounds like the sort of thing that would get rejected by the integrators to me.

          Also - MD has given his +1 to accept a patch that:

          • adds the new field with the read/write type
          • change logging code to make sure that the field is filled during upgrade, and always kept full after that whenever entries are added (possibly slow) or at least on a cron job.

          neither of the patches here do this - Petr has voiced his concerns over the use of temp tables too. (this doesn't necessarily mean the integrators or MD will reject a patch like the one Tony/Tyler have been working on) - their patches are also useful for others wanting to apply a fix to their local installs to speed up stats.

          Show
          Dan Marsden added a comment - I'm still not sure that replacing incorrect stats calculations with a faster version that still generates incorrect stats is quite right - especially if those incorrect stats were found as part of the patch. This sounds like the sort of thing that would get rejected by the integrators to me. Also - MD has given his +1 to accept a patch that: adds the new field with the read/write type change logging code to make sure that the field is filled during upgrade, and always kept full after that whenever entries are added (possibly slow) or at least on a cron job. neither of the patches here do this - Petr has voiced his concerns over the use of temp tables too. (this doesn't necessarily mean the integrators or MD will reject a patch like the one Tony/Tyler have been working on) - their patches are also useful for others wanting to apply a fix to their local installs to speed up stats.
          Hide
          Tony Levi added a comment -

          Neither of the patches do that because there isn't any need and it makes little sense! That part of the calculation is very fast - it will just serve to further bloat the mdl_log.

          Petr's temptable concerns are around mysql not supporting multiple references; part of the rewrite eliminates all multiple-reference cases.

          Tyler, can you give any details about the different rows? Maybe a copy of them? "X different rows" doesn't help track down any problem.

          Again, it is really bad for upstream to have a set of queries that does not work reasonably in all possible cases. That is exactly why this issue exists in the first place.

          Show
          Tony Levi added a comment - Neither of the patches do that because there isn't any need and it makes little sense! That part of the calculation is very fast - it will just serve to further bloat the mdl_log. Petr's temptable concerns are around mysql not supporting multiple references; part of the rewrite eliminates all multiple-reference cases. Tyler, can you give any details about the different rows? Maybe a copy of them? "X different rows" doesn't help track down any problem. Again, it is really bad for upstream to have a set of queries that does not work reasonably in all possible cases. That is exactly why this issue exists in the first place.
          Hide
          Dan Marsden added a comment -

          yep - no argument from me here - I just wanted to make it clear to Mike that Martin hasn't actually given his +1 on this particular design/patch. nice to Hear Petr's concerns are less of an issue with the latest code too.

          Show
          Dan Marsden added a comment - yep - no argument from me here - I just wanted to make it clear to Mike that Martin hasn't actually given his +1 on this particular design/patch. nice to Hear Petr's concerns are less of an issue with the latest code too.
          Hide
          Mike Churchward added a comment -

          Dan - Ultimately, replacing incorrect stats would be a goal, yes. But in a development process, its usually best to tackle one problem at a time so that testing and verification can be made easier. In this case, this issue was created to deal with performance issues. I believe we have come to an agreement on the patch that should be used.

          Now, there was extra changes made in Tony's patch to fix another problem. If we include that with this issue, and testing fails, we will have a difficult time determining whether it was the performance changes or the functional changes. That's why I prefer to separate them. If we can validate the performance changes as being correct, it will be easier to validate the functional changes.

          Next, we don't really have a problem statement for the functional changes that were introduced. We need to know what they are designed to fix, and what the test cases are to verify them.

          I believe we should go ahead with the performance patch, recommending it for release. No guarantees it will be accepted, but in the absence of a full rewrite, it is a better compromise than doing nothing.

          Show
          Mike Churchward added a comment - Dan - Ultimately, replacing incorrect stats would be a goal, yes. But in a development process, its usually best to tackle one problem at a time so that testing and verification can be made easier. In this case, this issue was created to deal with performance issues. I believe we have come to an agreement on the patch that should be used. Now, there was extra changes made in Tony's patch to fix another problem. If we include that with this issue, and testing fails, we will have a difficult time determining whether it was the performance changes or the functional changes. That's why I prefer to separate them. If we can validate the performance changes as being correct, it will be easier to validate the functional changes. Next, we don't really have a problem statement for the functional changes that were introduced. We need to know what they are designed to fix, and what the test cases are to verify them. I believe we should go ahead with the performance patch, recommending it for release. No guarantees it will be accepted, but in the absence of a full rewrite, it is a better compromise than doing nothing.
          Hide
          Tyler Bannister added a comment -

          I've done some additional testing using some new data with 100,000 users. Using this heavier load, my 4 table patch took 15,402s to generate a years worth of daily logs, while my 1 table patch (based on Tony's patch) took 52,039s. On the larger load the 4 table patch was more than 3 times faster.

          Show
          Tyler Bannister added a comment - I've done some additional testing using some new data with 100,000 users. Using this heavier load, my 4 table patch took 15,402s to generate a years worth of daily logs, while my 1 table patch (based on Tony's patch) took 52,039s. On the larger load the 4 table patch was more than 3 times faster.
          Hide
          Tony Levi added a comment -

          So were there any glaring differences in the data or not?

          Show
          Tony Levi added a comment - So were there any glaring differences in the data or not?
          Hide
          Tyler Bannister added a comment - - edited

          I discovered a small performance problem with temporary tables when they are repeatedly created and dropped. Running my test on two years worth of data showed a significant increase in the time it took to create new temporary tables. Accordingly, I've moved the table creation and dropping code out of the daily stats loop, and replaced that drop/create part with "truncate" queries instead. I also added some additional output for weekly and monthly stat generation.

          Tony, to check for differences, I'll have to run the test again with your code. So far I've only been looking at performance with this new, larger, set of data.

          Show
          Tyler Bannister added a comment - - edited I discovered a small performance problem with temporary tables when they are repeatedly created and dropped. Running my test on two years worth of data showed a significant increase in the time it took to create new temporary tables. Accordingly, I've moved the table creation and dropping code out of the daily stats loop, and replaced that drop/create part with "truncate" queries instead. I also added some additional output for weekly and monthly stat generation. Tony, to check for differences, I'll have to run the test again with your code. So far I've only been looking at performance with this new, larger, set of data.
          Hide
          Tony Levi added a comment -

          Interesting, is that persistent across fresh connections, or are you running the 2 years in a single go?

          If it only happens when doing 2 years in a single cron script, I would say that isn't really worth worrying about too much; nice quirk of mysql though.

          Show
          Tony Levi added a comment - Interesting, is that persistent across fresh connections, or are you running the 2 years in a single go? If it only happens when doing 2 years in a single cron script, I would say that isn't really worth worrying about too much; nice quirk of mysql though.
          Hide
          Tyler Bannister added a comment -

          It only occurs when running 2 years in a single go. Obviously, it's not going to be a common occurrence, however, it's actually less work to truncate the tables instead of creating and dropping the tables in the loop. I figure we should probably do it the right way since it also fixes an unusual case at the same time.

          The updated code is available on github

          Show
          Tyler Bannister added a comment - It only occurs when running 2 years in a single go. Obviously, it's not going to be a common occurrence, however, it's actually less work to truncate the tables instead of creating and dropping the tables in the loop. I figure we should probably do it the right way since it also fixes an unusual case at the same time. The updated code is available on github
          Hide
          Tony Levi added a comment -

          Fair enough. And naughty mysql.

          Show
          Tony Levi added a comment - Fair enough. And naughty mysql.
          Hide
          Sam Hemelryk added a comment -

          Hi Tyler, Tony, Dan, Mike and all others involved here.

          Michael assigned this this issue to me so that I can help out or complete things to make sure we don't miss out on the excellent discussion and coding that has gone on here.

          I've been looking over this issue and your patch Tyler at https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed for the past day.
          I like the look of the way things have shaped up. The use of temp tables appears really smart and seems well suited to the present situation.
          While I agree with Petr's earlier comments about the log systems as well as stats needing a general overhaul I think certainly that if we get this code tidied up a little and verify its effects across the 4 DB's we support in core then it should land.

          As mentioned I've already been looking over this code, and have talked to Eloy about this issue and the proposed solution as he really is our database expert.
          As I see it at the moment there are three things that need to happen before we can get this issue up for integration and eventually into core.

          1. Review and tidy up the code.
          There is one large change I believe needs to be made that came from discussions with Eloy. We think it would be best to create temp tables using a method similar to that used when creating the temp backup tables.
          Rather than construct the temp tables in code as is happening now within stats_temp_table_create we think it is best those temporary tables are created from templates.
          In the case of backup that is handled through backup_controller::create_temptable_from_real_table. It then creates temp tables from the real tables that have been set up as templates e.g. backup_ids_template and backup_files_template.
          Taking this approach may not be quite so quick but having those tables defined in a standard way helps make them more visible and manageable in the future.
          In this case things could also be done a little differently. The stats_daily and stats_user_daily tables are nearly identical to the temp tables you are creating. If we get indexes sorted then we could easily use those as the templates for their temp table equivalents.
          The temp log tables are a different story, really they would require a separate template table. Perhaps called stats_log_template.
          Other than that I havn't spotted anything requiring significant changes. Naming of tables and functions is perhaps the only other thing worth mentioning. Naming the temp tables with "_temp" as a prefix worth doing and a more consistent scheme to temp tables already being used in Moodle.

          2. Guarantee results through unit tests with a crafted dataset.
          In discussing this with Eloy we also felt this would be something that would be not only essential to getting this integrated but also highly beneficial in ensuring that things were being generated with the same accuracy that they were prior to this patch. As someone mentioned above this issue should focus on performance. Any efforts to improve stats generation should be undertaken in another issue.
          With unit tests we would have an easy means of testing any and all changes, both as part of this issue and in any future issues concerning stats generation.
          Certainly it will help prove any changes being made in queries.

          3. Ensure operation and performance on all supported databases.
          This is really just about testing. Certainly having the unit tests above will help us in this, but definitely it is still worthwhile running the stats generation on large datasets in all 4 databases and monitoring performance to ensure we only move performance forwards and not backwards in all systems.
          This should also be reflected in the test instruction for this issue when it does get integrated so perhaps creating a means of generating the required data would help.

          Really the first question I have is for Tony, whether you would like continue developing this solution or whether you would like me to take over and finish off development, namely the tasks noted above?
          If you would like to continue development then I will peer-review the code you've got properly, give detailed feedback and help you out in any other way I can.
          If I take over development then I will review and make changes as I go, of course I will make any changes on top of existing commits kudo's aren't lost.
          I'm quite happy either way.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Tyler, Tony, Dan, Mike and all others involved here. Michael assigned this this issue to me so that I can help out or complete things to make sure we don't miss out on the excellent discussion and coding that has gone on here. I've been looking over this issue and your patch Tyler at https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed for the past day. I like the look of the way things have shaped up. The use of temp tables appears really smart and seems well suited to the present situation. While I agree with Petr's earlier comments about the log systems as well as stats needing a general overhaul I think certainly that if we get this code tidied up a little and verify its effects across the 4 DB's we support in core then it should land. As mentioned I've already been looking over this code, and have talked to Eloy about this issue and the proposed solution as he really is our database expert. As I see it at the moment there are three things that need to happen before we can get this issue up for integration and eventually into core. 1. Review and tidy up the code. There is one large change I believe needs to be made that came from discussions with Eloy. We think it would be best to create temp tables using a method similar to that used when creating the temp backup tables. Rather than construct the temp tables in code as is happening now within stats_temp_table_create we think it is best those temporary tables are created from templates. In the case of backup that is handled through backup_controller::create_temptable_from_real_table. It then creates temp tables from the real tables that have been set up as templates e.g. backup_ids_template and backup_files_template. Taking this approach may not be quite so quick but having those tables defined in a standard way helps make them more visible and manageable in the future. In this case things could also be done a little differently. The stats_daily and stats_user_daily tables are nearly identical to the temp tables you are creating. If we get indexes sorted then we could easily use those as the templates for their temp table equivalents. The temp log tables are a different story, really they would require a separate template table. Perhaps called stats_log_template. Other than that I havn't spotted anything requiring significant changes. Naming of tables and functions is perhaps the only other thing worth mentioning. Naming the temp tables with "_ temp " as a prefix worth doing and a more consistent scheme to temp tables already being used in Moodle. 2. Guarantee results through unit tests with a crafted dataset. In discussing this with Eloy we also felt this would be something that would be not only essential to getting this integrated but also highly beneficial in ensuring that things were being generated with the same accuracy that they were prior to this patch. As someone mentioned above this issue should focus on performance. Any efforts to improve stats generation should be undertaken in another issue. With unit tests we would have an easy means of testing any and all changes, both as part of this issue and in any future issues concerning stats generation. Certainly it will help prove any changes being made in queries. 3. Ensure operation and performance on all supported databases. This is really just about testing. Certainly having the unit tests above will help us in this, but definitely it is still worthwhile running the stats generation on large datasets in all 4 databases and monitoring performance to ensure we only move performance forwards and not backwards in all systems. This should also be reflected in the test instruction for this issue when it does get integrated so perhaps creating a means of generating the required data would help. Really the first question I have is for Tony, whether you would like continue developing this solution or whether you would like me to take over and finish off development, namely the tasks noted above? If you would like to continue development then I will peer-review the code you've got properly, give detailed feedback and help you out in any other way I can. If I take over development then I will review and make changes as I go, of course I will make any changes on top of existing commits kudo's aren't lost. I'm quite happy either way. Cheers Sam
          Hide
          Tyler Bannister added a comment -

          I've begun work on the review and code tidy up. I will note that there are some caveats on the create_temptable_from_real_table function, it leaks memory and increases memory usage by approximately 10 MB, but I have none the less incorporated it's xml loading procedure. I will run a test on my big data set soon to see how large the performance impact is.

          If I understand correctly, the temp log tables should be in lib/db/stats_log_template.xml?

          I'm using "temp_" as an additional prefix, this will make the table names (for the standard prefix mdl_) mdl_temp_log1, mdl_temp_log2, mdl_temp_stats_daily and mdl_temp_stats_user_daily. Were you saying that you wanted the tables names to be _temp_log1 or is mdl_temp_log1 acceptable?

          I'm beginning work on test cases now, and I think that may turn out to be the majority of the additional work required.

          Show
          Tyler Bannister added a comment - I've begun work on the review and code tidy up. I will note that there are some caveats on the create_temptable_from_real_table function, it leaks memory and increases memory usage by approximately 10 MB, but I have none the less incorporated it's xml loading procedure. I will run a test on my big data set soon to see how large the performance impact is. If I understand correctly, the temp log tables should be in lib/db/stats_log_template.xml? I'm using "temp_" as an additional prefix, this will make the table names (for the standard prefix mdl_) mdl_temp_log1, mdl_temp_log2, mdl_temp_stats_daily and mdl_temp_stats_user_daily. Were you saying that you wanted the tables names to be _temp_log1 or is mdl_temp_log1 acceptable? I'm beginning work on test cases now, and I think that may turn out to be the majority of the additional work required.
          Hide
          Tyler Bannister added a comment -

          The updated code with test cases is now on Github.

          I changed the file name to temp_stats_log_template.xml to emphasize that it was for temp tables, and I used the <prefix>temp_<table> format for the table names. The test cases took quite a bit longer than the actual fixes, but the test file does test all of the daily stats related functions. All of the db queries will be triggered by the test cases. The should be a problem running the test cases against unmodified code because I changed the name of one of the functions (stats_daily_progress to stats_progress because it's now used to print progress for weekly and monthly stats) so two test cases (test_statslib_progress_debug, and test_statslib_progress_no_debug) won't work. Lastly, I did not write any tests for weekly or monthly stats.

          Show
          Tyler Bannister added a comment - The updated code with test cases is now on Github. I changed the file name to temp_stats_log_template.xml to emphasize that it was for temp tables, and I used the <prefix>temp_<table> format for the table names. The test cases took quite a bit longer than the actual fixes, but the test file does test all of the daily stats related functions. All of the db queries will be triggered by the test cases. The should be a problem running the test cases against unmodified code because I changed the name of one of the functions (stats_daily_progress to stats_progress because it's now used to print progress for weekly and monthly stats) so two test cases (test_statslib_progress_debug, and test_statslib_progress_no_debug) won't work. Lastly, I did not write any tests for weekly or monthly stats.
          Hide
          Matteo Scaramuccia added a comment -

          Hi Tyler,
          thanks for your hard working! I'm wondering if someone, when the issue will be closed for 2.x, will also take care to backport such work in 1.9 (Ref.: MDL-18484) even if 1.9 is "just" under security fixes maintenance.

          Show
          Matteo Scaramuccia added a comment - Hi Tyler, thanks for your hard working! I'm wondering if someone, when the issue will be closed for 2.x, will also take care to backport such work in 1.9 (Ref.: MDL-18484 ) even if 1.9 is "just" under security fixes maintenance.
          Hide
          Dan Marsden added a comment -

          my -1 for a 1.9 backport - it would require significant testing across multiple db types which I'm not too keen on doing myself.
          1.9 is only really supported by us for Serious security issues - if this fix encourages people to upgrade to 2.x that's a good thing!

          Show
          Dan Marsden added a comment - my -1 for a 1.9 backport - it would require significant testing across multiple db types which I'm not too keen on doing myself. 1.9 is only really supported by us for Serious security issues - if this fix encourages people to upgrade to 2.x that's a good thing!
          Hide
          Tyler Bannister added a comment -

          I found a small discrepancy between my code and the original statslib code, when certain logs weren't present a blank daily stats record wasn't produced in the new code that was produced in the original. I've fixed the new code to produce that record and updated the test cases to reflect that those log entries should be present. So Tasks 1 and 2 should be complete now.

          Show
          Tyler Bannister added a comment - I found a small discrepancy between my code and the original statslib code, when certain logs weren't present a blank daily stats record wasn't produced in the new code that was produced in the original. I've fixed the new code to produce that record and updated the test cases to reflect that those log entries should be present. So Tasks 1 and 2 should be complete now.
          Hide
          Michael de Raadt added a comment -

          Taking this out of the Stable Sprint so that it can be worked on when the Sprint version is archived.

          Show
          Michael de Raadt added a comment - Taking this out of the Stable Sprint so that it can be worked on when the Sprint version is archived.
          Hide
          Tyler Bannister added a comment -

          Well, I backported the code anyway since we're going to apply the fix to our customer's M19 sites. The backported code has been attached to one of my comments in MDL-18484.

          Show
          Tyler Bannister added a comment - Well, I backported the code anyway since we're going to apply the fix to our customer's M19 sites. The backported code has been attached to one of my comments in MDL-18484 .
          Hide
          Martin Dougiamas added a comment -

          Thanks very much Tyler. HQ we need to land this ASAP.

          Show
          Martin Dougiamas added a comment - Thanks very much Tyler. HQ we need to land this ASAP.
          Hide
          Jean-Philippe Gaudreau added a comment -

          I've updated the branch to put the one Tyler did.

          Show
          Jean-Philippe Gaudreau added a comment - I've updated the branch to put the one Tyler did.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Eloy Lafuente (stronk7) added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Tyler Bannister added a comment -

          Ok, I've rebased the MDL-30643_master-log-speed and master branches to today's release.

          Show
          Tyler Bannister added a comment - Ok, I've rebased the MDL-30643 _master-log-speed and master branches to today's release.
          Hide
          Aparup Banerjee added a comment -

          Hi All,
          I've picked this up for review pretty much expecting this to be able to land soon given everyone's dedicated involvement here. However,
          looks like we need some minor touch up (to MDL-30643_master-log-speed branch) here before touch down:

          • there are 4 calls to get_context_instance() which need to call either context_course::instance() or context_system::instance() as the former is deprecated now for master.
          • i've attached a log (MDL-30643_pu.log) of my phpunit test failures, i'm not sure why they're failing for me. (php 5.4.5 here). will look at this in a few hours.
          • suggestion : the statistics cron runs fine for me but on hardly any data, it would be cool to have some generated dataset with variety to test performance here or perhaps some data with benchmarks for comparison against this patch in the test instructions. (maybe some comment in the test code about the expected speed for some given system just as a scale to compare against)

          cheers!

          Show
          Aparup Banerjee added a comment - Hi All, I've picked this up for review pretty much expecting this to be able to land soon given everyone's dedicated involvement here. However, looks like we need some minor touch up (to MDL-30643 _master-log-speed branch) here before touch down: there are 4 calls to get_context_instance() which need to call either context_course::instance() or context_system::instance() as the former is deprecated now for master. i've attached a log ( MDL-30643 _pu.log) of my phpunit test failures, i'm not sure why they're failing for me. (php 5.4.5 here). will look at this in a few hours. suggestion : the statistics cron runs fine for me but on hardly any data, it would be cool to have some generated dataset with variety to test performance here or perhaps some data with benchmarks for comparison against this patch in the test instructions. (maybe some comment in the test code about the expected speed for some given system just as a scale to compare against) cheers!
          Hide
          Tyler Bannister added a comment -

          Hi Aparup,

          1. I've removed the references to get_context_instance() and I realized that I had not put the final changes into the MDL-30643_master-log-speed branch. The final change adds another temporary table that eliminates the repeated joins between mdl_enrol and mdl_user_enrolments. It mirrors a similar change I made to the MDL-18484 patch when back porting the fixes to Moodle 1.9.
          2. It looks like there were some errors in the test case handling of time zones. I think I've got the test case issues fixed.
          3. I have a dataset that I tested on, however, it's about 26 GB uncompressed and takes about 7 days to load into a distribution standard MySQL server. I don't think I can get it down to a manageable size.
          Show
          Tyler Bannister added a comment - Hi Aparup, I've removed the references to get_context_instance() and I realized that I had not put the final changes into the MDL-30643_master-log-speed branch. The final change adds another temporary table that eliminates the repeated joins between mdl_enrol and mdl_user_enrolments. It mirrors a similar change I made to the MDL-18484 patch when back porting the fixes to Moodle 1.9. It looks like there were some errors in the test case handling of time zones. I think I've got the test case issues fixed. I have a dataset that I tested on, however, it's about 26 GB uncompressed and takes about 7 days to load into a distribution standard MySQL server. I don't think I can get it down to a manageable size.
          Hide
          Aparup Banerjee added a comment -

          thanks for that

          that looks good now and has been integrated into master for further testing..

          please feel free to run any tests against integration code with any data available to you.

          Show
          Aparup Banerjee added a comment - thanks for that that looks good now and has been integrated into master for further testing.. please feel free to run any tests against integration code with any data available to you.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Hi, FYI I'm getting 10 phpunit test failures with your patch applied against postgresql (seems to pass ok against mysql):

          http://stronk7.doesntexist.com/view/master/job/07.%20Run%20phpunit%20UnitTests%20(master)/1010/testReport/

          (I also get some other temptable errors at the end of the execution, perhaps you are missing to drop them after use or so?)

          So, if you can investigate that a bit...TIA!

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Hi, FYI I'm getting 10 phpunit test failures with your patch applied against postgresql (seems to pass ok against mysql): http://stronk7.doesntexist.com/view/master/job/07.%20Run%20phpunit%20UnitTests%20(master)/1010/testReport/ (I also get some other temptable errors at the end of the execution, perhaps you are missing to drop them after use or so?) So, if you can investigate that a bit...TIA! Ciao
          Hide
          Petr Škoda added a comment -

          hi, the "protected function prepare_db($logs) {" is wrong, tests should not insert data directly into system tables - we have data generator for that...

          Show
          Petr Škoda added a comment - hi, the "protected function prepare_db($logs) {" is wrong, tests should not insert data directly into system tables - we have data generator for that...
          Hide
          Petr Škoda added a comment -

          To integrators: next time please make sure integrated code uses our SQL indentation style, thanks.

          Show
          Petr Škoda added a comment - To integrators: next time please make sure integrated code uses our SQL indentation style, thanks.
          Hide
          Michael de Raadt added a comment - - edited

          Test result: Unsuccessful.

          This ran fine on MySQL, but I ran into the following errors when I ran cron with other DBs...

          • MS SQL
            Running daily statistics gathering, starting at 1334073600:
            Temporary tables created
            Enrolments caclulated
            !!! Error writing to database !!!
            !! SQLState: 23000<br />
            Error Code: 8101<br />
            Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]An explicit value for the identity column in table '#mdl_temp_log1' can only be specified when a column list is used and IDENTITY_INSERT is ON.<br />
            
            INSERT INTO #mdl_temp_log1
                            SELECT id, userid, course, action FROM mdl_log l
                            WHERE l.time &gt;= 1334073600 AND l.time &lt; 1334160000
            [array (
            )]
            Error code: dmlwriteexception !!
            !! Stack trace: * line 410 of \lib\dml\moodle_database.php: dml_write_exception thrown
            * line 258 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database-&gt;query_end()
            * line 365 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database-&gt;query_end()
            * line 740 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database-&gt;do_query()
            * line 1608 of \lib\statslib.php: call to sqlsrv_native_moodle_database-&gt;execute()
            * line 194 of \lib\statslib.php: call to stats_temp_table_fill()
            * line 451 of \lib\cronlib.php: call to stats_cron_daily()
            * line 88 of \admin\cron.php: call to cron_run()
             !!
            
          • PostgreSQL
            Running daily statistics gathering, starting at 1332864000:
            Temporary tables created
            Enrolments caclulated
            !!! Error writing to database !!!
            !! ERROR:  duplicate key value violates unique constraint "mdl_templog1_id_pk"
            DETAIL:  Key (id)=(1) already exists.
            INSERT INTO mdl_temp_log1 (userid,course,action) VALUES($1,$2,$3) RETURNING id
            [array (
              'userid' =&gt; 0,
              'course' =&gt; '1',
              'action' =&gt; '',
            )]
            Error code: dmlwriteexception !!
            !! Stack trace: * line 410 of \lib\dml\moodle_database.php: dml_write_exception thrown
            * line 239 of \lib\dml\pgsql_native_moodle_database.php: call to moodle_database-&gt;query_end()
            * line 819 of \lib\dml\pgsql_native_moodle_database.php: call to pgsql_native_moodle_database-&gt;query_end()
            * line 206 of \lib\statslib.php: call to pgsql_native_moodle_database-&gt;insert_record_raw()
            * line 451 of \lib\cronlib.php: call to stats_cron_daily()
            * line 88 of \admin\cron.php: call to cron_run()
             !!
            
          • Oracle
            Running daily statistics gathering, starting at 1334073600:
            Temporary tables created
            Enrolments caclulated
            !!! Error reading from database !!!
            !! ORA-00001: unique constraint (MOODLEUSER.M_TEMPLOG1_ID_PK) violated
            
            [NULL]
            Error code: dmlreadexception !!
            !! Stack trace: * line 407 of \lib\dml\moodle_database.php: dml_read_exception thrown
            * line 274 of \lib\dml\oci_native_moodle_database.php: call to moodle_database-&gt;query_end()
            * line 1208 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database-&gt;query_end()
            * line 206 of \lib\statslib.php: call to oci_native_moodle_database-&gt;insert_record_raw()
            * line 451 of \lib\cronlib.php: call to stats_cron_daily()
            * line 88 of \admin\cron.php: call to cron_run()
             !!
            
          Show
          Michael de Raadt added a comment - - edited Test result: Unsuccessful. This ran fine on MySQL, but I ran into the following errors when I ran cron with other DBs... MS SQL Running daily statistics gathering, starting at 1334073600: Temporary tables created Enrolments caclulated !!! Error writing to database !!! !! SQLState: 23000<br /> Error Code: 8101<br /> Message: [Microsoft][SQL Server Native Client 10.0][SQL Server]An explicit value for the identity column in table '#mdl_temp_log1' can only be specified when a column list is used and IDENTITY_INSERT is ON.<br /> INSERT INTO #mdl_temp_log1 SELECT id, userid, course, action FROM mdl_log l WHERE l.time &gt;= 1334073600 AND l.time &lt; 1334160000 [array ( )] Error code: dmlwriteexception !! !! Stack trace: * line 410 of \lib\dml\moodle_database.php: dml_write_exception thrown * line 258 of \lib\dml\sqlsrv_native_moodle_database.php: call to moodle_database-&gt;query_end() * line 365 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database-&gt;query_end() * line 740 of \lib\dml\sqlsrv_native_moodle_database.php: call to sqlsrv_native_moodle_database-&gt;do_query() * line 1608 of \lib\statslib.php: call to sqlsrv_native_moodle_database-&gt;execute() * line 194 of \lib\statslib.php: call to stats_temp_table_fill() * line 451 of \lib\cronlib.php: call to stats_cron_daily() * line 88 of \admin\cron.php: call to cron_run() !! PostgreSQL Running daily statistics gathering, starting at 1332864000: Temporary tables created Enrolments caclulated !!! Error writing to database !!! !! ERROR: duplicate key value violates unique constraint "mdl_templog1_id_pk" DETAIL: Key (id)=(1) already exists. INSERT INTO mdl_temp_log1 (userid,course,action) VALUES($1,$2,$3) RETURNING id [array ( 'userid' =&gt; 0, 'course' =&gt; '1', 'action' =&gt; '', )] Error code: dmlwriteexception !! !! Stack trace: * line 410 of \lib\dml\moodle_database.php: dml_write_exception thrown * line 239 of \lib\dml\pgsql_native_moodle_database.php: call to moodle_database-&gt;query_end() * line 819 of \lib\dml\pgsql_native_moodle_database.php: call to pgsql_native_moodle_database-&gt;query_end() * line 206 of \lib\statslib.php: call to pgsql_native_moodle_database-&gt;insert_record_raw() * line 451 of \lib\cronlib.php: call to stats_cron_daily() * line 88 of \admin\cron.php: call to cron_run() !! Oracle Running daily statistics gathering, starting at 1334073600: Temporary tables created Enrolments caclulated !!! Error reading from database !!! !! ORA-00001: unique constraint (MOODLEUSER.M_TEMPLOG1_ID_PK) violated [NULL] Error code: dmlreadexception !! !! Stack trace: * line 407 of \lib\dml\moodle_database.php: dml_read_exception thrown * line 274 of \lib\dml\oci_native_moodle_database.php: call to moodle_database-&gt;query_end() * line 1208 of \lib\dml\oci_native_moodle_database.php: call to oci_native_moodle_database-&gt;query_end() * line 206 of \lib\statslib.php: call to oci_native_moodle_database-&gt;insert_record_raw() * line 451 of \lib\cronlib.php: call to stats_cron_daily() * line 88 of \admin\cron.php: call to cron_run() !!
          Hide
          Aparup Banerjee added a comment - - edited

          looks like this is a candidate for reversion before rolling..

          • protected function prepare_db($logs) is indeed writing to system tables . an example for datagenerator is at course/lib/courselib_test.php test_reorder_sections().
          • the failures for cron noted too on every DB except mysql (which is probably not reporting it). Thanks for testing Michael.

          ps: http://docs.moodle.org/dev/Writing_PHPUnit_tests could use some elaboration on data generator.

          EDIT: and Petr's reference to sql style has this upcoming doc for SQL @ http://tracker.moodle.org/browse/MDLSITE-1914

          Show
          Aparup Banerjee added a comment - - edited looks like this is a candidate for reversion before rolling.. protected function prepare_db($logs) is indeed writing to system tables . an example for datagenerator is at course/lib/courselib_test.php test_reorder_sections(). the failures for cron noted too on every DB except mysql (which is probably not reporting it). Thanks for testing Michael. ps: http://docs.moodle.org/dev/Writing_PHPUnit_tests could use some elaboration on data generator. EDIT: and Petr's reference to sql style has this upcoming doc for SQL @ http://tracker.moodle.org/browse/MDLSITE-1914
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Reverted 8 (from tyler) + 1 (from Apu) commits. This needs a bit more of love before landing. Reopening, thanks!

          Show
          Eloy Lafuente (stronk7) added a comment - Reverted 8 (from tyler) + 1 (from Apu) commits. This needs a bit more of love before landing. Reopening, thanks!
          Hide
          Tyler Bannister added a comment -

          I've rewritten the test cases to use the data generators. That change should, I think, fix one of the PostgresSQL test failures (casting "varying" to "bigint").

          The MS SQL error appears to be easily solved by dropping the id column and specifying the values, when the data log if filled. Apparently MS SQL doesn't normally allow primary keys to be specified.

          From most of the test errors and the error output posted by Michael above, it looks like there may be an error in insert_record_raw for PostgreSQL and Oracle. Under MySQL insert_record_raw generates a new unique key for the id column, but it doesn't appear to do so in PostgreSQL and Oracle. I don't know why the behavior is different.

          In any case, I'll change the call on 206 to insert_record and see if that fixes the problem. I should updated code ready tomorrow morning.

          Show
          Tyler Bannister added a comment - I've rewritten the test cases to use the data generators. That change should, I think, fix one of the PostgresSQL test failures (casting "varying" to "bigint"). The MS SQL error appears to be easily solved by dropping the id column and specifying the values, when the data log if filled. Apparently MS SQL doesn't normally allow primary keys to be specified. From most of the test errors and the error output posted by Michael above, it looks like there may be an error in insert_record_raw for PostgreSQL and Oracle. Under MySQL insert_record_raw generates a new unique key for the id column, but it doesn't appear to do so in PostgreSQL and Oracle. I don't know why the behavior is different. In any case, I'll change the call on 206 to insert_record and see if that fixes the problem. I should updated code ready tomorrow morning.
          Hide
          Petr Škoda added a comment -

          insert_record_raw() is a low level method, it does not do any data validation, please always use only insert_record().

          Show
          Petr Škoda added a comment - insert_record_raw() is a low level method, it does not do any data validation, please always use only insert_record().
          Hide
          CiBoT added a comment -

          Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

          Show
          CiBoT added a comment - Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.
          Hide
          Tyler Bannister added a comment -

          Petr,

          Just to be clear, the problem wasn't validation, it was failure to generate a suitable primary key id. In the MySQL implementation of insert_record_raw, a valid primary key id will be generated. If an id is passed, the MySQL insert_record_raw will drop the id and generate a new one. In this case, no id was passed and an already used primary key id was generated for PostgreSQL and Oracle. The code worked in MySQL because the insert_record_raw did the right thing for MySQL and it failed in PostgreSQL and Oracle because insert_record_raw did the wrong thing for those databases.

          But yes, it should have been insert_record() instead.

          Show
          Tyler Bannister added a comment - Petr, Just to be clear, the problem wasn't validation, it was failure to generate a suitable primary key id. In the MySQL implementation of insert_record_raw, a valid primary key id will be generated. If an id is passed, the MySQL insert_record_raw will drop the id and generate a new one. In this case, no id was passed and an already used primary key id was generated for PostgreSQL and Oracle. The code worked in MySQL because the insert_record_raw did the right thing for MySQL and it failed in PostgreSQL and Oracle because insert_record_raw did the wrong thing for those databases. But yes, it should have been insert_record() instead.
          Hide
          Petr Škoda added a comment -

          1/ Oh, the problem may be somewhere else - you can not just insert records with fixed 'id' into our tables in function stats_temp_table_fill(), you MUST reset table sequences afterwards using $database_manager->reset_sequence($tablename).

          2/ the $DB->insert_record_raw('temp_log1', array('userid' => 0, 'course' => SITEID, 'action' => '')); expects normalised values for given database - in this particular case action value '' would be converted to NULL in Oracle which is wrong.

          Show
          Petr Škoda added a comment - 1/ Oh, the problem may be somewhere else - you can not just insert records with fixed 'id' into our tables in function stats_temp_table_fill(), you MUST reset table sequences afterwards using $database_manager->reset_sequence($tablename). 2/ the $DB->insert_record_raw('temp_log1', array('userid' => 0, 'course' => SITEID, 'action' => '')); expects normalised values for given database - in this particular case action value '' would be converted to NULL in Oracle which is wrong.
          Hide
          Tyler Bannister added a comment -

          Thanks Petr, I didn't realize empty strings would become NULLs in Oracle.

          I've now updated my MDL-30643_master-log-speed branch with fixes for all of the testing issues mentioned above. I've setup a local PostgreSQL DB and tested the updated code locally on both PostgreSQL and MySQL and it's passing all the tests for both.

          I don't think I can get around having to insert the log entries directly into the database (using insert_record now) because I need to create log events that occurred in the past, and add_to_log() always uses the current time.

          Show
          Tyler Bannister added a comment - Thanks Petr, I didn't realize empty strings would become NULLs in Oracle. I've now updated my MDL-30643 _master-log-speed branch with fixes for all of the testing issues mentioned above. I've setup a local PostgreSQL DB and tested the updated code locally on both PostgreSQL and MySQL and it's passing all the tests for both. I don't think I can get around having to insert the log entries directly into the database (using insert_record now) because I need to create log events that occurred in the past, and add_to_log() always uses the current time.
          Hide
          Petr Škoda added a comment -

          Thanks Tyler for working on this issue and especially for listening to feedback!

          Show
          Petr Škoda added a comment - Thanks Tyler for working on this issue and especially for listening to feedback!
          Hide
          Tyler Bannister added a comment -

          When is the next opportunity to get this issue back into integration and testing?

          Show
          Tyler Bannister added a comment - When is the next opportunity to get this issue back into integration and testing?
          Hide
          Tyler Bannister added a comment -

          The issues identified in testing should be fixed now. The changes have been tested in MySQL and PostgreSQL.

          Show
          Tyler Bannister added a comment - The issues identified in testing should be fixed now. The changes have been tested in MySQL and PostgreSQL.
          Hide
          Aparup Banerjee added a comment -

          Tyler: integration starts weekly on mondays and testing is weekly on wednesdays (barring special release weeks/freeze etc)

          Show
          Aparup Banerjee added a comment - Tyler: integration starts weekly on mondays and testing is weekly on wednesdays (barring special release weeks/freeze etc)
          Hide
          Tyler Bannister added a comment -

          Thanks Agarup, I assume requesting a peer review was the proper way to get that process rolling.

          Show
          Tyler Bannister added a comment - Thanks Agarup, I assume requesting a peer review was the proper way to get that process rolling.
          Hide
          Aparup Banerjee added a comment -

          that's right Tyler

          Show
          Aparup Banerjee added a comment - that's right Tyler
          Hide
          Tyler Bannister added a comment -

          Ok, this has been waiting for review for more than a month now. Is there anything we can do to encourage the review?

          Show
          Tyler Bannister added a comment - Ok, this has been waiting for review for more than a month now. Is there anything we can do to encourage the review?
          Hide
          Aparup Banerjee added a comment -

          sending this straight up for integration review.

          Show
          Aparup Banerjee added a comment - sending this straight up for integration review.
          Hide
          Aparup Banerjee added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Aparup Banerjee added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Tyler Bannister added a comment -

          Ok, I've rebased the branch.

          Show
          Tyler Bannister added a comment - Ok, I've rebased the branch.
          Hide
          Dan Poltawski added a comment -

          The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week.

          TIA and ciao

          Show
          Dan Poltawski added a comment - The main moodle.git repository has just been updated with latest weekly modifications. You may wish to rebase your PULL branches to simplify history and avoid any possible merge conflicts. This would also make integrator's life easier next week. TIA and ciao
          Hide
          Tyler Bannister added a comment - - edited

          Ok, I've rebased the branch again. Is there any chance we can get the integration review done before the next weekly update?

          Show
          Tyler Bannister added a comment - - edited Ok, I've rebased the branch again. Is there any chance we can get the integration review done before the next weekly update?
          Hide
          Aparup Banerjee added a comment -

          Hi Tyler,

          I've just looked at the commits here and they have commits mentioning 'Revert "Revert ...."' . In the case of reverted issue/commits, the same rebasing strategy should be applied instead re-reverting the commits. (i've clarified this with a note here just now http://docs.moodle.org/dev/Integration_Review#Schedule)

          For now i've picked and reworded the commits here : https://github.com/nebgor/moodle/compare/int_master...int_master_MDL-30643

          Show
          Aparup Banerjee added a comment - Hi Tyler, I've just looked at the commits here and they have commits mentioning 'Revert "Revert ...."' . In the case of reverted issue/commits, the same rebasing strategy should be applied instead re-reverting the commits. (i've clarified this with a note here just now http://docs.moodle.org/dev/Integration_Review#Schedule ) For now i've picked and reworded the commits here : https://github.com/nebgor/moodle/compare/int_master...int_master_MDL-30643
          Hide
          Aparup Banerjee added a comment -

          Hi Tyler,
          I've gone through this now (with Petr who was in HQ for the day actually) and here are my notes :

          1. As Sam mentioned in his comment, a function similar to backup_controller::create_temptable_from_real_table seems the best way to go but this seems to work atm so i'm asking Eloy about his opinion.
          2. TRUNCATE is not really safe to use here perhaps look at using $DB->delete_records($name) there which does the job too.
          3. https://github.com/nebgor/moodle/compare/int_master...int_master_MDL-30643#L1R1579 stats_temp_table_fill($timestart, $timeend) has code which seems sql-injectable , perhaps ensure that they're integer.
          4. there are print_r() calls within the phpunit tests as well as trigger_error() calls. phpunit tests will be run from the cli and these do not apply here well at all. if you're looking to report a failure here, i'd suggest using an assert statement else i'd remove them.
          5. Also see the trigger error here although i'm not sure if asserts are fine in the setup stage.
          6. the large arrays of test data can be from loaded from csv/xml type files which would make this lots more readable (in a separate test data file) : see lib/phpunit/classes/advanced_testcase.php for createXMLDataSet(), createCsvDataSet() and friends there. (available as part of advanced_testcase).
          7. test_statslib_get_action_names() has lots of hardcoded numbers,
          8. if (!$DB->execute()) type calls aren't going to bode well here as the call only ever returns true and throws dml_exception otherwise. I'd just let it throw an exception as this sort of failure would mean phpunit isn't going anywhere later.
          9. test_statslib_progress_no_debug() looks like it can easily be a false positive in future after code or simple string changes. i'm not sure how to better test this atm.
          10. test_statslib_get_action_names() has the hard coded actions, perhaps these could be constants in stats lib or somewhere in logs ? not sure.
            SQL:
          11. there are minor inconsistencies with the coding style with regards to keywords such as 'COUNT' and 'AS' etc within statslib.php that could be made more consistent (or according to what might be evolving @ MDLSITE-1914)

          overall i think this should go in once its cleaned up as it definitely seems to be helpful
          imo, the lib/db/temp_stats_log_template.xml file seems to be starting an undocumented convention whereas as suggested this could be created from an existing table defined within install.xml. (again, i'll ping Eloy here about this)

          I hope thats helpful feedback Tyler, looking to see this land.

          Cheers,
          Aparup

          ps: Another point is that if this is really useful, then we have a need for a proper temp-table usage api within some DB layer API. this could possibly work similar to backup_controller::create_temptable_from_real_table().

          pps: rebase and get rid of those recursive revert messages

          Show
          Aparup Banerjee added a comment - Hi Tyler, I've gone through this now (with Petr who was in HQ for the day actually) and here are my notes : As Sam mentioned in his comment, a function similar to backup_controller::create_temptable_from_real_table seems the best way to go but this seems to work atm so i'm asking Eloy about his opinion. TRUNCATE is not really safe to use here perhaps look at using $DB->delete_records($name) there which does the job too. https://github.com/nebgor/moodle/compare/int_master...int_master_MDL-30643#L1R1579 stats_temp_table_fill($timestart, $timeend) has code which seems sql-injectable , perhaps ensure that they're integer. there are print_r() calls within the phpunit tests as well as trigger_error() calls. phpunit tests will be run from the cli and these do not apply here well at all. if you're looking to report a failure here, i'd suggest using an assert statement else i'd remove them. Also see the trigger error here although i'm not sure if asserts are fine in the setup stage. the large arrays of test data can be from loaded from csv/xml type files which would make this lots more readable (in a separate test data file) : see lib/phpunit/classes/advanced_testcase.php for createXMLDataSet(), createCsvDataSet() and friends there. (available as part of advanced_testcase). test_statslib_get_action_names() has lots of hardcoded numbers, if (!$DB->execute()) type calls aren't going to bode well here as the call only ever returns true and throws dml_exception otherwise. I'd just let it throw an exception as this sort of failure would mean phpunit isn't going anywhere later. test_statslib_progress_no_debug() looks like it can easily be a false positive in future after code or simple string changes. i'm not sure how to better test this atm. test_statslib_get_action_names() has the hard coded actions, perhaps these could be constants in stats lib or somewhere in logs ? not sure. SQL: there are minor inconsistencies with the coding style with regards to keywords such as 'COUNT' and 'AS' etc within statslib.php that could be made more consistent (or according to what might be evolving @ MDLSITE-1914 ) overall i think this should go in once its cleaned up as it definitely seems to be helpful imo, the lib/db/temp_stats_log_template.xml file seems to be starting an undocumented convention whereas as suggested this could be created from an existing table defined within install.xml. (again, i'll ping Eloy here about this) I hope thats helpful feedback Tyler, looking to see this land. Cheers, Aparup ps: Another point is that if this is really useful, then we have a need for a proper temp-table usage api within some DB layer API. this could possibly work similar to backup_controller::create_temptable_from_real_table(). pps: rebase and get rid of those recursive revert messages
          Hide
          Tyler Bannister added a comment -

          Aparup, I'm afraid your comment on the reverts isn't clearing things up for me very much. I originally reverted the revert of my commits because they would no longer apply cleanly onto the master branch (I think, because they were reverted). Only my newest commits were applied. If I understand correctly, you're saying that instead of reverting them I should cherry-pick (or otherwise add) the commits back onto the tail end of the master branch? Rebasing was a disaster when I tried it. Instead of reverting the commits and editing the message so that the REVERT "REVERT " part of the message is removed.

          On the bullet points:

          1. That might be a good basis for an improvement issue.
          2. Ok
          3. You are correct that it would be SQL-injectable (I'll fix that) but it was only ever called with internally generated values.
          4. I think only one of the print_r codes is reachable in the code, the others are commented out (and will be deleted). I can remove it, but it's there to help debug the test case in case it fails to setup properly. I don't think the assert can be called safely in any of the cases where trigger_error is called they are all pre-test case code.
          5. I don't think they are. The other print_errors are also in setup code.
          6. Ok
          7. I assume you mean strings (the numbers are irrelevant). I don't know of a better way to generate those lists.
          8. Ok. I forgot to remove the if statements when I ported that from my MOODLE_19_STABLE stats code.
          9. I could make the string that's print for nodebug a defined value (STATSLIB_PLACEHOLDER_OUTPUT or something like that).
          10. I think adding defined names for those actions falls outside of my scope here (since the actions names are defined in the logging code).
          11. I might as well fix up the COUNTs and ASs. I assume you mean they should all be all-caps?
          Show
          Tyler Bannister added a comment - Aparup, I'm afraid your comment on the reverts isn't clearing things up for me very much. I originally reverted the revert of my commits because they would no longer apply cleanly onto the master branch (I think, because they were reverted). Only my newest commits were applied. If I understand correctly, you're saying that instead of reverting them I should cherry-pick (or otherwise add) the commits back onto the tail end of the master branch? Rebasing was a disaster when I tried it. Instead of reverting the commits and editing the message so that the REVERT "REVERT " part of the message is removed. On the bullet points: That might be a good basis for an improvement issue. Ok You are correct that it would be SQL-injectable (I'll fix that) but it was only ever called with internally generated values. I think only one of the print_r codes is reachable in the code, the others are commented out (and will be deleted). I can remove it, but it's there to help debug the test case in case it fails to setup properly. I don't think the assert can be called safely in any of the cases where trigger_error is called they are all pre-test case code. I don't think they are. The other print_errors are also in setup code. Ok I assume you mean strings (the numbers are irrelevant). I don't know of a better way to generate those lists. Ok. I forgot to remove the if statements when I ported that from my MOODLE_19_STABLE stats code. I could make the string that's print for nodebug a defined value (STATSLIB_PLACEHOLDER_OUTPUT or something like that). I think adding defined names for those actions falls outside of my scope here (since the actions names are defined in the logging code). I might as well fix up the COUNTs and ASs. I assume you mean they should all be all-caps?
          Hide
          Aparup Banerjee added a comment - - edited

          Hi Tyler,
          Yup cherry-picking would do too for commits that don't change. any effort to achieve the rebase concept really. I believe rebasing fails usually with larger strings of commits where not every commit is changed. Hm, perhaps the rebase message can include some hint towards handling of reverts. (this has messed up a few devs here at HQ and i'm hoping to make this potential time waster go away)

          On the points (avoiding bullets :-D):

          1. yes agreed, it seems theres more thought needed here bbut for now (after discussing with Eloy), to create less overhead for changes in future, seems better to stick with what exists and afford the 2-3 secs needed for having the tables defined as standard i.e.: having the table definition inside install.xml and creating a copy for temp usage. I'll create an issue about making this more standardised/normalised (MDL-36410).
          2. cool, Altitude 1000ft
          3. Just staying safe, Altitude 900ft
          4. I'm not able to understand why the setup would fail code-wise. Other unit tests should pick up DB failures otherwise its infrastructure setup failing. In any case, imo this failure would be beyond the scope of the unit test. If debugging() is required, perhaps they should be within the code being tested rather than within the test itself.
          5. cool. I think there is scope for this in another phpunit improvement issue. (MDL-36411)
          6. Altitude 800ft.
          7. oops - i hadn't finished that one and can't recall but i think some 'future proofing against development changes' direction was there. don't think its any game stopper.
          8. Altitude 700ft.
          9. agreed. Altitude 600ft.
          10. yup out of scope, (MDL-36416)
          11. yes. (hopefully this changes a few commits to allow an easy rebase instead of picking)

          (/me turns off MSAW for http://www.youtube.com/watch?v=v_z5HtME9n8)

          Show
          Aparup Banerjee added a comment - - edited Hi Tyler, Yup cherry-picking would do too for commits that don't change. any effort to achieve the rebase concept really. I believe rebasing fails usually with larger strings of commits where not every commit is changed. Hm, perhaps the rebase message can include some hint towards handling of reverts. (this has messed up a few devs here at HQ and i'm hoping to make this potential time waster go away) On the points (avoiding bullets :-D): yes agreed, it seems theres more thought needed here bbut for now (after discussing with Eloy), to create less overhead for changes in future, seems better to stick with what exists and afford the 2-3 secs needed for having the tables defined as standard i.e.: having the table definition inside install.xml and creating a copy for temp usage. I'll create an issue about making this more standardised/normalised ( MDL-36410 ). cool, Altitude 1000ft Just staying safe, Altitude 900ft I'm not able to understand why the setup would fail code-wise. Other unit tests should pick up DB failures otherwise its infrastructure setup failing. In any case, imo this failure would be beyond the scope of the unit test. If debugging() is required, perhaps they should be within the code being tested rather than within the test itself. cool. I think there is scope for this in another phpunit improvement issue. ( MDL-36411 ) Altitude 800ft. oops - i hadn't finished that one and can't recall but i think some 'future proofing against development changes' direction was there. don't think its any game stopper. Altitude 700ft. agreed. Altitude 600ft. yup out of scope, ( MDL-36416 ) yes. (hopefully this changes a few commits to allow an easy rebase instead of picking) (/me turns off MSAW for http://www.youtube.com/watch?v=v_z5HtME9n8 )
          Hide
          Aparup Banerjee added a comment -

          ping! any changes? this is seeming like it won't make for 2.4 atm.

          Show
          Aparup Banerjee added a comment - ping! any changes? this is seeming like it won't make for 2.4 atm.
          Hide
          Aparup Banerjee added a comment -

          I'm reopening this for now. (unfortunately point 1 is the sticking point)

          Please resubmit when ready.
          (ping for faster response in devchat if necessary for 2.4)

          Show
          Aparup Banerjee added a comment - I'm reopening this for now. (unfortunately point 1 is the sticking point) Please resubmit when ready. (ping for faster response in devchat if necessary for 2.4)
          Hide
          Tyler Bannister added a comment - - edited

          Ok, just to be clean for #1 you want me to remove the xml file for the temporary database tables and simply add them into the primary install.xml file?

          I'm working on #6 right now. I takes a while to work out how all of this is going to work together, I could not find any examples in Core where an XML file with placeholder values has to be loaded and parsed and then compared against the contents of the database.

          I think everything except #1 and #6 is complete now.

          Also, Aparup, could you make sure that test case issues outlined here are documented somewhere, it's a bit frustrating to have to keep rewriting test cases to meet what seems like an evolving standard. For instance the Writing PHPUnit tests page does not mention using XML/CSV files to load data at all.

          Show
          Tyler Bannister added a comment - - edited Ok, just to be clean for #1 you want me to remove the xml file for the temporary database tables and simply add them into the primary install.xml file? I'm working on #6 right now. I takes a while to work out how all of this is going to work together, I could not find any examples in Core where an XML file with placeholder values has to be loaded and parsed and then compared against the contents of the database. I think everything except #1 and #6 is complete now. Also, Aparup, could you make sure that test case issues outlined here are documented somewhere, it's a bit frustrating to have to keep rewriting test cases to meet what seems like an evolving standard. For instance the Writing PHPUnit tests page does not mention using XML/CSV files to load data at all.
          Hide
          Tyler Bannister added a comment -

          Ok, I've updated my branch with the code changes as requested.

          Show
          Tyler Bannister added a comment - Ok, I've updated my branch with the code changes as requested.
          Hide
          Tyler Bannister added a comment -

          Let try and get this done for 2.4.

          Show
          Tyler Bannister added a comment - Let try and get this done for 2.4.
          Hide
          Tyler Bannister added a comment -

          As per Eloy's comment in dev chat, I've renamed the temp tables in install.xml to use the name syntax of "temp_<table>_template". That should avoid any possible problems with name conflicts. The updated code has been pushed to Github.

          Show
          Tyler Bannister added a comment - As per Eloy's comment in dev chat, I've renamed the temp tables in install.xml to use the name syntax of "temp_<table>_template". That should avoid any possible problems with name conflicts. The updated code has been pushed to Github.
          Hide
          Aparup Banerjee added a comment -

          Cool, thanks Tyler.
          (ftr:dev chat @ https://moodle.org/local/chatlogs/index.php?conversationid=11443)

          I've added http://docs.moodle.org/dev/Writing_PHPUnit_tests#Large_test_data which hopefully improves visibility of the short explanations as well as tagged MDL-32960 as dev_docs_required (for stub expansion).

          Show
          Aparup Banerjee added a comment - Cool, thanks Tyler. (ftr:dev chat @ https://moodle.org/local/chatlogs/index.php?conversationid=11443 ) I've added http://docs.moodle.org/dev/Writing_PHPUnit_tests#Large_test_data which hopefully improves visibility of the short explanations as well as tagged MDL-32960 as dev_docs_required (for stub expansion).
          Hide
          Aparup Banerjee added a comment -

          Hi Tyler,
          i've just come across one bug (minor) in the unit test:
          https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed#L13R40

          1) statslib_daily_testcase::test_statslib_get_start_from
          Log entry start
          Failed asserting that '1272657610' matches expected 1272700810.
          
          /Users/aparup/Sites/i/lib/tests/statslib_test.php:310
          /Users/aparup/Sites/i/lib/phpunit/classes/advanced_testcase.php:76
          
          To re-run:
           phpunit statslib_daily_testcase lib/tests/statslib_test.php
          

          so i've added one commit and tested across different times and it seems fine.

          mysql and postgres looking good, i'm just testing now on mssql and oracle before integrating this.

          Show
          Aparup Banerjee added a comment - Hi Tyler, i've just come across one bug (minor) in the unit test: https://github.com/tbannister/moodle/compare/master...MDL-30643_master-log-speed#L13R40 1) statslib_daily_testcase::test_statslib_get_start_from Log entry start Failed asserting that '1272657610' matches expected 1272700810. /Users/aparup/Sites/i/lib/tests/statslib_test.php:310 /Users/aparup/Sites/i/lib/phpunit/classes/advanced_testcase.php:76 To re-run: phpunit statslib_daily_testcase lib/tests/statslib_test.php so i've added one commit and tested across different times and it seems fine. mysql and postgres looking good, i'm just testing now on mssql and oracle before integrating this.
          Hide
          Aparup Banerjee added a comment -

          oh i don't have a working phpunit on windows and seeing that MDLQA-5180 MDLQA-5181 MDLQA-5182 and MDLQA-5183 haven't been done yet atm i'm pushing this up now.

          this has been integrated into master only.

          Show
          Aparup Banerjee added a comment - oh i don't have a working phpunit on windows and seeing that MDLQA-5180 MDLQA-5181 MDLQA-5182 and MDLQA-5183 haven't been done yet atm i'm pushing this up now. this has been integrated into master only.
          Hide
          Aparup Banerjee added a comment - - edited

          ah the integration checks have sprung up missing upgrade steps for the tables.
          Table temp_enroled_template only available in first DB
          Table temp_log_template only available in first DB

          i've now added the upgrade script into integration.git (7d27055)

          Show
          Aparup Banerjee added a comment - - edited ah the integration checks have sprung up missing upgrade steps for the tables. Table temp_enroled_template only available in first DB Table temp_log_template only available in first DB i've now added the upgrade script into integration.git (7d27055)
          Hide
          Aparup Banerjee added a comment -

          I'm attaching phpunit test results for oracle and mssql (on windows) i've received from Michael here. The failures are 9 on both.

          Show
          Aparup Banerjee added a comment - I'm attaching phpunit test results for oracle and mssql (on windows) i've received from Michael here. The failures are 9 on both.
          Hide
          Aparup Banerjee added a comment - - edited

          ok,
          statslib_daily_testcase::test_statslib_get_start_from
          Log entry start
          Failed asserting that '1272592810' matches expected 1272657610.

          was thrown up by integration servers whilst it works for me here..

          also noting another :
          statslib_daily_testcase::test_statslib_temp_table_fill
          Failed asserting that 0 matches expected 1.

          /var/lib/jenkins/git_repositories/master/lib/tests/statslib_test.php:468
          /var/lib/jenkins/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76

          Show
          Aparup Banerjee added a comment - - edited ok, statslib_daily_testcase::test_statslib_get_start_from Log entry start Failed asserting that '1272592810' matches expected 1272657610. was thrown up by integration servers whilst it works for me here.. also noting another : statslib_daily_testcase::test_statslib_temp_table_fill Failed asserting that 0 matches expected 1. /var/lib/jenkins/git_repositories/master/lib/tests/statslib_test.php:468 /var/lib/jenkins/git_repositories/master/lib/phpunit/classes/advanced_testcase.php:76
          Hide
          Tyler Bannister added a comment -

          It looks like all of the stats generation tests are failing under MSSQL and Oracle, and the change I made to prevent problems with stats generation from disabling the cron (handling the exceptions) is preventing phpUnit from reporting the actual problem.

          Show
          Tyler Bannister added a comment - It looks like all of the stats generation tests are failing under MSSQL and Oracle, and the change I made to prevent problems with stats generation from disabling the cron (handling the exceptions) is preventing phpUnit from reporting the actual problem.
          Hide
          Aparup Banerjee added a comment -

          Hi Tyler,
          regarding the self::DAY timestamp, our friend is get_timezone_offset() in stats_get_base_daily().
          So i've realised we cannot use $CFG->timezone = 99 in any unit tests. i've pushed up a fix to set it to 8.

          regarding mssql/oracle turbulence, after setting $CFG->timezone = 8 i can get these 9 failures on postgres (on integration server). I'm attaching a file now which has a bit more data about the errors. from a glance they all seems to be related to time or timezones as the expected data is off by some similar amounts (eg: +-63200).

          What's strange is why i can't replicate it locally... hence timezone guess again.

          Hopefully we can resolve the mysterious 9 now

          Show
          Aparup Banerjee added a comment - Hi Tyler, regarding the self::DAY timestamp, our friend is get_timezone_offset() in stats_get_base_daily(). So i've realised we cannot use $CFG->timezone = 99 in any unit tests. i've pushed up a fix to set it to 8. regarding mssql/oracle turbulence, after setting $CFG->timezone = 8 i can get these 9 failures on postgres (on integration server). I'm attaching a file now which has a bit more data about the errors. from a glance they all seems to be related to time or timezones as the expected data is off by some similar amounts (eg: +-63200). What's strange is why i can't replicate it locally... hence timezone guess again. Hopefully we can resolve the mysterious 9 now
          Hide
          Aparup Banerjee added a comment -

          could it be that adjusting the self::DAY value has resulted in these discrepancies? if so then feel free to propose a patch against integration that changes the self::DAY back to 1272592810 together with what the $CFG->timezone should be. goodnite.

          Show
          Aparup Banerjee added a comment - could it be that adjusting the self::DAY value has resulted in these discrepancies? if so then feel free to propose a patch against integration that changes the self::DAY back to 1272592810 together with what the $CFG->timezone should be. goodnite.
          Hide
          Tyler Bannister added a comment -

          I found a few problems in the test code. It has to do with the wonky way that day starts are calculated, and a call to date that could potentially make the code not run if the configured timezone didn't match the machine time zone. It would abort the starts because the configured (system) time was too far away from current (Moodle) time.

          I think it's fixed now, I tested it with a number of different time zone settings and it always seems to work now. I've added a commit that fixes the problems to the pull branch.

          Show
          Tyler Bannister added a comment - I found a few problems in the test code. It has to do with the wonky way that day starts are calculated, and a call to date that could potentially make the code not run if the configured timezone didn't match the machine time zone. It would abort the starts because the configured (system) time was too far away from current (Moodle) time. I think it's fixed now, I tested it with a number of different time zone settings and it always seems to work now. I've added a commit that fixes the problems to the pull branch.
          Hide
          Aparup Banerjee added a comment -

          Thanks Tyler,
          I'm running these now in a few timezones as well atm. Wonky is the word! But the performance gain is what we want in this issue and the test should work to have a comparison against, so i think we're doing fine so far.

          pg and mysql are passing but i'm hearing from MdR that mssql and Oracle still aren't. i'm wondering if there are timezone specifics at play here for these DBs (i'm not aware of any)

          Show
          Aparup Banerjee added a comment - Thanks Tyler, I'm running these now in a few timezones as well atm. Wonky is the word! But the performance gain is what we want in this issue and the test should work to have a comparison against, so i think we're doing fine so far. pg and mysql are passing but i'm hearing from MdR that mssql and Oracle still aren't. i'm wondering if there are timezone specifics at play here for these DBs (i'm not aware of any)
          Hide
          Michael de Raadt added a comment -

          I've attached the PHPUnit test results for Oracle and MSSQL after the most recent changes.

          Show
          Michael de Raadt added a comment - I've attached the PHPUnit test results for Oracle and MSSQL after the most recent changes.
          Hide
          Aparup Banerjee added a comment -

          i've got a freetds installation that is thoring these up as well on which i can throw more debugging messages on hopefully soon..

          Show
          Aparup Banerjee added a comment - i've got a freetds installation that is thoring these up as well on which i can throw more debugging messages on hopefully soon..
          Hide
          Tyler Bannister added a comment - - edited

          Aparup, the easiest way to get more information is to remove the output buffering calls. The output buffering is hiding the mtrace messages that would normally be output by the cron.

          Ok, I've pushed a new update that works around the output buffering to print the mtrace output if the test fails to complete. Also, if the Moodle site has debugging enabled (at least DEBUG_ALL) the SQL errors will be in that mtrace output. Hopefully this will help us get some useful output from the MSSQL and Oracle tests.

          Show
          Tyler Bannister added a comment - - edited Aparup, the easiest way to get more information is to remove the output buffering calls. The output buffering is hiding the mtrace messages that would normally be output by the cron. Ok, I've pushed a new update that works around the output buffering to print the mtrace output if the test fails to complete. Also, if the Moodle site has debugging enabled (at least DEBUG_ALL) the SQL errors will be in that mtrace output. Hopefully this will help us get some useful output from the MSSQL and Oracle tests.
          Hide
          Aparup Banerjee added a comment -

          I've attached the output after picking the debugging switch patch (12ccfc1).

          i'm not sure what quirks are up to no good now and temp tables are proving difficult to debug with.

          Show
          Aparup Banerjee added a comment - I've attached the output after picking the debugging switch patch (12ccfc1). i'm not sure what quirks are up to no good now and temp tables are proving difficult to debug with.
          Hide
          Aparup Banerjee added a comment - - edited

          Petr, Eloy,
          could you look at phpunit_MDL-30643_debug_n.txt attached and lend some DB expertise here

          i should probably add the extract from the file which is :

          0:0 Error writing to database (Invalid column name 'courseid'.
          INSERT INTO #u_temp_stats_user_daily
          (stattype, timeend, courseid, userid, statsreads)

          SELECT 'logins', 1272758400 AS timeend, 1 AS courseid,
          userid, COUNT(id) AS statsreads
          FROM #u_temp_log1 l
          WHERE action = 'login'
          GROUP BY timeend, courseid, userid
          HAVING COUNT(id) > 0
          [array (
          )])
          ...error occurred, completed 0 days of statistics in 0 s.

          Show
          Aparup Banerjee added a comment - - edited Petr, Eloy, could you look at phpunit_ MDL-30643 _debug_n.txt attached and lend some DB expertise here i should probably add the extract from the file which is : 0:0 Error writing to database (Invalid column name 'courseid'. INSERT INTO #u_temp_stats_user_daily (stattype, timeend, courseid, userid, statsreads) SELECT 'logins', 1272758400 AS timeend, 1 AS courseid, userid, COUNT(id) AS statsreads FROM #u_temp_log1 l WHERE action = 'login' GROUP BY timeend, courseid, userid HAVING COUNT(id) > 0 [array ( )]) ...error occurred, completed 0 days of statistics in 0 s.
          Hide
          Dan Poltawski added a comment -

          Just had a quick look at this, don't have much input, apart from I noticed {{ $tempfile = $CFG->dirroot . '/lib/db/temp_stats_log_template.xml';}} is unused??

          Show
          Dan Poltawski added a comment - Just had a quick look at this, don't have much input, apart from I noticed {{ $tempfile = $CFG->dirroot . '/lib/db/temp_stats_log_template.xml';}} is unused??
          Hide
          Michael de Raadt added a comment -

          I've attached new PHPUnit output with the extra debugging output for MSSQL and Oracle.

          Show
          Michael de Raadt added a comment - I've attached new PHPUnit output with the extra debugging output for MSSQL and Oracle.
          Hide
          Aparup Banerjee added a comment -

          Thanks Micael,
          interestingly the ORA-00904 has more info up and about
          http://www.dba-oracle.com/t_ora_00904_string_invalid_identifier.htm

          Show
          Aparup Banerjee added a comment - Thanks Micael, interestingly the ORA-00904 has more info up and about http://www.dba-oracle.com/t_ora_00904_string_invalid_identifier.htm
          Hide
          Aparup Banerjee added a comment - - edited

          some further notes that we can clean up here,

          • Dan's comment
          • statslib.php stats_temp_table_create() ddlunknowntable ddl_exception uses undefined $name

          ok i've replicated the query problem with :
          INSERT INTO [moodle_gitintegrationmaster].[dbo].[mdl_stats_user_daily]
          (stattype, timeend, courseid, userid, statsreads)

          SELECT 'logins' as statype, 1272758400 AS timeend, 1 AS courseid,
          userid, COUNT(id) AS statsreads
          FROM [moodle_gitintegrationmaster].[dbo].[u_temp_log_template] tlt
          WHERE action = 'login'
          GROUP BY timened, courseid, userid
          HAVING COUNT(id) >0

          and its led me to believe that this might work (i don't know why we're grouping by a fixed number in this query s to the server so i've removed it):
          INSERT INTO [moodle_gitintegrationmaster].[dbo].[mdl_stats_user_daily]
          (stattype, timeend, courseid, userid, statsreads)

          SELECT 'logins' as statype, 1272758400 AS timeend, 1 AS courseid,
          userid, COUNT(id) AS statsreads
          FROM [moodle_gitintegrationmaster].[dbo].[u_temp_log_template] tlt
          WHERE action = 'login'
          GROUP BY userid
          HAVING COUNT(id) >0

          https://github.com/nebgor/moodle/compare/int_master...int_MDL-30643_mssql_oracle_fix

          Show
          Aparup Banerjee added a comment - - edited some further notes that we can clean up here, Dan's comment statslib.php stats_temp_table_create() ddlunknowntable ddl_exception uses undefined $name ok i've replicated the query problem with : INSERT INTO [moodle_gitintegrationmaster] . [dbo] . [mdl_stats_user_daily] (stattype, timeend, courseid, userid, statsreads) SELECT 'logins' as statype, 1272758400 AS timeend, 1 AS courseid, userid, COUNT(id) AS statsreads FROM [moodle_gitintegrationmaster] . [dbo] . [u_temp_log_template] tlt WHERE action = 'login' GROUP BY timened, courseid, userid HAVING COUNT(id) >0 and its led me to believe that this might work (i don't know why we're grouping by a fixed number in this query s to the server so i've removed it): INSERT INTO [moodle_gitintegrationmaster] . [dbo] . [mdl_stats_user_daily] (stattype, timeend, courseid, userid, statsreads) SELECT 'logins' as statype, 1272758400 AS timeend, 1 AS courseid, userid, COUNT(id) AS statsreads FROM [moodle_gitintegrationmaster] . [dbo] . [u_temp_log_template] tlt WHERE action = 'login' GROUP BY userid HAVING COUNT(id) >0 https://github.com/nebgor/moodle/compare/int_master...int_MDL-30643_mssql_oracle_fix
          Hide
          Aparup Banerjee added a comment -

          i've updated that commit to fix all other similar SQLs. please check them.

          I've also put in a separate wip commit which i'm really actualyl not commited to but with it its looking like the fails are reduced to 7 and the data is whats failing. So no SQL woes. hopefully we can get to a stage where the tests are passing Tyler

          Show
          Aparup Banerjee added a comment - i've updated that commit to fix all other similar SQLs. please check them. I've also put in a separate wip commit which i'm really actualyl not commited to but with it its looking like the fails are reduced to 7 and the data is whats failing. So no SQL woes. hopefully we can get to a stage where the tests are passing Tyler
          Hide
          Tyler Bannister added a comment -

          Aparup, I incorporated your changes into my pull branch. To fix the data issue, I changed the final table to work differently, I removed some of the hard coded outer query values and instead return the required values from the inner query and group by the returned columns. I think that should allow that query to function in MSSQL and Oracle while still returning the proper values.

          Show
          Tyler Bannister added a comment - Aparup, I incorporated your changes into my pull branch. To fix the data issue, I changed the final table to work differently, I removed some of the hard coded outer query values and instead return the required values from the inner query and group by the returned columns. I think that should allow that query to function in MSSQL and Oracle while still returning the proper values.
          Hide
          Aparup Banerjee added a comment -

          Thanks for that Tyler. I had created my changes in a separate commit for you to be able to pull it in (saving time) instead of having to create another commit

          Anyway, i've now tested this with all our latest changes and its passing for me for my mssql (freetds) setup. I'll ask Michael to run phpunit on oracle and sqlsrv once more to verify.

          Show
          Aparup Banerjee added a comment - Thanks for that Tyler. I had created my changes in a separate commit for you to be able to pull it in (saving time) instead of having to create another commit Anyway, i've now tested this with all our latest changes and its passing for me for my mssql (freetds) setup. I'll ask Michael to run phpunit on oracle and sqlsrv once more to verify.
          Hide
          Michael de Raadt added a comment -

          Test result: Success.

          This now seems to work across DBs. I ran the related unit tests and also ran cron to run the daily statistics. I can't testify to an improvement in speed, but I hope partners will be able to let us know.

          Show
          Michael de Raadt added a comment - Test result: Success. This now seems to work across DBs. I ran the related unit tests and also ran cron to run the daily statistics. I can't testify to an improvement in speed, but I hope partners will be able to let us know.
          Hide
          Martin Dougiamas added a comment -

          Congrats all!

          Show
          Martin Dougiamas added a comment - Congrats all!
          Hide
          Justin Filip added a comment -

          Woo! Great work everyone. HQ folks, thanks for the help on this one.

          Show
          Justin Filip added a comment - Woo! Great work everyone. HQ folks, thanks for the help on this one.
          Hide
          Tyler Bannister added a comment -

          Hurray! Done at last!

          Show
          Tyler Bannister added a comment - Hurray! Done at last!
          Hide
          Petr Škoda added a comment -

          congrats!

          Show
          Petr Škoda added a comment - congrats!
          Hide
          Matteo Scaramuccia added a comment -

          Kudos to all of you! Borrowing the Animated Amy from Mary Evans:

          Show
          Matteo Scaramuccia added a comment - Kudos to all of you! Borrowing the Animated Amy from Mary Evans :
          Hide
          Tyler Bannister added a comment -

          I've added pull branches and diff urls for 2.2 and 2.3. I didn't include test cases in the 2.2 branch since they won't work without the changes in 2.3.

          Show
          Tyler Bannister added a comment - I've added pull branches and diff urls for 2.2 and 2.3. I didn't include test cases in the 2.2 branch since they won't work without the changes in 2.3.
          Hide
          Tyler Bannister added a comment - - edited

          Removing the 2.2 pull branch and diff url. It looks like disposing of temporary tables doesn't work properly in 2.2. When running the cron the following errors showed up on my test site (after completing the statistics generation):

          Notice: Trying to get property of non-object in /home/tyler/projects/moodle22-plain/lib/moodlelib.php on line 6346
          
          Call Stack:
             11.2980   96343048   1. moodle_database->__destruct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:0
             11.2980   96343048   2. mysqli_native_moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:140
             11.2980   96343048   3. moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:414
             11.2980   96343048   4. moodle_temptables->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:322
             11.2981   96348256   5. database_manager->drop_temp_table() /home/tyler/projects/moodle22-plain/lib/dml/moodle_temptables.php:130
             11.3063   96451760   6. database_manager->execute_sql_arr() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:522
             11.3063   96452192   7. database_manager->execute_sql() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:75
             11.3063   96452192   8. mysqli_native_moodle_database->change_database_structure() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:88
             11.3065   96337592   9. moodle_database->query_end() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:749
             11.3066   96361720  10. ddl_change_structure_exception->__construct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:400
             11.3066   96362152  11. ddl_exception->__construct() /home/tyler/projects/moodle22-plain/lib/ddllib.php:130
             11.3066   96362424  12. moodle_exception->__construct() /home/tyler/projects/moodle22-plain/lib/ddllib.php:81
             11.3066   96362640  13. core_string_manager->string_exists() /home/tyler/projects/moodle22-plain/lib/setuplib.php:120
             11.3067   96362984  14. core_string_manager->load_component_strings() /home/tyler/projects/moodle22-plain/lib/moodlelib.php:6444
          
          
          Fatal error: Uncaught exception 'ddl_change_structure_exception' with message 'error/ddlexecuteerror' in /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php on line 400
          
          ddl_change_structure_exception: error/ddlexecuteerror in /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php on line 400
          
          Call Stack:
             11.2980   96343048   1. moodle_database->__destruct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:0
             11.2980   96343048   2. mysqli_native_moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:140
             11.2980   96343048   3. moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:414
             11.2980   96343048   4. moodle_temptables->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:322
             11.2981   96348256   5. database_manager->drop_temp_table() /home/tyler/projects/moodle22-plain/lib/dml/moodle_temptables.php:130
             11.3063   96451760   6. database_manager->execute_sql_arr() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:522
             11.3063   96452192   7. database_manager->execute_sql() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:75
             11.3063   96452192   8. mysqli_native_moodle_database->change_database_structure() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:88
             11.3065   96337592   9. moodle_database->query_end() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:749
          
          Show
          Tyler Bannister added a comment - - edited Removing the 2.2 pull branch and diff url. It looks like disposing of temporary tables doesn't work properly in 2.2. When running the cron the following errors showed up on my test site (after completing the statistics generation): Notice: Trying to get property of non-object in /home/tyler/projects/moodle22-plain/lib/moodlelib.php on line 6346 Call Stack: 11.2980 96343048 1. moodle_database->__destruct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:0 11.2980 96343048 2. mysqli_native_moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:140 11.2980 96343048 3. moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:414 11.2980 96343048 4. moodle_temptables->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:322 11.2981 96348256 5. database_manager->drop_temp_table() /home/tyler/projects/moodle22-plain/lib/dml/moodle_temptables.php:130 11.3063 96451760 6. database_manager->execute_sql_arr() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:522 11.3063 96452192 7. database_manager->execute_sql() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:75 11.3063 96452192 8. mysqli_native_moodle_database->change_database_structure() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:88 11.3065 96337592 9. moodle_database->query_end() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:749 11.3066 96361720 10. ddl_change_structure_exception->__construct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:400 11.3066 96362152 11. ddl_exception->__construct() /home/tyler/projects/moodle22-plain/lib/ddllib.php:130 11.3066 96362424 12. moodle_exception->__construct() /home/tyler/projects/moodle22-plain/lib/ddllib.php:81 11.3066 96362640 13. core_string_manager->string_exists() /home/tyler/projects/moodle22-plain/lib/setuplib.php:120 11.3067 96362984 14. core_string_manager->load_component_strings() /home/tyler/projects/moodle22-plain/lib/moodlelib.php:6444 Fatal error: Uncaught exception 'ddl_change_structure_exception' with message 'error/ddlexecuteerror' in /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php on line 400 ddl_change_structure_exception: error/ddlexecuteerror in /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php on line 400 Call Stack: 11.2980 96343048 1. moodle_database->__destruct() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:0 11.2980 96343048 2. mysqli_native_moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:140 11.2980 96343048 3. moodle_database->dispose() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:414 11.2980 96343048 4. moodle_temptables->dispose() /home/tyler/projects/moodle22-plain/lib/dml/moodle_database.php:322 11.2981 96348256 5. database_manager->drop_temp_table() /home/tyler/projects/moodle22-plain/lib/dml/moodle_temptables.php:130 11.3063 96451760 6. database_manager->execute_sql_arr() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:522 11.3063 96452192 7. database_manager->execute_sql() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:75 11.3063 96452192 8. mysqli_native_moodle_database->change_database_structure() /home/tyler/projects/moodle22-plain/lib/ddl/database_manager.php:88 11.3065 96337592 9. moodle_database->query_end() /home/tyler/projects/moodle22-plain/lib/dml/mysqli_native_moodle_database.php:749
          Hide
          Tyler Bannister added a comment -

          Ok, found and fixed the 2.2 issue. Moodle 2.3 requires drop_table be used instead of drop_temp_table. I fixed the M22 branch to continue using drop_temp_table and the errors are no longer triggered.

          Show
          Tyler Bannister added a comment - Ok, found and fixed the 2.2 issue. Moodle 2.3 requires drop_table be used instead of drop_temp_table. I fixed the M22 branch to continue using drop_temp_table and the errors are no longer triggered.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Many, many thanks for your effort!

          Millions of people will enjoy the results of your work, yay!

          Closing as fixed. Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Many, many thanks for your effort! Millions of people will enjoy the results of your work, yay! Closing as fixed. Ciao
          Hide
          Tyler Bannister added a comment -

          Does anybody know if these changes be incorporated into M22 and M23?

          Show
          Tyler Bannister added a comment - Does anybody know if these changes be incorporated into M22 and M23?
          Hide
          Michael de Raadt added a comment -

          They were not.

          Show
          Michael de Raadt added a comment - They were not.
          Hide
          Eloy Lafuente (stronk7) added a comment - - edited

          Hi Tyler,

          no, it was only integrated to master. So only it and now 24_STABLE have your awesome improvement available.

          For sure it won't be backported to 22_STABLE ever (near becoming out of support) and 23_STABLE maybe a potential target, although right now I feel myself positioned against that.

          If you think it would be worth backporting to 23_STABLE, plz, fill an issue about that to have a proper place to discuss. But, by default, it's against the integration "politics". More if there is already a stable version fixed and the previous one is not new-new (say 4 months or after .2 minor release).

          Ciao

          Show
          Eloy Lafuente (stronk7) added a comment - - edited Hi Tyler, no, it was only integrated to master. So only it and now 24_STABLE have your awesome improvement available. For sure it won't be backported to 22_STABLE ever (near becoming out of support) and 23_STABLE maybe a potential target, although right now I feel myself positioned against that. If you think it would be worth backporting to 23_STABLE, plz, fill an issue about that to have a proper place to discuss. But, by default, it's against the integration "politics". More if there is already a stable version fixed and the previous one is not new-new (say 4 months or after .2 minor release). Ciao
          Hide
          Mike Churchward added a comment -

          Hi Eloy -

          Can you explain why you are "positioned against that", and why "it's against the integration 'politics'"? The biggest problem we have for us is that many clients will remain on 2.2 for many more months, and this issue prevents them from using this feature.

          Show
          Mike Churchward added a comment - Hi Eloy - Can you explain why you are "positioned against that", and why "it's against the integration 'politics'"? The biggest problem we have for us is that many clients will remain on 2.2 for many more months, and this issue prevents them from using this feature.
          Hide
          Aparup Banerjee added a comment -

          Hi Mike,
          i've been away for awhile so i'm only just reading this and here's what i can add.

          The general policy for changes to STABLE branches is to have minimal developmental changes and majorly bug fixes. Slow queries don't necessarily fall under 'bug fixes', but if the improvements are still required, backporting can be suggested via another MDL. Backporting of improvements onto a stable branch carry more risk than development and improvement on master as the stable branch doesn't go through the entire QA process again after it has been released.

          For 22, timing has made this very difficult to do safely. As Eloy has mentioned, 23 maybe considered in a separate MDL.

          Show
          Aparup Banerjee added a comment - Hi Mike, i've been away for awhile so i'm only just reading this and here's what i can add. The general policy for changes to STABLE branches is to have minimal developmental changes and majorly bug fixes. Slow queries don't necessarily fall under 'bug fixes', but if the improvements are still required, backporting can be suggested via another MDL. Backporting of improvements onto a stable branch carry more risk than development and improvement on master as the stable branch doesn't go through the entire QA process again after it has been released. For 22, timing has made this very difficult to do safely. As Eloy has mentioned, 23 maybe considered in a separate MDL.

            People

            • Votes:
              31 Vote for this issue
              Watchers:
              29 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: