I recently decreased the PHP memory_limit used by our web server to 128 MB. This was done to increase the scalability of our web site.

Since decreasing our memory limit, we are now receiving the following PHP error message when Elysia Cron is used to generate database backups via Backup Migrate:

[Thu Mar 01 09:15:07.409773 2018] [proxy_fcgi:error] [pid 9884:tid 140172146464512] [client 107.23.36.77:52886] AH01071: Got error 'PHP message: PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 12288 bytes) in /var/www/xxx_company/html/includes/database/database.inc on line 2227\nPHP message: PHP Fatal error: Uncaught PDOException: SQLSTATE[HY000]: General error: 2014 Cannot execute queries while other unbuffered queries are active. Consider using PDOStatement::fetchAll(). Alternatively, if your code is only ever going to run against mysql, you may enable query buffering by setting the PDO::MYSQL_ATTR_USE_BUFFERED_QUERY attribute. in /var/www/xxx_company/html/includes/database/database.inc:2227\nStack trace:\n#0 /var/www/xxx_company/html/includes/database/database.inc(2227): PDOStatement->execute(Array)\n#1 /var/www/xxx_company/html/includes/database/database.inc(697): DatabaseStatementBase->execute(Array, Array)\n#2 /var/www/xxx_company/html/includes/database/select.inc(1280): DatabaseConnection->query('SELECT ec.name ...', Array, Array)\n#3 /var/www/xxx_company/html/sites/all/modules/contrib/elysia_cron/elysia_cron.module(364): SelectQuery->execute()\n#4 /var/www/xxx_company/html/sites/all/modules/contrib/elysia_cron/elysia_cron.module(486): elysia_cron_get(':Alternate', true, 'running', 0, false)\n#...\n'

Rather than increase the PHP memory_limit used by our web site, I would like to conditionally set the memory limit used when Backup Migrate generates database backups. This will give Backup Migrate the memory it needs when database backups are generated, while preserving a low memory limit for all other PHP scripts executed by our web site.

Patch to follow.

Thank you,
Gordon

CommentFileSizeAuthor
#18 backup_migrate-n2949211-18.patch3.15 KBDamienMcKenna
#14 2949211-backup-migrate-increase-memory-limit-5.patch2.3 KBgturnbull
#13 2949211-backup-migrate-increase-memory-limit-4.patch2.29 KBgturnbull
#11 2949211-backup-migrate-increase-memory-limit-3.patch2.03 KBgturnbull
#8 2949211-backup-migrate-increase-memory-limit-2.patch2.19 KBgturnbull
#3 2949211-backup-migrate-increase-memory-limit.patch753 bytesgturnbull
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gturnbull created an issue. See original summary.

gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
Issue summary: View changes
gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
FileSize
2949211-backup-migrate-increase-memory-limit.patch753 bytes
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 3: 2949211-backup-migrate-increase-memory-limit.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Thanks for the idea and for taking the time to write the patch. While this could be a good feature for many sites, I don't like setting the default to 2gb - many VPSes would collapse under that, never mind shared web hosts.

What I'd suggest is: use a lower default, e.g. 512M or even 256M; add an extra check to see if the ini_set() function is available (it can be disabled); add some better logic to convert the strings to numbers and compare the raw numbers, otherwise if someone's site had the value set to "1G" it'd change it to 2G.

gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented

Hi Damien,
Thank you for taking the time to review my patch. I have created an updated version based on your suggestions. I hope this patch proves helpful to yourself and others.

Updated patch to follow.

Thank you,
Gordon

gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
FileSize
2949211-backup-migrate-increase-memory-limit-2.patch2.19 KB
gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
Assigned: gturnbull » Unassigned
Status: Needs work » Needs review
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Version: 7.x-3.5 » 7.x-3.x-dev
Status: Needs review » Needs work

Two quick things:

  • Drupal 8 has the Byte class for this sort of string conversion, borrowing code from it would be a reasonable idea.
  • The code itself needs to match the Drupal coding standards, take a look at the testbot results for some of the errors it caught.
gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
FileSize
2949211-backup-migrate-increase-memory-limit-3.patch2.03 KB

Hi Damien,
Thank you for your feedback. Here is an updated patch.

What do you think?

Thank you,
Gordon

gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
Status: Needs work » Needs review
gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
FileSize
2949211-backup-migrate-increase-memory-limit-4.patch2.29 KB

Updated my patch to better reflect Drupal coding standards.

Please review and let me know if this patch could be included in the next release.

Thank you,
Gordon

gturnbull’s picture

gturnbull CreditAttribution: gturnbull as a volunteer commented
FileSize
2949211-backup-migrate-increase-memory-limit-5.patch2.3 KB

I installed the Coder module and used it to identify coding standards problems previously overlooked. Updated patch attached. This should do it!

Please let me know what you think.

Thank you,
Gordon

Alex Bukach’s picture

Alex Bukach CreditAttribution: Alex Bukach commented

#14 works fine for me. Thanks @gturnbull!

3li’s picture

3li
he/him
Primary language English
Location U.K. 🇬🇧
CreditAttribution: 3li commented

Chiming in to say that #14 has also worked for me.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet at The University of British Columbia commented
Status: Needs review » Reviewed & tested by the community

Good idea thanks @gturnbull. I have a sneaky feeling I know which site that's for;)

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Parent issue: » #2942331: Plan for Backup and Migrate 7.x-3.6
FileSize
backup_migrate-n2949211-18.patch3.15 KB

Rerolled, and I added a note to the README.txt file.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: backup_migrate-n2949211-18.patch, failed testing. View results

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs work » Needs review

The tests actually pass. Not sure what's going on.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented
Status: Needs review » Fixed

Committed.

DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna at Mediacurrent commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.