Same issue as T300924... As I said back then, this code hasn't changed in years, and there's nothing that stands out as obviously wrong with it. CC'ing @Ladsgroup in case he may have some ideas, but I really don't know why this is happening.

This time I spent some time looking at the code and while I can't exactly say what is the reason behind this but I'm fairly certain it has to do with the way the code inserts new filters. It currently sets af_id in INSERT. INSERT INTO abuse_filter` (af_id,af_pattern,af_public_comments`. This is a bad practice and can lead to lots of issues in HA setups and you are basically at the mercy of your database being 100% consistent all the time. You should avoid making any change to af_id of any rows (the code does that: $dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );) and so on. The insert should avoid setting af_id and simply should read the value later and leave it to the db to set it properly.

We wanted to do this kind of gymnastics in url shorterner to get "custom short urls" but DBAs told us it can break in so many different ways.

General rule: Don't change or set columns that their values is auto_increment. Only read them.

Do we have any workaround until the code is fixed? On the other bug it fixed itself, but rowiki has almost 100 filters.

In T306692#7873834, @Ladsgroup wrote:

This time I spent some time looking at the code and while I can't exactly say what is the reason behind this but I'm fairly certain it has to do with the way the code inserts new filters. It currently sets af_id in INSERT. INSERT INTO abuse_filter` (af_id,af_pattern,af_public_comments`. This is a bad practice and can lead to lots of issues in HA setups and you are basically at the mercy of your database being 100% consistent all the time. You should avoid making any change to af_id of any rows (the code does that: $dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );) and so on. The insert should avoid setting af_id and simply should read the value later and leave it to the db to set it properly.

Good point. That code is responsible for both creating new filters and updating existing ones, hence the use of IDatabase::replace() with rows containing af_id (which is NULL for new filters). I wonder if IDatabase::upsert would be better suited for this use case (in fact, I don't quite get how to choose between the two).

In T306692#7874085, @Daimona wrote:

In T306692#7873834, @Ladsgroup wrote:

This time I spent some time looking at the code and while I can't exactly say what is the reason behind this but I'm fairly certain it has to do with the way the code inserts new filters. It currently sets af_id in INSERT. INSERT INTO abuse_filter` (af_id,af_pattern,af_public_comments`. This is a bad practice and can lead to lots of issues in HA setups and you are basically at the mercy of your database being 100% consistent all the time. You should avoid making any change to af_id of any rows (the code does that: $dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );) and so on. The insert should avoid setting af_id and simply should read the value later and leave it to the db to set it properly.

Good point. That code is responsible for both creating new filters and updating existing ones, hence the use of IDatabase::replace() with rows containing af_id (which is NULL for new filters). I wonder if IDatabase::upsert would be better suited for this use case (in fact, I don't quite get how to choose between the two).

replace doesn't insert when it's a new row (AFAIK and simply don't pass it). Upsert should insert but I suggest avoiding that as MySQL due to MVCC wastes an auto_increment value on update. I think mw core tires to avoid that but still, first do a select if it exists do an update, if not, do an upsert (to avoid issues in race conditions).

In T306692#7873847, @Strainu wrote:

Do we have any workaround until the code is fixed? On the other bug it fixed itself, but rowiki has almost 100 filters.

I can try to look at fixing the auto increment value for now. I was busy with another db issue.

In T306692#7874090, @Ladsgroup wrote:

In T306692#7874085, @Daimona wrote:

In T306692#7873834, @Ladsgroup wrote:

This time I spent some time looking at the code and while I can't exactly say what is the reason behind this but I'm fairly certain it has to do with the way the code inserts new filters. It currently sets af_id in INSERT. INSERT INTO abuse_filter` (af_id,af_pattern,af_public_comments`. This is a bad practice and can lead to lots of issues in HA setups and you are basically at the mercy of your database being 100% consistent all the time. You should avoid making any change to af_id of any rows (the code does that: $dbw->replace( 'abuse_filter', 'af_id', $newRow, __METHOD__ );) and so on. The insert should avoid setting af_id and simply should read the value later and leave it to the db to set it properly.

Good point. That code is responsible for both creating new filters and updating existing ones, hence the use of IDatabase::replace() with rows containing af_id (which is NULL for new filters). I wonder if IDatabase::upsert would be better suited for this use case (in fact, I don't quite get how to choose between the two).

replace doesn't insert when it's a new row

I think I didn't get this. AIUI, replace does a delete first, and then an insert. If the filter is new (af_id = NULL), no row is deleted and it just inserts a new row, letting the auto increment choose the ID. I thought it was good for this use case.

More generally, I'm a bit puzzled: we have two methods that seem to be fine for the use case where you want to either update an existing row or insert a new one (with auto increment). It's a bit weird that neither of them can be used instead. I wonder if this should be fixed in MW core then.

From looking at the docs, replace doesn't accept af_id with NULL. It even says Columns belonging to a key in $uniqueKeys must be defined here and non-null.

General note: Half of rdbms is overly complex with speculative benefits and needs to be removed. Probably this function would be one of them but doing it will take time. I'm working on it.

I reset the auto increment value to 98 so it should be working now for rowiki but this code path needs fixing.

Confirmed working for rowiki, but I do suggest leaving the bug open until it's properly fixed.

Yeah. I can spend some time on it next week.

Change 785971 had a related patch set uploaded (by Ladsgroup; author: Amir Sarabadani):

[mediawiki/extensions/AbuseFilter@master] FilterStore: Use upsert instead of replace

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

Change 785971 merged by jenkins-bot:

[mediawiki/extensions/AbuseFilter@master] FilterStore: Use upsert instead of replace

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