Last modified: 2014-01-31 11:30:05 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T49055, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 47055 - Revision::fetchFromConds SELECT ... FOR UPDATE invalid in Postgres
Revision::fetchFromConds SELECT ... FOR UPDATE invalid in Postgres
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Database (Other open bugs)
1.22.0
All All
: High normal with 2 votes (vote)
: 1.22.x release
Assigned To: Tyler Romeo
:
Depends on:
Blocks: postgres 57724 49523
  Show dependency treegraph
 
Reported: 2013-04-09 20:47 UTC by OverlordQ
Modified: 2014-01-31 11:30 UTC (History)
13 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description OverlordQ 2013-04-09 20:47:03 UTC
===
Apr  9 20:39:11 thedarkcitadel postgres[21778]: [3-1] 2013-04-09 20:39:10 UTC ERROR:  SELECT FOR UPDATE/SHARE cannot be applied to the nullable side of an outer join
Apr  9 20:39:11 thedarkcitadel postgres[21778]: [3-2] 2013-04-09 20:39:10 UTC STATEMENT:  SELECT /* Revision::fetchFromConds 127.0.0.1 */  rev_id,rev_page,rev_text_id,rev_timestamp,rev_comment,rev_user_text,rev_user,rev_minor_edit,rev_deleted,rev_len,rev_parent_id,rev_sha1,rev_content_format,rev_content_model,page_namespace,page_title,page_id,page_latest,page_is_redirect,page_len,user_name  FROM "unittest_revision" INNER JOIN "unittest_page" ON ((page_id = rev_page)) LEFT JOIN "unittest_mwuser" ON ((rev_user != 0) AND (user_id = rev_user))  WHERE page_id = '649' AND rev_id = '1055'  LIMIT 1   FOR UPDATE
===

This is likely what is breaking other phpunit tests
Comment 1 Fred Houweling 2013-06-08 01:46:34 UTC
i patched my Revision.php in the includes directory (1.21.1) with the following:

# diff Revision.php.org Revision.php
355a356
>               global $wgDBtype;
363c364,366
<                       $options[] = 'FOR UPDATE';
---
>                       if( $wgDBtype != "postgres" ) {
>                               $options[] = 'FOR UPDATE';
>                       }
Comment 2 Andre Klapper 2013-06-11 18:17:04 UTC
Hi! Thanks for your patch!

You are welcome to use Developer access
  https://www.mediawiki.org/wiki/Developer_access
to submit this as a Git branch directly into Gerrit:
  https://www.mediawiki.org/wiki/Git/Tutorial

Putting your branch in Git makes it easier to review it quickly.
Thanks again! We appreciate your contribution.
Comment 3 Fred Houweling 2013-06-12 08:48:31 UTC
Thanks Andre,
I did as you suggested.
https://gerrit.wikimedia.org/r/#/c/68143/
Regards
Fred
Comment 4 Tyler Romeo 2013-06-12 16:26:04 UTC
So what I would recommend is that rather than making this change in Revision::fetchFromConds, instead make the change directly in DatabasePostgres (maybe override the selectSQLText() function and have it remove the FOR UPDATE option). That way this error won't occur anywhere.
Comment 5 OverlordQ 2013-06-12 16:49:05 UTC
The problem is that PG *does* support FOR UPDATE, just limits it to cases that make sense.

Why is the userJoinCond a LEFT JOIN when you're fetching rows for logged-in users which should have a row in the users table?
Comment 6 Tyler Romeo 2013-06-12 17:35:39 UTC
(In reply to comment #5)
> The problem is that PG *does* support FOR UPDATE, just limits it to cases
> that
> make sense.

Then maybe an exception should be thrown instead, as a warning to developers that you shouldn't be doing FOR UPDATE on outer joins.

(In reply to comment #5)
> Why is the userJoinCond a LEFT JOIN when you're fetching rows for logged-in
> users which should have a row in the users table?

The issue is that the row doesn't necessarily exist in the user table. If the revision was made by an anonymous user, then there will be no corresponding row. The idea is to fetch the user row if it exists.
Comment 7 db [inactive,noenotif] 2013-06-12 19:34:43 UTC
(In reply to comment #6)
> The issue is that the row doesn't necessarily exist in the user table. If the
> revision was made by an anonymous user, then there will be no corresponding
> row. The idea is to fetch the user row if it exists.

That is wrong, because there is a rev_user = 0 on the join condition, so this is false for anon editing. The query should always find a user row.
Comment 8 Tyler Romeo 2013-06-12 22:09:54 UTC
(In reply to comment #7)
> That is wrong, because there is a rev_user = 0 on the join condition, so this
> is false for anon editing. The query should always find a user row.

"on the join condition" is the key phrase here. If rev_user = 0 were in the WHERE clause, then that'd be true, but in an outer join, if the join condition doesn't match, the row is still there but just has a null join. Also think about it logically: why would Revision::fetchFromConds, the main function for fetching revision info, only allow logged in revisions?
Comment 9 Fred Houweling 2013-06-12 23:17:56 UTC
Hi Thanks all for exploring options,
Some related discussions I found when researching the error:
http://www.postgresql.org/docs/9.1/interactive/sql-select.html#SQL-FOR-UPDATE-SHARE
http://www.postgresql.org/docs/9.1/interactive/explicit-locking.html#LOCKING-ROWS

http://www.postgresql.org/message-id/21634.1160151923@sss.pgh.pa.us
http://postgresql.1045698.n5.nabble.com/outer-joins-and-for-update-td1937029.html

Also, my PostgreSQL server version is 9.1.2

If we can reach an agreement on how best to resolve this issue for Wikimedia installs using a PostgreSQL databases, (unless someone beats me to it) I'll rework the patch to comply to your collective wisdom, thank you all for your time.
Comment 10 Tyler Romeo 2013-06-12 23:57:07 UTC
OK, so with all of this, I also noticed one more thing: PostgreSQL has an extended FOR UPDATE syntax. You can do "FOR UPDATE OF mytable", which will only lock that table.

With that in mind, I think the proper fix for this would be to change DatabasePostgresql so that when the FOR UPDATE clause is generated, it only includes the main table and inner joined tables.
Comment 11 Pierre Bourdon 2013-06-20 19:00:09 UTC
This bug causes image deletion to fail on PostgreSQL, by the way. Manually applying the workaround mentioned above fixes the problem.
Comment 12 Gerrit Notification Bot 2013-06-20 19:41:58 UTC
Related URL: https://gerrit.wikimedia.org/r/69767 (Gerrit Change I1ac587ac39f448b9e7f4befb44826b43044ad6f0)
Comment 13 Marcin Cieślak 2013-07-17 15:04:59 UTC
What about if we require callers to explicitly specify list of tables to be locked (by using additional parameters and not $options[]) ?
Comment 14 Gerrit Notification Bot 2013-10-15 13:07:15 UTC
Change 69767 abandoned by coren:
Changed FOR UPDATE handling in Postgresql

Reason:
Worked only because it was broken the right way.  :-)

https://gerrit.wikimedia.org/r/69767
Comment 15 Marc A. Pelletier 2013-10-15 13:23:34 UTC
(In reply to comment #10)
> PostgreSQL has an
> extended FOR UPDATE syntax. You can do "FOR UPDATE OF mytable", which will
> only
> lock that table.

Hmm, no.  FOR UPDATE OF isn't a postgres extension, but a part SQL which mysql doesn't implement fully (SQL:2003 14.1.).

But yeah, trying to lock on an outer join is fundamentally nonsensical, and mysql not throwing an exception is a standard compliance bug at best - it's unwise to rely on that kind of behaviour.
Comment 16 Fred Houweling 2013-10-16 07:42:50 UTC
I agree with Marc,
It is broken the wrong way, there is still an issue that needs to be resolved.

(In reply to comment #15)
> (In reply to comment #10)
> > PostgreSQL has an
> > extended FOR UPDATE syntax. You can do "FOR UPDATE OF mytable", which will
> > only
> > lock that table.
> 
> Hmm, no.  FOR UPDATE OF isn't a postgres extension, but a part SQL which
> mysql
> doesn't implement fully (SQL:2003 14.1.).
> 
> But yeah, trying to lock on an outer join is fundamentally nonsensical, and
> mysql not throwing an exception is a standard compliance bug at best - it's
> unwise to rely on that kind of behaviour.
Comment 17 Gerrit Notification Bot 2013-11-04 02:22:33 UTC
Change 69767 restored by Parent5446:
Changed FOR UPDATE handling in Postgresql

Reason:
Why was this abandoned? It's my change.

https://gerrit.wikimedia.org/r/69767
Comment 18 Nemo 2013-12-30 09:29:23 UTC
Update per https://www.mediawiki.org/w/index.php?title=MediaWiki_1.22/Known_issues&oldid=847749 which seems to be the master list.
Comment 19 Gerrit Notification Bot 2014-01-13 03:10:32 UTC
Change 107112 had a related patch set uploaded by MarkAHershberger:
Changed FOR UPDATE handling in Postgresql

https://gerrit.wikimedia.org/r/107112
Comment 20 Gerrit Notification Bot 2014-01-13 03:15:49 UTC
Change 69767 merged by jenkins-bot:
Changed FOR UPDATE handling in Postgresql

https://gerrit.wikimedia.org/r/69767
Comment 21 Gerrit Notification Bot 2014-01-13 03:17:09 UTC
Change 107112 merged by jenkins-bot:
Changed FOR UPDATE handling in Postgresql

https://gerrit.wikimedia.org/r/107112
Comment 22 Nemo 2014-01-31 11:30:05 UTC
Having fixed this bug allegedly unblocks https://gerrit.wikimedia.org/r/#/c/98045/

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links