When queries overheat, raise an exception · recaptime.dev/phorge@18b444e

@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge

phorge phabricator

fork

When queries overheat, raise an exception

Summary:
Ref T13259. Currently, queries set a flag and return a partial result set when they overheat. This is mostly okay:

- It's very unusual for queries to overheat if they don't have a real viewer.
- Overheating is rare in general.
- In most cases where queries can overheat, the context is a SearchEngine UI, which handles this properly.

In T13259, we hit a case where a query with an omnipotent viewer can overheat: if you have more than 1,000 consecutive commits in the database with invalid `repositoryID` values, we'll overheat and bail out. This is pretty bad, since we don't process everything.

Change this beahvior:

- Throw by default, so this stuff doesn't slip through the cracks.
- Handle the SearchEngine case explicitly ("it's okay to overheat, we'll handle it").
- Make `QueryIterator` disable overheating behavior: if we're iterating over all objects, we want to hit the whole table even if most of it is garbage.

There are some cases where this might cause new exception behavior that we don't necessarily want. For example, in Owners, each package shows "recent commits in this package". If you can't see the first 1,000 recent commits, you'd previously get a slow page with no results. Now you'll probably get an exception.

If these crop up, I think the best approach for now is to deal with them on a case-by-case basis and see how far we get. In the "Owners" case, it might be good to query by repositories you can see first, then query by commits in the package in those repositories. That should give us a better outcome than any generic behavior we could implement.

Test Plan:
- Added 100000 to all repositoryID values for commits on my local install.
- Before making changes, ran `bin/repository rebuild-identities --all --trace`. Saw the script process 1,000 rows and exit silently.
- Applied the first part ("throw by default") and ran `bin/repository rebuild-identities`. Saw the script process 1,000 rows, then raise an exception.
- Applied the second part ("disable for queryiterator") and ran the script again. Saw the script process all 15,000 rows without issues (although none are valid and none actually load).
- Viewed Diffusion, saw appropriate NUX / "overheated" UIs.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13259

Differential Revision: https://secure.phabricator.com/D20294

epriestley 7 years ago 18b444e4 8449c179

+33 -3

3 changed files

expand all

src

applications

controller

PhabricatorApplicationSearchController.php

infrastructure

query

policy

PhabricatorPolicyAwareQuery.php

storage

lisk

PhabricatorQueryIterator.php

src/applications/search/controller/PhabricatorApplicationSearchController.php

··· 249 249 $pager = $engine->newPagerForSavedQuery($saved_query); 250 250 $pager->readFromRequest($request); 251 251 252 + $query->setReturnPartialResultsOnOverheat(true); 253 + 252 254 $objects = $engine->executeQuery($query, $pager); 253 255 254 256 $force_nux = $request->getBool('nux'); ··· 798 800 $object = $query 799 801 ->setViewer(PhabricatorUser::getOmnipotentUser()) 800 802 ->setLimit(1) 803 + ->setReturnPartialResultsOnOverheat(true) 801 804 ->execute(); 802 805 if ($object) { 803 806 return null;

+28 -3

src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php

··· 45 45 */ 46 46 private $raisePolicyExceptions; 47 47 private $isOverheated; 48 + private $returnPartialResultsOnOverheat; 49 + private $disableOverheating; 48 50 49 51 50 52 /* -( Query Configuration )------------------------------------------------ */ ··· 127 129 */ 128 130 final public function requireCapabilities(array $capabilities) { 129 131 $this->capabilities = $capabilities; 132 + return $this; 133 + } 134 + 135 + final public function setReturnPartialResultsOnOverheat($bool) { 136 + $this->returnPartialResultsOnOverheat = $bool; 137 + return $this; 138 + } 139 + 140 + final public function setDisableOverheating($disable_overheating) { 141 + $this->disableOverheating = $disable_overheating; 130 142 return $this; 131 143 } 132 144 ··· 319 331 break; 320 332 } 321 333 322 - if ($overheat_limit && ($total_seen >= $overheat_limit)) { 323 - $this->isOverheated = true; 324 - break; 334 + if (!$this->disableOverheating) { 335 + if ($overheat_limit && ($total_seen >= $overheat_limit)) { 336 + $this->isOverheated = true; 337 + 338 + if (!$this->returnPartialResultsOnOverheat) { 339 + throw new Exception( 340 + pht( 341 + 'Query (of class "%s") overheated: examined more than %s '. 342 + 'raw rows without finding %s visible objects.', 343 + get_class($this), 344 + new PhutilNumber($overheat_limit), 345 + new PhutilNumber($need))); 346 + } 347 + 348 + break; 349 + } 325 350 } 326 351 } while (true); 327 352

src/infrastructure/storage/lisk/PhabricatorQueryIterator.php

··· 25 25 $pager = clone $this->pager; 26 26 $query = clone $this->query; 27 27 28 + $query->setDisableOverheating(true); 29 + 28 30 $results = $query->executeWithCursorPager($pager); 29 31 30 32 // If we got less than a full page of results, this was the last set of

Configure Feed

Configure Feed