Moodle
  1. Moodle
  2. MDL-38541

incorrect rebuild_course_cache() calls in upgrade (display, show_expanded problems)

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.3.6, 2.4.3
    • Fix Version/s: 2.3.7, 2.4.4
    • Component/s: Caching, Resource
    • Labels:
    • Rank:
      48553

      Description

      While upgrading a performance test site from 2.4.1 to master (2.5) I get the following error consistently during the upgrade process.

      Debug info: ERROR: column "display" does not exist
      LINE 1: SELECT id, name, display, intro, introformat FROM mdl_folder...
      ^
      SELECT id, name, display, intro, introformat FROM mdl_folder WHERE id = $1
      [array (
      0 => '19',
      )]
      Error code: dmlreadexception
      Stack trace:
      line 426 of /lib/dml/moodle_database.php: dml_read_exception thrown
      line 248 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      line 753 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      line 1401 of /lib/dml/moodle_database.php: call to pgsql_native_moodle_database->get_records_sql()
      line 1373 of /lib/dml/moodle_database.php: call to moodle_database->get_record_sql()
      line 1352 of /lib/dml/moodle_database.php: call to moodle_database->get_record_select()
      line 429 of /mod/folder/lib.php: call to moodle_database->get_record()
      line 936 of /course/lib.php: call to folder_get_coursemodule_info()
      line 1457 of /lib/modinfolib.php: call to get_array_of_activities()
      line 1463 of /lib/db/upgrade.php: call to rebuild_course_cache()
      line 1532 of /lib/upgradelib.php: call to xmldb_main_upgrade()
      line 284 of /admin/index.php: call to upgrade_core()

        Issue Links

          Activity

          Hide
          Sam Hemelryk added a comment -

          I feel this is a real blocker presently (I'm going to hack code to get around it fro the time being)

          Having had a quick look I believe this issue comes about as a regression of two combination issues:

          1. MDL-36612 - introduces a call to rebuild_course_cache within the main lib/db/upgrade.php script
          2. MDL-37455 - which adds a display column to the folder table and then makes use of it within folder_get_coursemodule_info. This is handled within the mod/folder/lib/db/upgrade.php

          The issue being that the main upgrade script executes before the modules upgrade script has had a chance to add the new column, and as the upgrade script calls an API method things get very messy.

          Really the best solution is not to have the main upgrade script call rebuild_course_cache as that function relies on internal API's extending out to plugins.
          There is no doubt a raft of possible solutions to this issue. Perhaps the one that most appeals to me is:

          • Create a new method in upgradelib.php, upgrade_rebuild_course_caches. When a script needs to rebuild the course caches it calls that method.
          • At the end of a site upgrade we check to see if upgrade_rebuild_course_caches has been called and if so we call rebuild_course_caches.

          This way we could clear the rebuild caches in circumstances requiring it still, and not have to call it after every upgrade nor provide some secondary upgrade means.

          Many thanks
          Sam

          p.s. leaving this for Michael to triage in case there have been other reports. I've searched and not found myself.

          Show
          Sam Hemelryk added a comment - I feel this is a real blocker presently (I'm going to hack code to get around it fro the time being) Having had a quick look I believe this issue comes about as a regression of two combination issues: MDL-36612 - introduces a call to rebuild_course_cache within the main lib/db/upgrade.php script MDL-37455 - which adds a display column to the folder table and then makes use of it within folder_get_coursemodule_info. This is handled within the mod/folder/lib/db/upgrade.php The issue being that the main upgrade script executes before the modules upgrade script has had a chance to add the new column, and as the upgrade script calls an API method things get very messy. Really the best solution is not to have the main upgrade script call rebuild_course_cache as that function relies on internal API's extending out to plugins. There is no doubt a raft of possible solutions to this issue. Perhaps the one that most appeals to me is: Create a new method in upgradelib.php, upgrade_rebuild_course_caches. When a script needs to rebuild the course caches it calls that method. At the end of a site upgrade we check to see if upgrade_rebuild_course_caches has been called and if so we call rebuild_course_caches. This way we could clear the rebuild caches in circumstances requiring it still, and not have to call it after every upgrade nor provide some secondary upgrade means. Many thanks Sam p.s. leaving this for Michael to triage in case there have been other reports. I've searched and not found myself.
          Hide
          Sam Hemelryk added a comment -

          I've linked to MDL-36612 + MDL-37455 and have included the assignees as watchers here. Just in case anyone is interested

          Show
          Sam Hemelryk added a comment - I've linked to MDL-36612 + MDL-37455 and have included the assignees as watchers here. Just in case anyone is interested
          Hide
          Eloy Lafuente (stronk7) added a comment -

          I like the idea of signaling a rebuild is needed. It will avoid problems like this, and, at the same time, also will avoid double-triple execution of the same stuff (I think I found 2 rebuilds in current upgrade).

          Well, spotted, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - I like the idea of signaling a rebuild is needed. It will avoid problems like this, and, at the same time, also will avoid double-triple execution of the same stuff (I think I found 2 rebuilds in current upgrade). Well, spotted, ciao
          Hide
          Michael de Raadt added a comment -

          Marina: I've added you as assignee for this issue as it relates to your previous work. It looks like Sam is already working on a solution, though.

          Show
          Michael de Raadt added a comment - Marina: I've added you as assignee for this issue as it relates to your previous work. It looks like Sam is already working on a solution, though.
          Hide
          Marina Glancy added a comment - - edited

          I don't think we need the new method. Every time when upgrade script wants to force rebuilding course caches it just directly updates DB. And I'm pretty sure we have an agreement that we don't call core functions in upgrade script (with some exceptions)

          For all courses:

          $DB->set_field('course', 'modinfo', null);
          $DB->set_field('course', 'sectioninfo', null);
          

          For one course:

          $course = new stdClass();
          $course->id = $courseid;
          $course->sectioncache = null;
          $course->modinfo = null;
          $DB->update_record('course', $course);
          

          After this course cache will be rebuilt by itself on the first call to get_fast_modinfo()

          Show
          Marina Glancy added a comment - - edited I don't think we need the new method. Every time when upgrade script wants to force rebuilding course caches it just directly updates DB. And I'm pretty sure we have an agreement that we don't call core functions in upgrade script (with some exceptions) For all courses: $DB->set_field('course', 'modinfo', null ); $DB->set_field('course', 'sectioninfo', null ); For one course: $course = new stdClass(); $course->id = $courseid; $course->sectioncache = null ; $course->modinfo = null ; $DB->update_record('course', $course); After this course cache will be rebuilt by itself on the first call to get_fast_modinfo()
          Hide
          Sam Hemelryk added a comment -

          Hi Marina,

          Indeed we try not to call API within the upgrade script. The idea behind creating a signalling function is to allow us to remove the need to call the API within upgrade code and instead make the required call after the upgrade is complete allowing us to get around the issue of API calls within upgrades.

          Inline with that the suggestion I made for the new function comes because this is not the first time this function has been added to upgrade code, and probably will not be the last.
          Clearing the sectioncache and modinfo fields manually in the database works perfectly, hence I used that as part of the workaround. However it may lead to this code manually resetting the those fields appearing several times within upgrade code.
          In suggesting the signalling function I had in mind that this function could be called several times within upgrade code yet the resulting API call being only made once.
          It would also form part of our upgrade API, much like upgrade_set_timeout, that will hopefully be easier for people to find and understand.
          Ideally the need to call this reset function in the future will be an easier task with this small new function.
          As such my vote would still go to the signalling function. However as you are the assignee I will leave it up to you and keep my nose out of it

          Also just noting I have not started work on this issue, as I took a minute to diagnose the situation I also took a minute to understand it and the ideas I've shared here were nothing more than ideas.

          Many thanks
          Sam

          Show
          Sam Hemelryk added a comment - Hi Marina, Indeed we try not to call API within the upgrade script. The idea behind creating a signalling function is to allow us to remove the need to call the API within upgrade code and instead make the required call after the upgrade is complete allowing us to get around the issue of API calls within upgrades. Inline with that the suggestion I made for the new function comes because this is not the first time this function has been added to upgrade code, and probably will not be the last. Clearing the sectioncache and modinfo fields manually in the database works perfectly, hence I used that as part of the workaround. However it may lead to this code manually resetting the those fields appearing several times within upgrade code. In suggesting the signalling function I had in mind that this function could be called several times within upgrade code yet the resulting API call being only made once. It would also form part of our upgrade API, much like upgrade_set_timeout, that will hopefully be easier for people to find and understand. Ideally the need to call this reset function in the future will be an easier task with this small new function. As such my vote would still go to the signalling function. However as you are the assignee I will leave it up to you and keep my nose out of it Also just noting I have not started work on this issue, as I took a minute to diagnose the situation I also took a minute to understand it and the ideas I've shared here were nothing more than ideas. Many thanks Sam
          Hide
          Marina Glancy added a comment -

          Sam, one of the reasons I'm against this function is that I don't like the very idea of the fields modinfo and sectioncache. I really want to move their contents to MUC and it will be accessible only through get_fast_modinfo(). In this case all upgrade code BEFORE this change should reset the fields in DB and upgrade code AFTER this change should clear the cache. This way inserting the low-level code in upgrade script ensures that we do it correctly.

          Show
          Marina Glancy added a comment - Sam, one of the reasons I'm against this function is that I don't like the very idea of the fields modinfo and sectioncache. I really want to move their contents to MUC and it will be accessible only through get_fast_modinfo(). In this case all upgrade code BEFORE this change should reset the fields in DB and upgrade code AFTER this change should clear the cache. This way inserting the low-level code in upgrade script ensures that we do it correctly.
          Hide
          Marina Glancy added a comment -

          Just realised that I should not wait for Sam's reply and should submit this for integration ASAP. I have no doubts in my solution

          Show
          Marina Glancy added a comment - Just realised that I should not wait for Sam's reply and should submit this for integration ASAP. I have no doubts in my solution
          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
          Eloy Lafuente (stronk7) added a comment -

          grrr, and here I did exactly the same (MDL-38992) a bunch of hours later for problems with modinfo trying to use an still unavailable column (folder->show_expanded).

          My immediate solution was to, simply, change the uses of the function the their "clean-only" behavior, that has been used in upgrade code since ages ago. Knowing that it's API use, but a safe one.

          That leaded to me thinking about to add some sort of upgrade_cannot_use_this() to be used here and there, I see that you've done that (harcoding, instead of using a reusable function).

          Also, about the whole thing, I agree that, if you're planning to change the implementation of the modinfo and sectioninfo from current "database caches" to "MUC caches" the only solution is to leave upgrade code 100% clean of any call to any API and, for all current uses, issue the update manually. In the other side, you would agree that such change can be done when the "MUC caches" arrive, not before. And only in master, not in stable branches.

          Anyway, summary:

          1) I'm going to close my issue as dupe of this, grrr

          2) both 24 and master need fixing of current uses (do whatever you want: call the function in clean-only mode or change to direct DB access (in case you select the later, I'd say that 1 simple update is faster than 2 separate ones). In my issue I verified early branches (22 & 23) and all them were using the clean-only mode.

          3) decide if it's worth having a central upgrade_cannot_use_this(error|warn) (name is surely incorrect, hehe) and use it in your solution.

          4) for stables, relax a bit the the upgraderunning check, and only inform if the call is without $clearonly. We cannot change the behavior there coz it was "allowed" and there is code out there potentially doing such resets.

          5) for master, let's go the radical way and abort any call to the function. Incoming changes to implementation will render it unusable/unsafe 100%. Document the change!

          Hope this helps...ciao

          Show
          Eloy Lafuente (stronk7) added a comment - grrr, and here I did exactly the same ( MDL-38992 ) a bunch of hours later for problems with modinfo trying to use an still unavailable column (folder->show_expanded). My immediate solution was to, simply, change the uses of the function the their "clean-only" behavior, that has been used in upgrade code since ages ago. Knowing that it's API use, but a safe one. That leaded to me thinking about to add some sort of upgrade_cannot_use_this() to be used here and there, I see that you've done that (harcoding, instead of using a reusable function). Also, about the whole thing, I agree that, if you're planning to change the implementation of the modinfo and sectioninfo from current "database caches" to "MUC caches" the only solution is to leave upgrade code 100% clean of any call to any API and, for all current uses, issue the update manually. In the other side, you would agree that such change can be done when the "MUC caches" arrive, not before. And only in master, not in stable branches. Anyway, summary: 1) I'm going to close my issue as dupe of this, grrr 2) both 24 and master need fixing of current uses (do whatever you want: call the function in clean-only mode or change to direct DB access (in case you select the later, I'd say that 1 simple update is faster than 2 separate ones). In my issue I verified early branches (22 & 23) and all them were using the clean-only mode. 3) decide if it's worth having a central upgrade_cannot_use_this(error|warn) (name is surely incorrect, hehe) and use it in your solution. 4) for stables, relax a bit the the upgraderunning check, and only inform if the call is without $clearonly. We cannot change the behavior there coz it was "allowed" and there is code out there potentially doing such resets. 5) for master, let's go the radical way and abort any call to the function. Incoming changes to implementation will render it unusable/unsafe 100%. Document the change! Hope this helps...ciao
          Hide
          Eloy Lafuente (stronk7) added a comment -

          (I've amended the title of the issue to be a bit more descriptive)

          Show
          Eloy Lafuente (stronk7) added a comment - (I've amended the title of the issue to be a bit more descriptive)
          Hide
          Damyon Wiese added a comment -

          Stopping for now - the branches need updating to address points 4 and 5 of Eloys comment above.

          Show
          Damyon Wiese added a comment - Stopping for now - the branches need updating to address points 4 and 5 of Eloys comment above.
          Hide
          Damyon Wiese added a comment -

          Reopening so this issues doesn't get picked by integrator until it's updated. Please resubmit when the branches are updated.

          Thanks, Damyon

          Show
          Damyon Wiese added a comment - Reopening so this issues doesn't get picked by integrator until it's updated. Please resubmit when the branches are updated. Thanks, Damyon
          Hide
          Marina Glancy added a comment -

          For 2.4 I made almost no changes (except single DB query for clearing both fields).

          For master I created new function upgrade_ensure_not_running() in /lib/setuplib.php because it is included on every page (where /lib/upgradelib.php is not). It is called at the moment from rebuild_course_cache() and get_fast_modinfo().

          Show
          Marina Glancy added a comment - For 2.4 I made almost no changes (except single DB query for clearing both fields). For master I created new function upgrade_ensure_not_running() in /lib/setuplib.php because it is included on every page (where /lib/upgradelib.php is not). It is called at the moment from rebuild_course_cache() and get_fast_modinfo().
          Hide
          Marina Glancy added a comment -

          I also added checks for 2.3 and 2.2 (rebuild_course_cache() is not called from upgrade.php in those versions)

          Show
          Marina Glancy added a comment - I also added checks for 2.3 and 2.2 (rebuild_course_cache() is not called from upgrade.php in those versions)
          Hide
          Dan Poltawski added a comment -

          Hi Marina,

          Looks good, but on the stable branches the debugging calls should only be set to DEBUG_DEVELOPER I think.

          Dan

          Show
          Dan Poltawski added a comment - Hi Marina, Looks good, but on the stable branches the debugging calls should only be set to DEBUG_DEVELOPER I think. Dan
          Hide
          Marina Glancy added a comment -

          Hi Dan,
          sounds fair. Changed commits in 2.2, 2.3 and 2.4 to set debugging level to developer

          Show
          Marina Glancy added a comment - Hi Dan, sounds fair. Changed commits in 2.2, 2.3 and 2.4 to set debugging level to developer
          Hide
          Marina Glancy added a comment -

          no, wait, I want to re-open it.

          If we only softly warn in 2.4 but die with fatal error in 2.5 - that's not good. Imagine plugin developer that used rebuild_course_cache() some time long ago in 2.4: he will never run this code again himself and will not see the warning. He releases 2.5 version and then somebody else tries to upgrade from early 2.4 (or even 2.3) to 2.5 and this one contributed plugin causes fatal errors on upgrade.

          Before throwing an exception we should check that upgrade-from version is high enough

          Show
          Marina Glancy added a comment - no, wait, I want to re-open it. If we only softly warn in 2.4 but die with fatal error in 2.5 - that's not good. Imagine plugin developer that used rebuild_course_cache() some time long ago in 2.4: he will never run this code again himself and will not see the warning. He releases 2.5 version and then somebody else tries to upgrade from early 2.4 (or even 2.3) to 2.5 and this one contributed plugin causes fatal errors on upgrade. Before throwing an exception we should check that upgrade-from version is high enough
          Hide
          Dan Poltawski added a comment -

          Reopening

          Show
          Dan Poltawski added a comment - Reopening
          Hide
          Dan Poltawski added a comment -
          Show
          Dan Poltawski added a comment - Just thought i'd paste this link: http://docs.moodle.org/dev/Upgrade_API#Upgrade_code_restrictions
          Hide
          Dan Poltawski added a comment -

          Discussion on this topic: https://moodle.org/local/chatlogs/index.php?conversationid=12571

          So actually, this shouldn't be preventing running during upgrade at all. Else we leave plugin authors with no choice but to write breakable code. We should allow it to be called by plugins code.

          Show
          Dan Poltawski added a comment - Discussion on this topic: https://moodle.org/local/chatlogs/index.php?conversationid=12571 So actually, this shouldn't be preventing running during upgrade at all. Else we leave plugin authors with no choice but to write breakable code. We should allow it to be called by plugins code.
          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
          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
          Marina Glancy added a comment -

          2.5

          Make sure course cache is cleared properly during upgrade

          • Core upgrade MUST NOT call rebuild_course_cache() at all - update DB directly instead
          • Plugins MUST call rebuild_course_cache(.., TRUE) for clearing the course cache and can not update DB directly
          • Plugins MUST NOT call rebuild_course_cache() unless for clearing the cache
          • Created function upgrade_ensure_not_running() to be included in other functions that can not be used during upgrade

          2.4

          • Substituted calls to rebuild_course_cache() in core upgrade script with DB queries
          • Add warning and explicitly set $clearonly to true when rebuild_course_cache() is called without $clearonly argument from plugins upgrade scripts

          2.3, 2.2

          • Add warning and explicitly set $clearonly to true when rebuild_course_cache() is called without $clearonly argument from plugins upgrade scripts
          Show
          Marina Glancy added a comment - 2.5 Make sure course cache is cleared properly during upgrade Core upgrade MUST NOT call rebuild_course_cache() at all - update DB directly instead Plugins MUST call rebuild_course_cache(.., TRUE) for clearing the course cache and can not update DB directly Plugins MUST NOT call rebuild_course_cache() unless for clearing the cache Created function upgrade_ensure_not_running() to be included in other functions that can not be used during upgrade 2.4 Substituted calls to rebuild_course_cache() in core upgrade script with DB queries Add warning and explicitly set $clearonly to true when rebuild_course_cache() is called without $clearonly argument from plugins upgrade scripts 2.3 , 2.2 Add warning and explicitly set $clearonly to true when rebuild_course_cache() is called without $clearonly argument from plugins upgrade scripts
          Hide
          Dan Poltawski added a comment -

          Integrated to master, 24 and 23 (not 22 as its only supported for bugfixes).

          Looks good to me!

          Show
          Dan Poltawski added a comment - Integrated to master, 24 and 23 (not 22 as its only supported for bugfixes). Looks good to me!
          Hide
          Adrian Greeve added a comment -

          2.4 site created along with several folder resources.
          Upgraded to 2.5.
          Everything worked.
          Created a 2.2 site.
          Upgraded to 2.5
          no problems encountered.
          Test passed.

          Show
          Adrian Greeve added a comment - 2.4 site created along with several folder resources. Upgraded to 2.5. Everything worked. Created a 2.2 site. Upgraded to 2.5 no problems encountered. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Your awesome contributions are now part of Moodle, your fav LMS out there.

          Closing this as fixed.

          Many thanks for all the hard work, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Your awesome contributions are now part of Moodle, your fav LMS out there. Closing this as fixed. Many thanks for all the hard work, ciao

            People

            • Votes:
              1 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: