Seems to be a problem with the RevisionStore implementation of getRevisionByTitle. A LinkTarget gets passed in, but is not getting passed on to loadRevisionFromConds/newRevisionFromRow, even though this is supported.

Passing the Title up the call stack is probably not the Right Way To Do This, but somebody seems to have intended this use case, and doing so prevents Title::newFromID being called, stopping the extra query.

diff of debug log:

26,27d25
< [DBQuery] wiki SELECT /* Title::newFromID 10.0.2.2 */  page_namespace,page_title,page_id,page_len,page_is_redirect,page_latest,page_content_model  FROM `page`    WHERE page_id = '1'  LIMIT 1  
< [DBQuery] SELECT  page_namespace,page_title,page_id,page_len,page_is_redirect,page_latest,page_content_model  FROM `page`    WHERE page_id = '1'  LIMIT 1  
33a32
>

Seems this got lost in translation when @daniel split Revision into RevisionStore and RevisionRecord back in https://gerrit.wikimedia.org/r/c/mediawiki/core/+/374077 -- the old static methods do not accept a Title argument (perhaps do not need to), and the option was added in the new code, but not wired up.

In order to pass the object on safely, the type hinting for getRevisionByTitle will need to change from LinkTarget to Title, because newRevisionFromRow expects a Title, not just a LinkTarget, and calls methods outside the LinkTarget interface. I think the motivation for supporting LinkTarget was that the conds array could be built using its methods alone, but once it gets passed on, it actually needs to be a Title, and there is no fast branch here since all roads lead to newRevisionFromRow. Also, the method is called getRevisionByTitle.

However, I will have to comb through and see whether anybody is passing a non-Title LinkTarget to this function before finalizing a patch.

Update
Simplest/safest solution here is just to call Title::newFromLinkTarget until the LinkTarget abstraction is fully enforced -- at least it's an obvious thing to grep for later. I'm going to do this for the patch.

In T206498#4748098, @jlinehan wrote:

Simplest/safest solution here is just to call Title::newFromLinkTarget until the LinkTarget abstraction is fully enforced -- at least it's an obvious thing to grep for later. I'm going to do this for the patch.

That's probably the way to go, yes.

I'l like to avoid more type hints against Title, since Title is a terrible kitchen sink, and I hope to get rid of it in the not-too-far future. Then again, narrowing a type hint from Title to LinkTarget later should not be a problem, since all Titles are LinkTargets. Changing it to Title now could in theory break callers, but in reality, we still use Title objects nearly everywhere.

Note how this ties in with T208776: RFC: Introduce PageIdentity to be used instead of WikiPage.

In T206498#4750417, @daniel wrote:

Changing it to Title now could in theory break callers, but in reality, we still use Title objects nearly everywhere.

Indeed, from a quick inspection, it seemed the method is only being called with a Title, but I realized that was missing the point.

Title is a terrible kitchen sink, and I hope to get rid of it in the not-too-far future.

Excitement

In T206498#4750462, @jlinehan wrote:

In T206498#4750417, @daniel wrote:

Title is a terrible kitchen sink, and I hope to get rid of it in the not-too-far future.

Excitement

Indeed... for another angle on this, see T208764: Remove cyclic dependency between Title and User classes

Change 473769 had a related patch set uploaded (by Jason Linehan; owner: Jason Linehan):
[mediawiki/core@master] Fully utilize LinkTarget passed to getRevisionByTitle

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

Note this patch is currently failing because there is a Title cache hit when ApiMoveTest calls Title::newFromText after moving a page. The cached Title object will have the pre-move pageId, when in fact we want it to have the pageId of the newly-created redirect page.

Before this patch, the Title object was being re-instantiated and so this was not noticed, however now it results in an exception thanks to a sanity check in RevisionStoreRecord's constructor. Note that this is a pre-existing bug in the Title cache, which assumes that Titles are immutable when in fact they can be mutated by (at least) MovePage.

There are a few different solutions to this, none of them very palatable, so this is on hold for now until I have a few conversations.

Change 474726 had a related patch set uploaded (by Jason Linehan; owner: Jason Linehan):
[mediawiki/core@master] Calling $x->resetArticleID() now removes $x from the Title cache

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

Change 473769 had a related patch set uploaded (by Krinkle; owner: Jason Linehan):
[mediawiki/core@master] Fully utilize LinkTarget passed to getRevisionByTitle

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

Change 473769 merged by jenkins-bot:
[mediawiki/core@master] Fully utilize LinkTarget passed to getRevisionByTitle

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

In T206498#4758560, @gerritbot wrote:

