Page MenuHomePhabricator

Database purges should not be done for GET requests
Closed, ResolvedPublic


In ThrottleOverrideHooks::onPingLimiter, when a database lookup results in an expired record it is purged from the database. Current best practice is to avoid database modifications in response to a GET request as the GET verb is expected to be idempotent.

Event Timeline

I'd handle thr_expiry like ug_expiry in user_groups:

  • SpecialThrottleOverrideList should filter out expired rows (it might already do, haven't checked)
  • The hooks class should filter out expired rows (each row has to be checked against the current time anyways so that expired throttles do not apply).
  • SpecialOverrideThrottle should purge expired rows whenever another (no matter whether a related one) override is edited/added/removed (the latter pending to be implemented). These operations are POST-requests anyways.
  • The purging process should live in a in a job for performance reasons. DeferredUpdate would do as well, but according to the docs that's for time-critical stuff. If we just ignore expired rows, it doesn't matter whether those are removed from the db immediately or a few hours - or even a few days - later.

This will have expired overrides staying in the db for quite a while on small wikis (where SpecialOverrideThrottle isn't used regularly) though. But using "WHERE thr_expiry < $NOW" shouldn't be that hard, so this will never hit the mw layer and thus I'd say it can be neglected.

Change 392778 had a related patch set uploaded (by BryanDavis; owner: Bryan Davis):
[mediawiki/extensions/ThrottleOverride@master] Move expired record purge to a job

Change 392778 merged by jenkins-bot:
[mediawiki/extensions/ThrottleOverride@master] Move expired record purge to a job

EddieGP assigned this task to bd808.