Page MenuHomePhabricator
Create Task
Maniphest T42757

Joins on INTEGER and TEXT fail with PostgreSQL
Closed, ResolvedPublic
Actions

Description

SpecialAbuseLog does three "LEFT JOIN"s on "af_id=afl_filter" with af_id being INTEGER and afl_filter being TEXT. These fail on PostgreSQL.

MySQL is very forgiving on such joins and for example matches 4711 with "4711 mysql sucks", but PostgreSQL does not. For AbuseFilter, afl_filter seems to be either a reference to af_id or a reference to af_id with "global-" prepended, so there shouldn't be a need for complete imitation. There is no explanation for this data model which must make JOINs even on MySQL very expensive and the needed storage unnecessarily huge.

Short term solution would be something like "wfGetDB( DB_MASTER )->getType() == 'mysql' ? 'af_id=afl_filter' : 'af_id::TEXT=afl_filter'" (not the other way round as that fails on non-convertable strings), proper way would be of course to split afl_filter into afl_filter_id and afl_filter_global or similar and let the database do what it's best at.

Details

Reference
bz40757
SubjectRepoBranchLines +/-
Use string cast for Postgres compatibilitymediawiki/extensions/AbuseFiltermaster+6 -5
Read afl_filter_id and afl_global instead of afl_filtermediawiki/extensions/AbuseFiltermaster+88 -48
Start writing to afl_filter_id and afl_globalmediawiki/extensions/AbuseFiltermaster+5 -0
Customize query in gerrit

Related Objects
Search...

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
· · ·
InvalidNoneT2384 [REPLACED BY TAG] PostgreSQL/pgsql support (tracking)
ResolvedDaimonaT199544 Make AbuseFilter work on PostgreSQL and SQLite (epic)
ResolvedDaimonaT42757 Joins on INTEGER and TEXT fail with PostgreSQL
· · ·

Event Timeline

• bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:59 AM
• bzimport added a project: AbuseFilter.
• bzimport set Reference to bz40757.
• bzimport added a subscriber: Unknown Object (MLST).
scfc created this task.Oct 4 2012, 2:32 AM
Qgil added a comment.Apr 15 2014, 6:42 AM

High + Blocker, but no progress since it was reported, a year and half ago?

Is anybody currently planning to work on this?

Aklapper added a comment.Jun 30 2014, 1:43 PM

Lowering priority - PostgreSQL support is maintained by volunteers and patches are welcome...

• werdna unsubscribed.Dec 10 2014, 5:24 PM
Umherirrender subscribed.Sep 11 2016, 9:11 AM

There is a new Database::buildStringCast which can be used here - https://gerrit.wikimedia.org/r/#/c/296066/

Qgil unsubscribed.Sep 12 2016, 9:06 AM
Jdforrester-WMF added a project: PostgreSQL.Nov 2 2016, 8:16 PM
Jdforrester-WMF subscribed.

Migrating from the old tracking task to a tag for PostgreSQL-related tasks.

Umherirrender unsubscribed.Jan 1 2018, 2:29 PM
Daimona added a parent task: T191740: Bundle AbuseFilter extension with MediaWiki.Apr 24 2018, 4:25 PM
Daimona mentioned this in T193068: Add support for SQLite and postgre when searching patterns.Apr 26 2018, 11:20 AM
matej_suchanek updated the task description. (Show Details)Jun 11 2018, 10:52 AM
matej_suchanek removed a subscriber: • wikibugs-l-list.
Daimona added a parent task: T199544: Make AbuseFilter work on PostgreSQL and SQLite (epic).Jul 13 2018, 2:52 PM
Daimona removed a parent task: T191740: Bundle AbuseFilter extension with MediaWiki.
Daimona added a project: DBA.Sep 10 2018, 6:15 PM
Daimona subscribed.

Right now we have a total of five cases where such a JOIN is done. Schema changes aren't something to take carelessly, but this column doesn't really make sense. The table is pretty large though (for instance, 22 millions rows on enwiki). I guess we should first add the new column, start writing it, and reading from both. Then clean the afl_filter column with a maintenance script and finally change it to be INT.
Adding DBA for thoughts about the process above.

• Marostegui removed a project: DBA.Sep 10 2018, 6:22 PM
• Marostegui subscribed.

