Page MenuHomePhabricator

Address issues identified in the performance review
Closed, ResolvedPublic

Description

The following issues were identified in the performance review (T302858) and should be addressed:

  • updateStoredAddresses() uses a DELETE/INSERT pattern that's prone to deadlocks. One workaround is to use GET_LOCK, non-locking SELECT, and INSERT/UPDATE based on the SELECT, but it could be left as is since it will be called rarely.
  • onPageDeleteComplete and onPageMoveComplete should load data from DB_PRIMARY, not DB_REPLICA
  • Would be nice to define getGroupName() and i18n for the entries at Special:SpecialPages

Event Timeline

Change 860638 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Load data from the master DB in delete and move hook handlers

https://gerrit.wikimedia.org/r/860638

For this one:

Would be nice to define getGroupName() and i18n for the entries at Special:SpecialPages

I remember not putting them in any existing group because none of them seemed appropriate. Since we have several special pages at this point, I think we may create a group for them. What should the name of the group be? Maybe "Events"? @ifried

This is for event pages, if I understand correctly?

If yes, then I think "Events" is appropriate.

This is for event pages, if I understand correctly?

It's for all the special pages defined by the CampaignEvents extension. Currently, that is: Special:EnableEventRegistration, Special:EditEventRegistration, Special:DeleteEventRegistration, Special:RegisterForEvent, Special:CancelEventRegistration, Special:MyEvents, and Special:EventDetails.

Thanks for clarifying.

Since we will have pages defined by the extension that go beyond registration specifically, I think "Events" makes sense. Thanks!

Change 860638 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Load data from the master DB in delete and move hook handlers

https://gerrit.wikimedia.org/r/860638

Change 874846 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/extensions/CampaignEvents@master] Create group for special pages defined by the extension

https://gerrit.wikimedia.org/r/874846

I read this one more carefully:

updateStoredAddresses() uses a DELETE/INSERT pattern that's prone to deadlocks. One workaround is to use GET_LOCK, non-locking SELECT, and INSERT/UPDATE based on the SELECT, but it could be left as is since it will be called rarely.

But I'm not sure how to apply it. Specifically, we need to delete the relevant rows when an address is removed from an event, so we would still be performing a DELETE at some point. I assume one way to change that would be to do a SELECT, then INSERT/DELETE as needed. Basically, what's written in the suggestion above, modulo replacing "UPDATE" with "DELETE". For clarity, the current approach is:

  • Unconditional delete rows for addresses that are no longer associated with the event: DELETE ce_event_address FROM ce_event_address, ce_address WHERE ceea_address=cea_id AND cea_full_address NOT IN ( ... )
    • Note that the above is for MySQL; otherwise, the query is DELETE FROM ce_event_address WHERE ceea_address IN ( SELECT cea_id FROM ce_address WHERE cea_full_address NOT IN ( ... ) )
  • If we have at least one address, then: INSERT IGNORE INTO ce_event_address ...

Whereas the new approach would be:

  • GET_LOCK
  • SELECT ... FROM ce_event_address JOIN ce_address WHERE ...
  • Collect the rows whose addresses were removed from the event, then DELETE FROM ce_event_address WHERE ceea_address IN ( /* IDs obtained from the SELECT */ )
  • Check if any of the new addresses is not in the DB, and if so, INSERT them
    • Or we could also leave the current INSERT IGNORE

So they would do roughly the same thing, except the SELECT and DELETE are two separate queries. @aaron Is this what you meant in T302858#8354617?

I'd suggesting leaving it alone, since the rate that code path is hit won't be high.

It's just something to be careful about in general. Unconditional DELETE, when there are no rows, the "gap locking" in mysql can cause contention that you would not expect.

I'd suggesting leaving it alone, since the rate that code path is hit won't be high.

It's just something to be careful about in general. Unconditional DELETE, when there are no rows, the "gap locking" in mysql can cause contention that you would not expect.

Got it, thank you! I'm not changing it then.

Change 874846 merged by jenkins-bot:

[mediawiki/extensions/CampaignEvents@master] Create group for special pages defined by the extension

https://gerrit.wikimedia.org/r/874846

The only thing to test here is that all special pages defined by the extension are in a dedicated "Events" section of Special:SpecialPages.

The only thing to test here is that all special pages defined by the extension are in a dedicated "Events" section of Special:SpecialPages.

All special pages defined by the extension are in a dedicated "Events" section of Special:SpecialPages, but EnableEventRegistration is not included there on betacluster. It is included on local though. I am not sure if this is in scope of this ticket though @Daimona ?

English betacluster:

Screenshot 2023-01-20 at 1.42.56 PM.png (260×2 px, 50 KB)

Local:

Screenshot 2023-01-20 at 1.43.22 PM.png (266×1 px, 56 KB)

All special pages defined by the extension are in a dedicated "Events" section of Special:SpecialPages, but EnableEventRegistration is not included there on betacluster. It is included on local though. I am not sure if this is in scope of this ticket though @Daimona ?

That's because Special:EnableEventRegistration can only be accessed by those who can actually enable event registrations. That's why it appears in bold in your local (see legend at the bottom of Special:SpecialPages). Were you visiting beta while logged out?

All special pages defined by the extension are in a dedicated "Events" section of Special:SpecialPages, but EnableEventRegistration is not included there on betacluster. It is included on local though. I am not sure if this is in scope of this ticket though @Daimona ?

That's because Special:EnableEventRegistration can only be accessed by those who can actually enable event registrations. That's why it appears in bold in your local (see legend at the bottom of Special:SpecialPages). Were you visiting beta while logged out?

Ah yes, you are correct. I am marking this as done/resolved now.