Page MenuHomePhabricator

WikiPageConfigLoader uses master connection on GET
Closed, ResolvedPublicPRODUCTION ERROR

Description

Error
normalized_message
Expectation (masterConns <=) 0 by MediaWiki::main not met (actual: {actual}):
{query}
exception.trace
from /srv/mediawiki/php-1.37.0-wmf.11/includes/libs/rdbms/TransactionProfiler.php(444)
#0 /srv/mediawiki/php-1.37.0-wmf.11/includes/libs/rdbms/TransactionProfiler.php(224): Wikimedia\Rdbms\TransactionProfiler->reportExpectationViolated(string, string, integer)
#1 /srv/mediawiki/php-1.37.0-wmf.11/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1010): Wikimedia\Rdbms\TransactionProfiler->recordConnection(string, string, boolean)
#2 /srv/mediawiki/php-1.37.0-wmf.11/includes/libs/rdbms/loadbalancer/LoadBalancer.php(964): Wikimedia\Rdbms\LoadBalancer->getServerConnection(integer, string, integer)
#3 /srv/mediawiki/php-1.37.0-wmf.11/includes/libs/rdbms/loadbalancer/LoadBalancer.php(1103): Wikimedia\Rdbms\LoadBalancer->getConnection(integer, array, string, integer)
#4 /srv/mediawiki/php-1.37.0-wmf.11/includes/Revision/RevisionStore.php(269): Wikimedia\Rdbms\LoadBalancer->getConnectionRef(integer, array, string)
#5 /srv/mediawiki/php-1.37.0-wmf.11/includes/Revision/RevisionStore.php(258): MediaWiki\Revision\RevisionStore->getDBConnectionRef(integer)
#6 /srv/mediawiki/php-1.37.0-wmf.11/includes/Revision/RevisionStore.php(1273): MediaWiki\Revision\RevisionStore->getDBConnectionRefForQueryFlags(integer)
#7 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Config/WikiPageConfigLoader.php(157): MediaWiki\Revision\RevisionStore->getRevisionByTitle(Title, integer, integer)
#8 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Config/WikiPageConfigLoader.php(124): GrowthExperiments\Config\WikiPageConfigLoader->fetchConfig(Title, integer)
#9 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Config/WikiPageConfig.php(92): GrowthExperiments\Config\WikiPageConfigLoader->load(Title, integer)
#10 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Config/WikiPageConfig.php(155): GrowthExperiments\Config\WikiPageConfig->getConfigData(integer)
#11 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Config/GrowthExperimentsMultiConfig.php(99): GrowthExperiments\Config\WikiPageConfig->hasWithFlags(string, integer)
#12 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Specials/SpecialEditGrowthConfig.php(443): GrowthExperiments\Config\GrowthExperimentsMultiConfig->getWithFlags(string, integer)
#13 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Specials/SpecialEditGrowthConfig.php(514): GrowthExperiments\Specials\SpecialEditGrowthConfig->getValueGeConfig(string)
#14 /srv/mediawiki/php-1.37.0-wmf.11/includes/specialpage/FormSpecialPage.php(118): GrowthExperiments\Specials\SpecialEditGrowthConfig->getFormFields()
#15 /srv/mediawiki/php-1.37.0-wmf.11/includes/specialpage/FormSpecialPage.php(186): FormSpecialPage->getForm()
#16 /srv/mediawiki/php-1.37.0-wmf.11/extensions/GrowthExperiments/includes/Specials/SpecialEditGrowthConfig.php(105): FormSpecialPage->execute(NULL)
#17 /srv/mediawiki/php-1.37.0-wmf.11/includes/specialpage/SpecialPage.php(646): GrowthExperiments\Specials\SpecialEditGrowthConfig->execute(NULL)
#18 /srv/mediawiki/php-1.37.0-wmf.11/includes/specialpage/SpecialPageFactory.php(1362): SpecialPage->run(NULL)
#19 /srv/mediawiki/php-1.37.0-wmf.11/includes/MediaWiki.php(314): MediaWiki\SpecialPage\SpecialPageFactory->executePath(string, RequestContext)
#20 /srv/mediawiki/php-1.37.0-wmf.11/includes/MediaWiki.php(925): MediaWiki->performRequest()
#21 /srv/mediawiki/php-1.37.0-wmf.11/includes/MediaWiki.php(559): MediaWiki->main()
#22 /srv/mediawiki/php-1.37.0-wmf.11/index.php(53): MediaWiki->run()
#23 /srv/mediawiki/php-1.37.0-wmf.11/index.php(46): wfIndexMain()
#24 /srv/mediawiki/w/index.php(3): require(string)
#25 {main}
Impact

37 in a day.

Performance warning, no user-visible impact.

Notes

Details

Request URL
https://en.wikipedia.org/wiki/Special:EditGrowthConfig

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

SpecialEditGrowthConfig uses READ_LATEST when displaying the configuration. Either it should not do that and should use edit conflict detection on save instead, or it should suppress these warnings.

Krinkle subscribed.

This may throw a fatal error in the future. Please prioritise this regression.

The time it takes to receive a form from the server, render it, fill it out, and submit it, seems likely at least an order of magnitude longer than replication lag, hence it seems ineffective to use for this purpose. Note that ChronProtector already ensures the user's own consistency, and changes by other people seem just as likely to be in-flight as just-submitted so I would not expect it neither eliminate nor reduce conflicts :)

@Krinkle The reason why READ_LATEST is used in the special page is to bypass the memcached cache layer that's normally used in WikiPageConfigLoader. READ_LATEST was not used before, and it sometimes caused the special page to load the (outdated) information from memcached, making the user to save unintended reverts.

A primary DB read is definitely not needed in the special page. It happens as a side effect of the READ_LATEST flag being passed to RevisionLookup::getRevisionByTitle.

Using the standard IDBAccessObject-provided flags, is there any standard way to say "I want to use replica DB, but I don't want to use data from memcached"?

I'm happy to upload a patch to not use a primary DB read, I'm just not sure how to do it without re-introducing the (user-facing) bug I described above. Advice is appreciated.

The IDB flags are only for database queries, not for other layers on top. Perhaps the code to perform the fetch can be exposed directly for callers that should not be subject to caching. Thus the special page would call fetch() instead of load(). I believe that's a pattern we use in a few places already and might make sense to apply here. Alternative, a more intergrated approach would be to accept something like [ 'useCache' => false ] in a new options parameter for options specific to that service. This potentially makes the code less clean, however, and would require repeated entry/exit methods and is harder to test, document, and statically analyse.

Change 707866 had a related patch set uploaded (by Urbanecm; author: Urbanecm):

[mediawiki/extensions/GrowthExperiments@master] EditGrowthConfig: Do not use master connection on GET requests

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

Urbanecm_WMF triaged this task as High priority.

Thanks for the advice @Krinkle, I uploaded a patch. I'd appreciate your review of the fixing patch above.

@Urbanecm_WMF -- we think there is some unaddressed feedback on this patch. Could you please revisit and then we can review?

@Urbanecm_WMF -- we think there is some unaddressed feedback on this patch. Could you please revisit and then we can review?

Yes -- thank you for the reminder. I will change the patch soon.

@Urbanecm_WMF -- we think there is some unaddressed feedback on this patch. Could you please revisit and then we can review?

Yes -- thank you for the reminder. I will change the patch soon.

FYI, that just happened.

Change 707866 merged by jenkins-bot:

[mediawiki/extensions/GrowthExperiments@master] EditGrowthConfig: Do not use master connection on GET requests

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

Checked on wmf.20 - the error is not present.