From 00f665c634d670f0f71d9f5a0c76229b8decbfb6 Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 15:07:19 +0530 Subject: [PATCH 1/2] fix: email queue cleanup in pure SQL Currently this background job constantly fails because of the way query is written: 1. It tries to find all docs to delete using select query 2. Deletes them by using `in query` with a HUGE amount of docs 3. Deletes child table with parent, again using `IN` query with huge amount of docs. This times out and never finishes on old sites. Solution: 1. Modified deletion to straightaway delete all main table rows that are older 2. Apply same deletion logic to child table rows. PS: This has potential to leave some orphan child table rows behind for few more days iff modified time was later than parent doc (this is quite rare). But it's safe since child table doesn't contain "links" anyway. --- .../doctype/email_queue/test_email_queue.py | 39 +++++++++++++++++-- frappe/email/queue.py | 24 +++++++----- 2 files changed, 50 insertions(+), 13 deletions(-) diff --git a/frappe/email/doctype/email_queue/test_email_queue.py b/frappe/email/doctype/email_queue/test_email_queue.py index b3c5467085..96c566a041 100644 --- a/frappe/email/doctype/email_queue/test_email_queue.py +++ b/frappe/email/doctype/email_queue/test_email_queue.py @@ -1,10 +1,41 @@ # -*- coding: utf-8 -*- # Copyright (c) 2015, Frappe Technologies and Contributors # License: MIT. See LICENSE -import unittest -# test_records = frappe.get_test_records('Email Queue') +import frappe +from frappe.email.queue import clear_outbox +from frappe.tests.utils import FrappeTestCase -class TestEmailQueue(unittest.TestCase): - pass +class TestEmailQueue(FrappeTestCase): + def test_email_queue_deletion_based_on_modified_date(self): + old_record = frappe.get_doc( + { + "doctype": "Email Queue", + "sender": "Test ", + "show_as_cc": "", + "message": "Test message", + "status": "Sent", + "priority": 1, + "recipients": [ + { + "recipient": "test_auth@test.com", + } + ], + } + ).insert() + + old_record.modified = "2010-01-01 00:00:01" + old_record.recipients[0].modified = old_record.modified + old_record.db_update_all() + + new_record = frappe.copy_doc(old_record) + new_record.insert() + + clear_outbox() + + self.assertFalse(frappe.db.exists("Email Queue", old_record.name)) + self.assertFalse(frappe.db.exists("Email Queue Recipient", {"parent": old_record.name})) + + self.assertTrue(frappe.db.exists("Email Queue", new_record.name)) + self.assertTrue(frappe.db.exists("Email Queue Recipient", {"parent": new_record.name})) diff --git a/frappe/email/queue.py b/frappe/email/queue.py index b92dea3e65..07731417d8 100755 --- a/frappe/email/queue.py +++ b/frappe/email/queue.py @@ -195,18 +195,24 @@ def clear_outbox(days: int = None) -> None: Note: Used separate query to avoid deadlock """ days = days or 31 - email_queue = DocType("Email Queue") + email_queue = frappe.qb.DocType("Email Queue") + email_recipient = frappe.qb.DocType("Email Queue Recipient") - email_queues = ( + # Delete queue table + ( frappe.qb.from_(email_queue) - .select(email_queue.name) - .where(email_queue.modified < (Now() - Interval(days=days))) - .run(pluck=True) - ) + .delete() + .where((email_queue.modified < (Now() - Interval(days=days)))) + ).run() - if email_queues: - frappe.db.delete("Email Queue", {"name": ("in", email_queues)}) - frappe.db.delete("Email Queue Recipient", {"parent": ("in", email_queues)}) + # delete child tables, note that this has potential to leave some orphan + # child table behind if modified time was later than parent doc (rare). + # But it's safe since child table doesn't contain links. + ( + frappe.qb.from_(email_recipient) + .delete() + .where((email_recipient.modified < (Now() - Interval(days=days)))) + ).run() def set_expiry_for_email_queue(): From 29710621d8675f9a45dddde5a0774787ebd8638a Mon Sep 17 00:00:00 2001 From: Ankush Menat Date: Tue, 24 May 2022 15:14:18 +0530 Subject: [PATCH 2/2] fix: commit after each log cleanup This preserves "progress" and releases locks held on log tables --- frappe/core/doctype/log_settings/log_settings.py | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/frappe/core/doctype/log_settings/log_settings.py b/frappe/core/doctype/log_settings/log_settings.py index 0fde168532..5632f05a36 100644 --- a/frappe/core/doctype/log_settings/log_settings.py +++ b/frappe/core/doctype/log_settings/log_settings.py @@ -10,8 +10,13 @@ from frappe.query_builder.functions import Now class LogSettings(Document): - def clear_logs(self): + def clear_logs(self, commit=False): self.clear_email_queue() + if commit: + # Since since deleting many logs can take significant amount of time, commit is required to relase locks. + # Error log table doesn't require commit - myisam + # activity logs are deleted last so background job finishes and commits. + frappe.db.commit() self.clear_error_logs() self.clear_activity_logs() @@ -34,7 +39,7 @@ class LogSettings(Document): def run_log_clean_up(): doc = frappe.get_doc("Log Settings") - doc.clear_logs() + doc.clear_logs(commit=True) @frappe.whitelist()