Thanks for filing this, I was just looking to see if there was a task :)

One thing I don't understand is why the error is silent, I'd expect more fireworks for a failure like this.

From what I can see in the logs, the query is an INSERT OR IGNORE query. That's why it fails silently.

In T333389#8735920, @Scardenasmolinar wrote:

From what I can see in the logs, the query is an INSERT OR IGNORE query. That's why it fails silently.

Oh right. That is 100% my fault.

public function insert( QueueRecord $queueRecord ): Status {
		$status = new Status();
		$queueRecordData = $queueRecord->jsonSerialize();
		$this->dbw->insert(
			'pagetriage_page',
			$queueRecordData,
			__METHOD__,
			// 'IGNORE' is needed for fields like ptrp_tags_updated which will not be
			// present on a new record.
			[ 'IGNORE' ]
		);
		$status->setOK( $this->dbw->affectedRows() === 1 );
		return $status;
	}

added in rEPTRf30ba5abb4f7: QueueManager: Add insert method and use it from PageTriage class

// 'IGNORE' is needed for fields like ptrp_tags_updated which will not be
// present on a new record.

I hadn't seen this comment. ptrp_tags_updated is the field that is supposedly NOT NULL and the reason why the insertion is failing. That's why it's failing. Should we change the SQLite schema to reflect it can be NULL?

I have a few questions on how to proceed with this ticket:

1. Should we change the database schema and make ptrp_tags_updated to a nullable field?

OR

2. Should we add a now() timestamp to ptrp_tags_updated and then PageTriage::bulkSetTagsUpdated corrects the timestamp. I don't like this solution because prtp_tags_updated will have incorrect data until PageTriage::bulkSetTagsUpdated runs. It looks like it is only called when a user's block status is updated (PageTriageUtil::updateMetadataOnBlockChange).

Additional questions:

• Is PageTriage silently breaking in production because of this error?
• Why is MySQL not breaking and can add pages to Special:NewPagesFeed if it also has this field constraint?
• Should we call PageTriage::bulkSetTagsUpdated on other parts of the code?

Change 904582 had a related patch set uploaded (by Jsn.sherman; author: Jsn.sherman):

[mediawiki/extensions/PageTriage@master] WIP: make pagetriage_page.ptrp_reviewd_updated nullable

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

While looking at this, I initially was not seeing new pages going into the queue automatically even with mysql, but I seem to have that working today, so that makes me think that things would be fine in production.

Looking through the code and the schema, it looks like prtp_tags_updated didn't get a schema update when it became nullable.

I went ahead and updated the schema from my mariadb-backed docker environment:
https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageTriage/+/904582
@Scardenasmolinar could you try this patch in the sqllite-based environment?

note that you'll need to run the update script to do the migration

I commented on the Gerrit patch. It seems like you modified ptrp_reviewed_updated instead of prtp_tags_updated.

I got a little turned around with the tools for the abstract schema changes, but believe I have it sorted now. The sqlite patch handles the change by creating a temporary table.

It's working now! I missed the SQLite schema change file in my initial review.

@jcrespo we think we're looking at a schema change for the PageTriage extension. Looks like it's enabled on testwiki, testwiki2, and enwiki. Per https://wikitech.wikimedia.org/wiki/Schema_changes I thought it looked like the right time to tag a DBA. Does this look reasonable to you?

Hi, sorry for the confusion (maybe there is an old reference to my account somewhere)? or maybe or you just looked at an older task. The right person to tag for help with a schema changes is at the moment my workmate @Ladsgroup , as he is the person that will know the high level of the mw db status, review and deploy changes, and he will make sure the right person helps you move forward/support you on your database needs. :-)

I can help if needed, but he should be for now the right person to contact first, as that help us coordinate better! (using the DBA tag is how you can get our attention, which I now just added)

Hi, yeah, you need a separate ticket for this. For example: T333332. Most importantly and before doing so, you need to make a patch inside the extension to reflect the new schema (add me to review the patch). See https://www.mediawiki.org/wiki/Manual:Schema_changes for that. See patches connected to T188180 as examples.

Thank you both! I wasn't quite sure which step of the process to place us in since the application code treating this field as nullable is already in production and this schema change would be catching the db up with that. So I was treating this as step 2.

Make sure to involve as many relevant people as possible before merging. [...] Please include at least a DBA also, even for changes that may look trivial, as they can provide constructive feedback if warned in time. Sometimes simple schema changes require a lot of preparation for deployment.

I'll get that new ticket started. I do have a patch for the schema change in page triage, which I'll add you to!

Change 904582 merged by jenkins-bot:

[mediawiki/extensions/PageTriage@master] make pagetriage_page.ptrp_tags_updated nullable

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

after this migration happens, we should be able to stop relying on insert or ignore behavior in the queue manager insert function.