We don't maintain Postresql
Sorry :-(
Me personally I don't even have experience with Postresql either

Daimona added a project: DBA.Sep 10 2018, 7:12 PM

@Marostegui Unfortunately, neither do I :-) However, PG is just the cause here. What we want to do (and I'm asking DBAs about) is perform a schema change on abuse_filter_action. Simply, we'd like to add a column (afl_filter_global) and ALTER the afl_column to be integer instead of text, for every DBMS. As I was saying, the table isn't that small, so we need to make sure that the process will be smooth.

• Marostegui added a comment.Sep 11 2018, 5:11 AM
In T42757#4572252, @Daimona wrote:

@Marostegui Unfortunately, neither do I :-) However, PG is just the cause here. What we want to do (and I'm asking DBAs about) is perform a schema change on abuse_filter_action. Simply, we'd like to add a column (afl_filter_global) and ALTER the afl_column to be integer instead of text, for every DBMS. As I was saying, the table isn't that small, so we need to make sure that the process will be smooth.

Why don't you just add a new INT column, start writing to both, migrate rows from the old one to the new one with a maintenance script running on the background and once all the old rows have been migrated, drop the old TEXT one and only use the new INT one?

• Marostegui moved this task from Triage to Blocked external/Not db team on the DBA board.Sep 11 2018, 5:12 AM
Daimona added a comment.Sep 11 2018, 8:06 AM

@Marostegui Perfect. I guess we should seize the opportunity and rename afl_filter to afl_filter_id to make sure that it's INT. At any rate, I think this should be momentarily delayed, since we're already dealing with several big changes to AF (actor migration, major overhaul for throttling and major overhaul for profiling). IMHO they should have the priority over this change, which (although necessary for performance etc.) is harmless on MySQL/MariaDB.

• Marostegui added a comment.Sep 11 2018, 1:11 PM
In T42757#4573297, @Daimona wrote:

@Marostegui Perfect. I guess we should seize the opportunity and rename afl_filter to afl_filter_id to make sure that it's INT.

That is up to you, just include it on the schema change once you are ready for it.
https://wikitech.wikimedia.org/wiki/Schema_changes#Workflow_of_a_schema_change

gerritbot subscribed.Sep 11 2018, 5:00 PM

Change 459818 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] [WIP] Split afl_filter in afl_filter_id and afl_global

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

gerritbot added a project: Patch-For-Review.Sep 11 2018, 5:00 PM
• Marostegui removed a project: DBA.Sep 11 2018, 5:39 PM

As there are no more actionables for the DBA Team here I will untag it.
I will remain subscribed here though in case you have further questions.
Once this is good to go just send the schema change ticket as described in the link in my previous comment above and we will take it from there.

Thanks!

Daimona added a comment.Sep 11 2018, 5:55 PM

@Marostegui Yep, many thanks :-) I'll go ahead with that as soon as we'll be ready.

Daimona mentioned this in T54201: API should always return the filter id as a number.Oct 12 2018, 11:21 AM
Daimona claimed this task.Oct 18 2018, 5:12 PM
gerritbot added a comment.Oct 20 2018, 1:57 PM

Change 468706 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use string cast for Postgres compatibility

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

Daimona mentioned this in T93564: Addition of last hit date to Special:AbuseFilter table.Feb 6 2019, 11:12 AM
gerritbot added a comment.Feb 6 2019, 3:38 PM

Change 488477 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Read afl_filter_id and afl_global instead of afl_filter

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

gerritbot added a comment.Feb 27 2019, 2:57 PM

Change 493240 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Start writing to afl_filter_id and afl_global

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

gerritbot added a comment.Mar 23 2019, 11:44 AM

Change 468706 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Use string cast for Postgres compatibility

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

gerritbot added a comment.Mar 28 2019, 2:29 PM

Change 459818 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Split afl_filter in afl_filter_id and afl_global

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

gerritbot added a comment.Mar 28 2019, 2:45 PM

Change 493240 abandoned by Daimona Eaytoy:
Start writing to afl_filter_id and afl_global

Reason:
Done in parent patch with a config option.

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

gerritbot added a comment.Mar 28 2019, 6:40 PM

Change 488477 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Read afl_filter_id and afl_global instead of afl_filter

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

gerritbot added a comment.Mar 29 2019, 1:32 PM

Change 488477 abandoned by Daimona Eaytoy:
Read afl_filter_id and afl_global instead of afl_filter

Reason:
Done in parent using schema flag.

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

gerritbot added a comment.Apr 11 2019, 9:50 PM

Change 468706 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Use string cast for Postgres compatibility

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

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.1; 2019-04-16).Apr 11 2019, 10:00 PM
Daimona closed this task as Resolved.Apr 12 2019, 7:34 AM
Daimona removed a project: Patch-For-Review.

Calling this resolved because the compatibility problem with postgres is indeed resolved. I'm gonna open another task for the long-term goal of splitting afl_filter in afl_filter_id and afl_global.

Daimona mentioned this in T191740: Bundle AbuseFilter extension with MediaWiki.Sep 13 2019, 4:49 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits