Problem/Motivation

Offsite payment methods such as Amazon Pay and PayPal load payment transactions based on a remote_id field:

$transactions = commerce_payment_transaction_load_multiple(array(), array('remote_id' => $txn_id));

This results in a JOIN along this field, which in some cases can prove to be extremely problematic as the remote_id field is a varchar (text string).

Proposed resolution

Add an index to both remote_id fields in the entity property tables.

Remaining tasks

Requires an update hook and change to schema.

User interface changes

None

API changes

None

Data model changes

Add `remote_id` as an index on {commerce_payment_transaction} and {commerce_payment_transaction_revision}. Not sure if adding as a `foreign key` would also help, but might not hurt.

CommentFileSizeAuthor
#11 2924395-remote_id-index_exists.patch1.03 KBtorgosPizza
#2 2924395-commerce-payment-transaction_remote-id.patch1.4 KBtorgosPizza
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

torgosPizza created an issue. See original summary.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
Status: Active » Needs review
FileSize
2924395-commerce-payment-transaction_remote-id.patch1.4 KB

I'm not sure if it was just a concurrent request with something else that caused a lock, but this select query revealed this index issue for us. It took 160 seconds to complete, and was the result of someone coming to a PayPal return URL.

SELECT ... FROM commerce_payment_transaction base INNER JOIN commerce_payment_transaction_revision revision ON revision.revision_id = base.revision_id WHERE (base.remote_id = 'some_long_text_string')

Patch attached.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
Issue summary: View changes

Typo

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented

Great find, Erik. Do you think we'll need a warning on this to update via drush for large payment transaction tables similar to 1.14's order update? Or perhaps a disk space warning - that new index eat up a lot of space for you?

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented

That might not be a bad idea. At least a "Use Drush" warning, because the update did take several minutes to complete for us, with 1.5 million rows of transactions.

I don't see the indexes taking up too much space, considering the amount of data, but it's hard to tell as our hosting provider (Acquia) doesn't give us permission to read from our InnoDB stats database. But here's what I get from getting the table status:

commerce_payment_transaction (Total rows: 1486925)
- Data length: 1138753536
- Index length: 132399104

commerce_payment_transaction_revision (Total rows: 1487095)
- Data length: 267141120
- Index length: 19447808

Hope that helps! I'll let you know if I see any changes in behavior. Everything seems to work well, and now that we're implementing Amazon Pay thanks to Matt Glaman, I'm probably glad that we've fixed this sooner than later :)

  • rszrama committed f98cb3c on 7.x-1.x authored by torgosPizza 
    Issue #2924395 by torgosPizza: add an index to the remote_id column of...
rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Needs review » Fixed

Great, thanks for the extra info. Was just committing the patch, will try to remember to call out the update in the release notes. : D

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented

Happy to help. I'm actually somewhat surprised we had not seen this issue until now, but I suspect having two offsite payment gateways made the bottleneck a little more visible.

Thanks!

rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Fixed » Needs work

Just realized we should likely wrap these in db_index_exists() checks. Only thought of it b/c I was reviewing some other local code where I'd written an update function to make that check in case someone already added the index.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented

That's probably a good idea, just in case anyone's already thought of this.

torgosPizza’s picture

torgosPizza
he/him
Primary language English
Location Portland, OR
CreditAttribution: torgosPizza commented
Status: Needs work » Needs review
FileSize
2924395-remote_id-index_exists.patch1.03 KB

New patch attached rolled against Dev.

  • rszrama committed 7e76cb0 on 7.x-1.x authored by torgosPizza 
    Issue #2924395 by torgosPizza: check for the new indexes on transaction...
rszrama’s picture

rszrama CreditAttribution: rszrama at Centarro commented
Status: Needs review » Fixed

Groovy, thanks! Beat me to it. : D

Status: Fixed » Closed (fixed)

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