Change 474726 had a related patch set uploaded (by Jason Linehan; owner: Jason Linehan):
[mediawiki/core@master] Calling $x->resetArticleID() now removes $x from the Title cache

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

@Simetrical This might be of interest to you.

Tagging for clinic duty to move this forward

@Krinkle what's the severity level of this task?

There is currently more database roundtrip latencies involved on every page view than we need. In particular, one of the very simple queries is run twice. This isn't unheard of, but this particular one is explicitly meant to be cached by the LinkCache class service in PHP. The fact that is isn't caught there, suggests there might be a bug which could be part of a larger symptom that if fixed could also improve various other issues we may not have found.

I see it as part of the long-term work to making "simple" page views do as little work as possible (which blocks things like page composition where editor page views would become comparable to those of readers). Part of that work is identifying logic that isn't just slow (but easily cacheable), but might actually obscure a hidden dependency that bypasses cache for a reason.

Given this bypasses LinkCache, that might be such a case, in which case I'd like to know more about it. Based on previous work from @daniel and @jlinehan that already started on this last year, it looks like we haven't found any reason it can't be part of the regular title query cache. But it got stuck due to some debt in the unit tests. I don't know if thats been resolved, otherwise it might just need CR.

@jlinehan your patch needs a manual rebase. Once you do that and address the comments, please move this task back to the External Review Needed column on the Clinic Duty board and we'll take another look.

Please feel free to retag Platform Engineering if additional review is needed.

@CCicalese_WMF This is waiting for Daniel or someone else to work on the patch, I can help review it if/when needed.

@Krinkle Thanks for the update. Re-tagging with Platform Engineering for prioritization/scheduling for additional development.

The open patch's comment message says Calling Title::resetArticleID() now clears the Title cache. That appears to solve a problem unrelated to the one described in this ticket. The patch ensures that when calling resetArticleID(), the title cache is reset as well as the link cache. That seems valid, but it seems like it has nothing to do with the subject of this ticket.

As to the original concern raised by this ticket, it appears that RawAction no longer triggers a second lookup in the page table. However, the Skin still does during a normal page view, via LinkBatch.

The underlying cause is that caching code around page data is a complete mess. Some issues I have found during a quick review:

• LinkBatch will write into LinkCache, but does not use information already present in the LinkCache.
• LinkCache and Title's internal TitleCache are largely redundant, but do not share information, and they tend to get out of whack.
• We cache titles by title text, not ID. Title::newFromID will not benefit from cached titles, but may introduced duplicates into the cache.

The entire system should be overhauled. I can think of a few partial fixes, but I suppose it would be better to think about it holistically, and integrate the solution into the design of PageStore (T195069).

I think the solution here will be to centralized caching in a future PageStore service, see T195069.

@daniel I see the duplication of the "main" page query may have regressed or moved around since the introduction of the PageStore service.

When I tail the debug log now in MediaWiki-Docker and load the Main Page once (to warm up any caches, and any other one-off session noise) and then do a hard refresh in the browser, the following is logged by the hard refresh:

Start request GET /wiki/Main_Page
…
[session] SessionManager using store APCUBagOStuff
[localisation] LocalisationCache using store LCStoreStaticArray
…
[DBQuery] Wikimedia\Rdbms\Database::beginIfImplied (MediaWiki\Page\PageStore::getPageByNameViaLinkCache)
…
[DBQuery] MediaWiki\Page\PageStore::getPageByNameViaLinkCache [0.005s] unknown: SELECT  page_id,page_namespace,page_title,page_is_redirect,page_is_new,page_touched,page_links_updated,page_latest,page_len,page_content_model  FROM page    WHERE page_namespace = 0 AND page_title = 'Main_Page'  LIMIT 1  
…
[ContentHandler] Registered handler for wikitext: WikitextContentHandler
…
[DBQuery] WikiPage::pageData [0s] unknown: SELECT  page_id,page_namespace,page_title,page_is_redirect,page_is_new,page_random,page_touched,page_links_updated,page_latest,page_len,page_content_model  FROM page    WHERE page_namespace = 0 AND page_title = 'Main_Page'  LIMIT 1  
…
[MessageCache] MessageCache using store APCUBagOStuff

Notice how the same page row is essentially fetched twice. It seems either both benefit from the shared LinkCache, or explicitly have one consume or get passed down data from the other.

Change 811333 had a related patch set uploaded (by TK-999; author: TK-999):

[mediawiki/core@master] MediaWiki: Avoid spurious page query

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

Change 811333 merged by jenkins-bot:

[mediawiki/core@master] MediaWiki: Avoid spurious page query

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