Add ORDER BY Clause to CRM_Mailing_BAO_MailingRecipients->mailingQuery()

Overview

The SQL query in CRM_Mailing_BAO_MailingRecipients->mailingQuery() helps to segment all recipients of a mailing into smaller Mail Jobs / Batches. The query is iteratively called with a LIMIT and changing OFFSET with the goal of returning the next batch of recipients.

The problem is that without an ORDER BY clause, there is no guarantee that the order will be consistent across multiple queries.

I believe that this could potentially cause recipients to be duplicated or excluded from mailings if the order of results changes between batches.

My suggested solution is to add ORDER BY contact_id ASC, email_id ASC to the query. (Though, it might suffice to just order by email_id)

Reproduction steps

1. Create a Mailing.
2. Process the mailing with the process_mailing scheduled task.
3. Compare recipients in civicrm_mailing_recipients to recipients in related mail jobs in civicrm_mailing_event_queue.

Note that this problem is likely intermittent and there's no guarantee of reproducing it on any single run. Likelihood of error might also differ across mysql installations (MariaDB vs Percona, etc)

Environment information

• CiviCRM: Master/5.75
• PHP: 8.3
• CMS: Drupal 7.99
• Database: Percona for MySQL 8.0.41-32
• Web Server: Apache/2.4.63

changed the description

Good catch, @se_nate I think you're right. Databases don't make any promises re the order of records unless you specify one - though typically they're fairly predictable, complex queries may come out with different executions based on data, keys etc. so there will be cases when this hits.

Ordering by a primary key should be fairly efficient. Contact ID should do: a mailing should not go to the same contact twice, so that should be unique. Someone tell me if I'm wrong.

Email ID should also work, I think, but it's slightly harder to reason about. Civi can be set to send two emails to the same email address for different contacts, but even in that sitch there would be two unique email IDs for the same email address.

Are you ok to make a pull/merge request for this?

@artfulrobot Yes. I'm happy to create a PR. But, my plan is to, first, do a bit more testing on my end. I want to at least try to produce the problem. We are, in fact, having issues with duplicate recipients in our installation. That's what got me to look at it. So, if I can recreate the problem and prove the fix, then I'll feel better about things on my end.

added sig:bug triaged labels

added comp:CiviMail label

@se_nate

Thanks, we are also facing similar issue where in some contacts are randomly excluded from the mailing. The contact record exits in the mailing recipient table however, they are missing in mailing event queue. It is hard to replicate. Your proposed fix sounds logical.

I agree with @artfulrobot it would be good to order it by Contact ID and it should not have performance issues.

@kurund Thanks for corroborating. Are you by chance also seeing duplicated recipients in the mailing event queue for a given mailing?

@se_nate

I am not certain about duplicates. May be we had some in the past.

@se_nate @artfulrobot

I did a quick test on retrieving data from civicrm_mailing_recipients for our data set.

SELECT count(id) FROM civicrm_mailing_recipients;

Total records: 1465748

SELECT SQL_NO_CACHE * FROM civicrm_mailing_recipients WHERE mailing_id=X order by contact_id ASC LIMIT  0, 500;

Query took 0.0053 seconds.

SELECT SQL_NO_CACHE * FROM civicrm_mailing_recipients WHERE mailing_id=X order by id ASC LIMIT  0, 500;

Query took 0.0013 seconds.

SELECT SQL_NO_CACHE * FROM civicrm_mailing_recipients WHERE mailing_id=X LIMIT  0, 500;

Query took 0.0014 seconds.

This makes me think if we should just order by civicrm_mailing_recipients.id column?

(Updated time by restarting db server too)

@kurund I agree.

A pull request related to this issue has been opened with the title: #5812 Fixes missing and duplicate recipients from mailings

I have tested with this PR https://github.com/civicrm/civicrm-core/pull/32516 and is working as before.

It's hard to replicate the issue. I can confirm here after it's on prod and if we still get the issue again.