purgeList.php db-touch issue
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Reedy
	Sep 27 2020, 7:35 PM

Description

		$this->addOption( 'db-touch', 'Update the page.page_touched database field', false, false );

While poking at T263929, I noticed the description for db-touch isn't quite right. It only takes effect on the stdin ingress source, not from namespace or all-namespaces

So, should the documentation (ie param description) be updated to say that it only works from STDIN, or should we make db-touch work on all sources? The guarding with misermode suggests this is probably supposed to happen, but it doesn't actually happen with the code

		$conf = $this->getConfig();
		if ( ( $this->namespaceId !== null || $this->allNamespaces )
			&& $this->doDbTouch
			&& $conf->get( 'MiserMode' )
		) {
			$this->fatalError( 'Prevented mass db-invalidation (MiserMode is enabled).' );
		}

		if ( $this->allNamespaces ) {
			$this->purgeNamespace( false );
		} elseif ( $this->namespaceId !== null ) {
			$this->purgeNamespace( intval( $this->namespaceId ) );
		} else {
			$this->doPurge();
		}

And then in the doPurge loop while ( !feof( $stdin ) ) {

					if ( $this->doDbTouch ) {
						$title->invalidateCache();
					}

$this->doDbTouch isn't used in the purgeNamespace route

Details

	Subject	Repo	Branch	Lines +/-
	purgeList.php: Clarify that --db-touch is only for purging by title	mediawiki/core	master	+13 -10

Customize query in gerrit

Related Objects

Mentioned In: T263929: purgeList.php maintenance script not doing anything
Mentioned Here: rMW55439af8f7f9: maintenance: Rename purgeList.php --purge to --db-touch
T263929: purgeList.php maintenance script not doing anything

Event Timeline

Reedy created this task.Sep 27 2020, 7:35 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 27 2020, 7:35 PM

Krinkle edited projects, added MediaWiki-libs-BagOStuff; removed MediaWiki-Maintenance-system.Sep 27 2020, 8:29 PM

Krinkle moved this task from General to HTTP Cache on the MediaWiki-libs-BagOStuff board.

I believe neither the old --purge code prior to 1.35, nor the current code, supported bumping db-touch for an entire namespace. Is that right?

I wouldn't be opposed to allowing this. So long as it is opt-in, and results in an early failure on MiserMode/production.

In T263957#6496995, @Krinkle wrote:

I believe neither the old --purge code prior to 1.35, nor the current code, supported bumping db-touch for an entire namespace. Is that right?

Indeed. It only worked for the stdin type input.

So hence the task; we should either make that clear in the description for db-touch, or make it so it does do it in purgeNamespace too...

As this error handling (as was added in rMW55439af8f7f9748098975c3431ee937a37b31c12), does prevent it happening if miser mode too... but it doesn't get used in those modes anyway, so it's technically protecting/preventing nothing. But I do agree with the intention of disabling db-touch in miser mode... if those code paths were actually doing the db touching!

		if ( ( $this->namespaceId !== null || $this->allNamespaces )
			&& $this->doDbTouch
			&& $conf->get( 'MiserMode' )
		) {
			$this->fatalError( 'Prevented mass db-invalidation (MiserMode is enabled).' );
		}

Of course, it does leave the open question if db-touch should be allowed for std in in miser mode (why is that the exception?)... Considering that could be 1 or many urls to be purged

Reedy mentioned this in T263929: purgeList.php maintenance script not doing anything.Sep 28 2020, 4:32 PM

Of course, it does leave the open question if db-touch should be allowed for std in in miser mode (why is that the exception?)... Considering that could be 1 or many urls to be purged

It is currently allowed to purge CDN and DB touch for stdin, both by default and for WMF prod in miser mode. Given this is done by hand, and indeed could be a short list or a single title, seems fine to allow. But, it would be even simpler to just unconditionally disallow --db-touch in miser mode. I wouldn't oppose that. I'm not aware of any past use case for it. Afaik people use this in prod with a list of URLs to purge from the CDN.

I do agree with the intention of disabling db-touch in miser mode... if those code paths were actually doing the db touching!

Hm.. I see what you mean. I think it's still good to emit an error when setting --db-touch with a namespace/all option. But perhaps indeed we don't need the miser mode check, but can just error in general saying that we don't support it for performance reasons. My main concern here is that someone might come in and add support for it otherwise, not realizing that we can't want to allow that for performance reasons. So a mere error indicating it's a no-op would not suffice, we'd want to document also why and that it is an intentional omision (at least for MiserMode).

Krinkle edited projects, added Performance-Team; removed Performance-Team (Radar).Jan 26 2021, 4:58 PM

Krinkle triaged this task as Low priority.Feb 1 2021, 8:02 PM

Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.

Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.Feb 23 2021, 6:07 AM

Krinkle edited projects, added MediaWiki-Core-HTTP-Cache; removed MediaWiki-libs-BagOStuff.Jan 22 2022, 11:16 PM

Remaining work:

Update documentation for --db-touch to clarify that this is only used when purging by titlte, and not when purging by namespace or URL.

Out of scope, but fine for future work:

Implement support for it for namespaces, so long as it remains rejected/prevented with an error when MiserMode is enabled.

Change 802867 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/core@master] purgeList.php: Clarify that --db-touch is only for purging by title

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