impress-org / givewp

GiveWP - The #1 Donation Plugin for WordPress. Easily accept donations and fundraise using your WordPress website.
https://givewp.com/
GNU General Public License v3.0
340 stars 191 forks source link

Buffer Size Error #6467

Closed canny[bot] closed 2 years ago

canny[bot] commented 2 years ago

We have a handful of reports of users receiving the following error: Out of sort memory, consider increasing server sort buffer size. This error is stopping the acceptance of donations.

The current workaround is to have users asking their hosting providers to increase buffer size to 2M.

https://givewp.canny.io/admin/board/bug-reports/p/buffer-size-error

Visuals

SELECT id,
       user_id AS userId,
       email,
       name,
       purchase_value AS totalAmountDonated,
       purchase_count AS totalDonations,
       payment_ids AS paymentIds,
       date_created AS createdAt,
       token,
       verify_key AS verifyKey,
       verify_throttle AS verifyThrottle,
       give_donormeta_attach_meta_0.meta_value AS firstName,
       give_donormeta_attach_meta_1.meta_value AS lastName,
       CONCAT('[', GROUP_CONCAT(DISTINCT CONCAT('"', give_donormeta_attach_meta_2.meta_value, '"')),']') AS additionalEmails,
       give_donormeta_attach_meta_3.meta_value AS prefix
FROM local_give_donors
         LEFT JOIN local_give_donormeta give_donormeta_attach_meta_0
             ON ID = give_donormeta_attach_meta_0.donor_id
                    AND give_donormeta_attach_meta_0.meta_key = '_give_donor_first_name'
         LEFT JOIN local_give_donormeta give_donormeta_attach_meta_1
             ON ID = give_donormeta_attach_meta_1.donor_id
                    AND give_donormeta_attach_meta_1.meta_key = '_give_donor_last_name'
         LEFT JOIN local_give_donormeta give_donormeta_attach_meta_2
             ON ID = give_donormeta_attach_meta_2.donor_id
                    AND give_donormeta_attach_meta_2.meta_key = 'additional_email'
         LEFT JOIN local_give_donormeta give_donormeta_attach_meta_3
             ON ID = give_donormeta_attach_meta_3.donor_id
                    AND give_donormeta_attach_meta_3.meta_key = '_give_donor_title_prefix'
GROUP BY ID, firstName, lastName, prefix

Additional Context

Stack trace:
#0 /dom36425/wp-content/plugins/give/src/Framework/Database/DB.php(90): Give\Framework\Database\DB::runQueryWithErrorChecking(Object(Closure))
#1 /dom36425/wp-content/plugins/give/src/Framework/Models/ModelQueryBuilder.php(43): Give\Framework\Database\DB::__callStatic('get_row', Array)
#2 /dom36425/wp-content/plugins/give/src/Donors/Repositories/DonorRepository.php(316): Give\Framework\Models\ModelQueryBuilder->get()
#3 /dom36425/wp-content/plugins/give/src/Donors/Repositories/DonorRepositoryProxy.php(67): Give\Donors\Repositories\DonorRepository->getByEmail('[emailredacted@example.org](mailto:emailredacted@example.org)')
#4 /dom36425/wp-content/plugins/give/src/Donors/Models/Donor.php(80): Give\Donors\Repositories\DonorRepositoryProxy->__call('getByEmail', Array)
#5 /dom36425/wp-content/plugins/give/src/LegacyPaymentGateways/Adapters/LegacyPaymentGatewayAdapter.php(157): Give\Donors\Models in /dom36425/wp-content/plugins/give/src/Framework/Database/DB.php on line 242
canny[bot] commented 2 years ago

This issue has been linked to a Canny post: Buffer Size Error :tada:

kjohnson commented 2 years ago

It looks like the query is generated by prepareQuery() via getByEmail(), which does not actually use the "additional emails" column (it may be used elsewhere).

Interestingly, if the email doesn't match a primary email address for the donor there is a subsequent query that looks at the additional_email meta, see getByAdditionalEmail() using a direct query of the give_donormeta table.

kjohnson commented 2 years ago

The method prepareQuery() is only called in four locations:

    public function queryById(int $donorId): ModelQueryBuilder
    {
        return $this->prepareQuery()
            ->where('id', $donorId);
    }
    public function getByWpUserId(int $userId)
    {
        ...
        return $this->prepareQuery()
            ->where('user_id', $userId)
            ->get();
    }
    public function getByEmail(string $email)
    {
        $donorObjectByPrimaryEmail = $this->prepareQuery()
            ->where('email', $email)
            ->get();

       ...

        return $donorObjectByPrimaryEmail;
    }
    public static function query(): ModelQueryBuilder
    {
        return give()->donors->prepareQuery();
    }

The last of which, Donor::query(), being the most concerning because it is intended to be extended for bespoke queries.

kjohnson commented 2 years ago

Using PHPStorm I'm not actually seeing any direct use of Donor::query().

kjohnson commented 2 years ago

That said, I think it may be safe to remove the additional_emails column SELECT from the prepared query, as it isn't used anywhere. While this would technically be a breaking change, being that we have not publicized the new Models will documentation the risk is low.

jonwaldstein commented 2 years ago

@kjohnson the additional_email column is being used directly by the Donor instance model to check if email exists and associated crud functionality. I think the root of the problem is how we're grabbing that column (this would be the same for any columns that have the same key), not necessarily that we're including it in the query.

Here is the PR that introduced this: https://github.com/impress-org/givewp/pull/6273

I think if we can optimize this query, we won't need to worry about same key queries in the future.

kjohnson commented 2 years ago

I see now where the SELECT column is being added to the Donor model, which is used.

kjohnson commented 2 years ago

I'm leaning towards not pre-loading meta with multiple values (rows) and having those fetched separately. This is actually how getByAdditionalEmail() works, interestingly.

jonwaldstein commented 2 years ago

Fixed by https://github.com/impress-org/givewp/pull/6469