Page MenuHomePhabricator

PostgreSQL LOCK IN SHARE MODE option is a syntax error
Closed, ResolvedPublic

Description

Author: ironiridis

Description:
When refreshing a category page, MediaWiki issues a query to my PostgreSQL backend ending in "LOCK IN SHARED MODE". This isn't valid syntax to PGSQL.

I have edited my copy of includes/db/Database.php, DatabaseBase::selectSQLText() to include...

if ($this->getType() == 'postgres' and
        (in_array('LOCK IN SHARE MODE', $options, true) or
        isset($options['LOCK IN SHARE MODE'])))
{
        // just fail       
        //throw new Exception('Cannot use LOCK IN SHARE MODE with Postgresql');
        // or just remove the option
        unset($options['LOCK IN SHARE MODE']);
        unset($options[array_search('LOCK IN SHARE MODE', $options, true)]);
}

The lack of a locking operation seems to me to be a better tradeoff than a category page flooded with pg_query error output.


Version: 1.19.0
Severity: major

Details

Reference
bz39635

Related Objects

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
InvalidNone
ResolvedNone

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:08 AM
bzimport set Reference to bz39635.

ironiridis wrote:

Avoid SQL Syntax Error on Postgres

Attached:

ironiridis wrote:

Add keywords that might help identify how trivial the fix is...

overlordq wrote:

What are the steps to reproduce this? I cannot say I've ran into this issue.

ironiridis wrote:

Viewing a category page reproduces it for me after creating about 20 articles in various categories from a clean install.

I can uncomment the exception line and generate a backtrace if that's useful.

Gentoo Linux (portage versions listed)
MediaWiki 1.19.1 (not available from this Bugzilla's version selector)
PostgreSQL 9.1.4-r1
Apache 2.2.22-r1
PHP 5.4.6

ironiridis wrote:

Just to add more comment spam (sorry) it's worth noting that the patch is sane even if this isn't a bug in current code, since the 'LOCK IN SHARED MODE' isn't valid syntax in Postgres, and will always fail. Hence why I left an exception there (but commented) as an option instead of silently discarding the flag, as that particular flag simply doesn't make sense with Postgres and should be treated as a bug.

I assume Mediawiki has some sort of assert() construct, though I'm not a MW dev so I'm not aware of it. If so, that might be a smarter approach.

This is was added in r32085 to bring in "Category" object. I wonder if this construct shouldn't really become "SELECT FOR UPDATE" instead?

I think we should really patch it like this:

iff --git a/includes/db/DatabasePostgres.php b/includes/db/DatabasePostgres.php
index 8f8f5e8..457bf38 100644

  • a/includes/db/DatabasePostgres.php

+++ b/includes/db/DatabasePostgres.php
@@ -1406,9 +1406,6 @@ SQL;

if ( isset( $noKeyOptions['FOR UPDATE'] ) ) {
        $postLimitTail .= ' FOR UPDATE';
}
  • if ( isset( $noKeyOptions['LOCK IN SHARE MODE'] ) ) {
  • $postLimitTail .= ' LOCK IN SHARE MODE';
  • } if ( isset( $noKeyOptions['DISTINCT'] ) || isset( $noKeyOptions['DISTINCTROW'] ) ) { $startOpts .= 'DISTINCT'; }

Since getting the code path in question executed is not easy (I can't reproduce the problem right now), can you see if this patch helps?

Patch filed as gerrit change 21606

Thank you for your patch. It has been successfully merged into the git repository this night.

In the future, if you're willing to submit patch, you can request a developer access, to be able to push code directly in our code review (Gerrit) infrastructure.

[Removing Linux platform, issue isn't OS depending. Pruning keywords. Bug assigned to code submitter. Resolved fixed.]

(In reply to comment #8)

In the future, if you're willing to submit patch, you can request a developer
access, to be able to push code directly in our code review (Gerrit)
infrastructure.

You can do so at https://www.mediawiki.org/wiki/Developer_access

  • Bug 46594 has been marked as a duplicate of this bug. ***
Jdforrester-WMF subscribed.

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