Moodle
  1. Moodle
  2. MDL-30521

Cron crash fetching external blog entries

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.1.2
    • Fix Version/s: 2.1.5, 2.2.2
    • Component/s: Administration
    • Labels:
    • Database:
      PostgreSQL
    • Testing Instructions:
      Hide

      Step 1:

      1. Install a stable 1.9 site and upgrade it to 2.1 integration
      2. In the database check post.uniquehash is 255 chars
      3. Perform step 2 on this branch
      4. Install a stable 1.9 site and Upgrade it to 2.2 integration
      5. In the database check post.uniquehash is 255 chars
      6. Perform step 2 on this branch
      7. Install a stable 2.2 site and upgrade it to integration master
      8. In the database check post.uniquehash is 255 chars
      9. Perform step 2 on this branch

      Step 2:

      1. Log in as admin
      2. Enable external blogs (Site administration -> Appearance -> Blog)
      3. Add external blog in your profile http://feeds.feedburner.com/ElBlogDeEnriqueDans (My profile settings -> Blogs -> External blogs)
      4. run cron
      5. It should not crash, keep an eye on error logs.
      6. Do the above in 21, 22, and master
      Show
      Step 1: Install a stable 1.9 site and upgrade it to 2.1 integration In the database check post.uniquehash is 255 chars Perform step 2 on this branch Install a stable 1.9 site and Upgrade it to 2.2 integration In the database check post.uniquehash is 255 chars Perform step 2 on this branch Install a stable 2.2 site and upgrade it to integration master In the database check post.uniquehash is 255 chars Perform step 2 on this branch Step 2: Log in as admin Enable external blogs (Site administration -> Appearance -> Blog) Add external blog in your profile http://feeds.feedburner.com/ElBlogDeEnriqueDans (My profile settings -> Blogs -> External blogs) run cron It should not crash, keep an eye on error logs. Do the above in 21, 22, and master
    • Workaround:
      Hide

      Change the field type from fields "uniquehash" and "subject" from "varchar(128)" to "text"

      Show
      Change the field type from fields "uniquehash" and "subject" from "varchar(128)" to "text"
    • Affected Branches:
      MOODLE_21_STABLE
    • Fixed Branches:
      MOODLE_21_STABLE, MOODLE_22_STABLE
    • Pull Master Branch:
      wip-mdl-30521
    • Rank:
      33230

      Description

      Cron crashes when fetching external blog entries (errors in Spanish):

      Fetching external blog entries...
      Default exception handler: Error al escribir a la base de datos
      Debug: ERROR: value too long for type character varying(128)
      INSERT INTO mdl_post (userid,module,content,uniquehash,publishstate,format,subject,summary,created,lastmodified) VALUES($1,$2,$3,$4,$5,$6,$7,$8,$9,$10) RETURNING id
      [array (
      'userid' => '231',
      'module' => 'blog_external',
      'content' => '4',
      'uniquehash' => 'http://feedproxy.google.com/~r/ElBlogDeEnriqueDans/~3/q2GdlIn_5Rc/la-censura-en-nombre-del-copyright-supone-una-violacion-de-los-derechos-fundamentales.html',
      'publishstate' => 'site',
      'format' => '1',
      'subject' => 'La censura en nombre del copyright supone una violación de los derechos fundamentales',
      'summary' => 'Mientras una filtración asegura que en España intentan aprovechar los “minutos de la basura” del gobierno en funciones para darle el último empujón a una ley Sinde absurda que nunca debió llegar a iniciar su tramitación, desde Europa al menos nos llegan buenas noticias: el Tribunal de Justicia de la Unión Europea (ECJ) ha determinado [...]',
      'created' => 1322120525,
      'lastmodified' => 1322120525,
      )]

      • line 397 of /lib/dml/moodle_database.php: dml_write_exception thrown
      • line 232 of /lib/dml/pgsql_native_moodle_database.php: call to moodle_database->query_end()
      • line 781 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->query_end()
      • line 833 of /lib/dml/pgsql_native_moodle_database.php: call to pgsql_native_moodle_database->insert_record_raw()
      • line 252 of /blog/lib.php: call to pgsql_native_moodle_database->insert_record()
      • line 410 of /lib/cronlib.php: call to blog_sync_external_entries()
      • line 61 of /admin/cli/cron.php: call to cron_run()

      !!! Error al escribir a la base de datos !!!

      The problem seems clear... When inserting the "uniquehash" field, the text is longer than 128 chars. In fact, we think this would happen with almost all RSS URLs, because they are generated with the title of the blog post (and the title can be very long!).

      We are considering changing the field type to "text", because this probably would resolve the problem (PostgreSQL doesn't allow to "alter column" increasing the size).

      This issue may affect the "subject" field too.

      The issue is reported as "Blocker" because, since the cron crashes, no RSS feeds are updated...

        Issue Links

          Activity

          Hide
          Jason Fowler added a comment -

          MDL-29254 is a duplicate of this

          Show
          Jason Fowler added a comment - MDL-29254 is a duplicate of this
          Hide
          Rajesh Taneja added a comment -

          Updated uniquehash precision to 256, as this should be sufficient. I haven't seen any issue reported for subject precision, hence leaving it for now and should be updated if it's a real blocker.

          In addition to this, it seems while upgrading to 1.9x somewhere summary and content fields were changed to accept null and got broken. Hence added code to make sure they can be null. NOTE: Only adding this in 21 and 22, as master requires upgrade from 22

          Show
          Rajesh Taneja added a comment - Updated uniquehash precision to 256, as this should be sufficient. I haven't seen any issue reported for subject precision, hence leaving it for now and should be updated if it's a real blocker. In addition to this, it seems while upgrading to 1.9x somewhere summary and content fields were changed to accept null and got broken. Hence added code to make sure they can be null. NOTE: Only adding this in 21 and 22, as master requires upgrade from 22
          Hide
          Jason Fowler added a comment -

          Looks fine Raj, just need to update the testing instructions

          Show
          Jason Fowler added a comment - Looks fine Raj, just need to update the testing instructions
          Hide
          Sam Hemelryk added a comment -

          Hi Raj,

          I was just having a look at this now.

          For master the changes are fine, however for the stable branches there is an issue.
          We don't allow DB changes in stable branches unless they are 100% necessary (critical or blocker). In the case of this issue it is probably OK to consider the main issue here a blocker as it is causing cron to crash, and given it is just a case of increasing the length of the unique hash field it is likely pretty safe.
          However the other changes being made on the stable branches should 100% not be there. Given that they are not there in the master branch I'm betting the problem has already been fixed there correct?

          One more thing actually, why did you choose 256? In master string fields can now go up to 1333, I wonder whether for master we look at increasing this field to the maximum to avoid further problems in the future?
          I suppose that depends a little on where and how the posts table is being used. Food for thought.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Hi Raj, I was just having a look at this now. For master the changes are fine, however for the stable branches there is an issue. We don't allow DB changes in stable branches unless they are 100% necessary (critical or blocker). In the case of this issue it is probably OK to consider the main issue here a blocker as it is causing cron to crash, and given it is just a case of increasing the length of the unique hash field it is likely pretty safe. However the other changes being made on the stable branches should 100% not be there. Given that they are not there in the master branch I'm betting the problem has already been fixed there correct? One more thing actually, why did you choose 256? In master string fields can now go up to 1333, I wonder whether for master we look at increasing this field to the maximum to avoid further problems in the future? I suppose that depends a little on where and how the posts table is being used. Food for thought. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Reopening so that things noted in previous comment can be looked at.

          Show
          Sam Hemelryk added a comment - Reopening so that things noted in previous comment can be looked at.
          Hide
          Rajesh Taneja added a comment -

          Hello Sam,

          I added extra code in stable, to make sure we don't miss summary and content update issue (mentioned in MDL-29254). Stable doesn't need this, as 2.3 require 2.2 install. It seems it got missed in some update from 1.8 to 1.9 and still an issue for few users.

          I choose 256, as it seems to be pretty reasonable, and I was not aware of 1333.

          Please suggest, what should be done for extra code in stable branches and I will update the same.

          Show
          Rajesh Taneja added a comment - Hello Sam, I added extra code in stable, to make sure we don't miss summary and content update issue (mentioned in MDL-29254 ). Stable doesn't need this, as 2.3 require 2.2 install. It seems it got missed in some update from 1.8 to 1.9 and still an issue for few users. I choose 256, as it seems to be pretty reasonable, and I was not aware of 1333. Please suggest, what should be done for extra code in stable branches and I will update the same.
          Hide
          Rajesh Taneja added a comment -

          Few standards which I found on http://www.sagerock.com/blog/title-tag-meta-description-length/

          1. Google shows 69 Characters (Including Spaces) for Page Title
          2. Yahoo shows up to 72 Characters (Including Spaces) for a Page Title. (PDF’s up to 75 characters)
          3. Bing shows 65 Characters (Including Spaces) for a Page Title Tag.

          In general, standard blog site should have max 70 chars for page title. Assuming that 256 chars should be good enough.
          On the other note, it should have been 255 or 1333 not 256 (Forgot about Null terminator).

          Will keep this open for more discussion.

          Show
          Rajesh Taneja added a comment - Few standards which I found on http://www.sagerock.com/blog/title-tag-meta-description-length/ Google shows 69 Characters (Including Spaces) for Page Title Yahoo shows up to 72 Characters (Including Spaces) for a Page Title. (PDF’s up to 75 characters) Bing shows 65 Characters (Including Spaces) for a Page Title Tag. In general, standard blog site should have max 70 chars for page title. Assuming that 256 chars should be good enough. On the other note, it should have been 255 or 1333 not 256 (Forgot about Null terminator). Will keep this open for more discussion.
          Hide
          Rajesh Taneja added a comment -

          In MDL-24875, we increased the length to 255. So, I am presuming, it's safe to keep it at 255 for now. If we encounter this issue again, then we have a debug message at /blog/lib.php#243 explaining what we have to do.

          Show
          Rajesh Taneja added a comment - In MDL-24875 , we increased the length to 255. So, I am presuming, it's safe to keep it at 255 for now. If we encounter this issue again, then we have a debug message at /blog/lib.php#243 explaining what we have to do.
          Hide
          Rajesh Taneja added a comment -

          (11:57:27) samhemelryk@moodle.org: In summary - In regards to the totally fubar'd check on lines 5751~5764 I vote to remove that entirely, check on line 5405 change to 255, and add a new upgrade block that checks if the field is already 255 and upgrades it isn't (so conditionally)

          Show
          Rajesh Taneja added a comment - (11:57:27) samhemelryk@moodle.org: In summary - In regards to the totally fubar'd check on lines 5751~5764 I vote to remove that entirely, check on line 5405 change to 255, and add a new upgrade block that checks if the field is already 255 and upgrades it isn't (so conditionally)
          Hide
          Rajesh Taneja added a comment -

          Hello Jason,
          I have modified the patch, as per Sam's suggestion.

          Can you please peer-review this again.

          Show
          Rajesh Taneja added a comment - Hello Jason, I have modified the patch, as per Sam's suggestion. Can you please peer-review this again.
          Hide
          Jason Fowler added a comment -

          Looks good to me Raj

          Show
          Jason Fowler added a comment - Looks good to me Raj
          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
          Sam Hemelryk added a comment -

          Thanks Raj this has been integrated now.

          Please in the future make sure 100% you rebase your issues that are up for integration... ESPECIALLY if they contain version bumps!

          In this case I have made an additional commit to re-bump the version.php and upgrade code. Because the version number you used was the same as the weekly release version (it was bumped as well) the merge went correctly (no conflict) and your upgrade code would not have been executed for those running on the weekly releases.

          Cheers
          Sam

          Show
          Sam Hemelryk added a comment - Thanks Raj this has been integrated now. Please in the future make sure 100% you rebase your issues that are up for integration... ESPECIALLY if they contain version bumps! In this case I have made an additional commit to re-bump the version.php and upgrade code. Because the version number you used was the same as the weekly release version (it was bumped as well) the merge went correctly (no conflict) and your upgrade code would not have been executed for those running on the weekly releases. Cheers Sam
          Hide
          Sam Hemelryk added a comment -

          Updated the testing instructions as it is IMPERATIVE the new upgrade code is tested thoroughly.

          Show
          Sam Hemelryk added a comment - Updated the testing instructions as it is IMPERATIVE the new upgrade code is tested thoroughly.
          Hide
          Rajesh Taneja added a comment -

          Sorry about that Sam,

          Didn't get chance to rebase it. Will remember it next time.

          Show
          Rajesh Taneja added a comment - Sorry about that Sam, Didn't get chance to rebase it. Will remember it next time.
          Hide
          Gerard Caulfield added a comment -

          Awesome, all worked with updated new test instructions. Test passed.

          Show
          Gerard Caulfield added a comment - Awesome, all worked with updated new test instructions. Test passed.
          Hide
          Eloy Lafuente (stronk7) added a comment -

          Some changes to Moodle should be milestones in the project by themselves.

          This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks!

          Closing as fixed, ciao

          Show
          Eloy Lafuente (stronk7) added a comment - Some changes to Moodle should be milestones in the project by themselves. This is not the case and your fix is not so important, but your collaboration is highly appreciated, thanks! Closing as fixed, ciao

            People

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

              Dates

              • Created:
                Updated:
                Resolved: