Page MenuHomePhabricator

PostgreSQL misuse of pg_get_result
Closed, ResolvedPublic

Description

Author: mike

Description:
Shortly after upgrading PHP (to 5.4.13) and MediaWiki (to 1.21.1), I saw the following line in Apache's error log:

[Mon Jun 24 07:39:43 2013] [error] FastCGI: server "/var/www/localhost/cgi-bin/php.fcgi" stderr: PHP Notice: pg_send_query(): There are results on this connection. Call pg_get_result() until it returns FALSE in /var/www/localhost/htdocs/mediawiki/includes/db/DatabasePostgres.php on line 441

Line 441 is the call to pg_send_query in here:

public function doQuery( $sql ) {
    if ( function_exists( 'mb_convert_encoding' ) ) {
        $sql = mb_convert_encoding( $sql, 'UTF-8' );
    }
    $this->mTransactionState->check();
    if( pg_send_query( $this->mConn, $sql ) === false ) {
        throw new DBUnexpectedError( $this, "Unable to post new query to PostgreSQL\n" );
    }
    $this->mLastResult = pg_get_result( $this->mConn );
    $this->mTransactionState->check();
    $this->mAffectedRows = null;
    if ( pg_result_error( $this->mLastResult ) ) {
        return false;
    }
    return $this->mLastResult;
}

I Googled and found the following thread: http://www.postgresql.org/message-id/flat/gtitqq$26l3$1@news.hub.org#gtitqq$26l3$1@news.hub.org

If I understand, PHP has detected that the results of one query are still pending when a second query is issued. To prevent this, it wants the results of each query to be cleared by *polling* pg_get_result() instead of making just a single call to that function.


Version: 1.21.x
Severity: normal

Details

Reference
bz50091

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.
StatusAssignedTask
InvalidNone
ResolvedYoonghm

Event Timeline

bzimport raised the priority of this task from to Normal.Nov 22 2014, 1:57 AM
bzimport added a project: Wikimedia-Rdbms.
bzimport set Reference to bz50091.
bzimport added a subscriber: Unknown Object (MLST).

mike wrote:

I've since replicated the bug, but only in API requests aimed at Semantic MediaWiki 1.7.1 (action=ask). Maybe that extension is the only source.

In any case, I'm now porting the affected wikis to MySQL. SMW's support for postgres has tanked lately.

saper added a comment.Jul 17 2013, 2:18 PM

Can you enable $wgDebugLogFile and $wgDebugDBTransactions and see what's there?
I wonder what kind of transitions do you get in this case.

Some functions in includes/db/DatabasePostgres.php does not clear PQresult after sending query to PostgreSQL.

To avoid the error/warning, can make the following changes, i.e., obtain result via pg_get_result(), clear the result via pg_free_result, before calling pg_send_query()

	public function doQuery( $sql ) {
		if ( function_exists( 'mb_convert_encoding' ) ) {
			$sql = mb_convert_encoding( $sql, 'UTF-8' );
		}
		$this->mTransactionState->check();

		// Read any incoming PQresult left in this channel. Who left it there?
		while ($res = pg_get_result( $this->mConn ) )
			pg_free_result($res);

		if ( pg_send_query( $this->mConn, $sql ) === false ) {
			throw new DBUnexpectedError( $this, "Unable to post new query to PostgreSQL\n" );
		}
		$this->mLastResult = pg_get_result( $this->mConn );
		$this->mTransactionState->check();
		$this->mAffectedRows = null;
		if ( pg_result_error( $this->mLastResult ) ) {
			return false;
		}

		return $this->mLastResult;
	}
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptSep 25 2015, 5:55 AM

@Yoonghm: Thanks for taking a look at the code!

You are very welcome to use developer access to submit code changes as a Git branch directly into Gerrit.
If you don't want to set up Git/Gerrit, you can also use the Gerrit Patch Uploader.

It takes a lot of effort and knowledge to contribute via https://gerrit.wikimedia.org

I have created a branch, T52091_postgresql_read_PQresult

$ git review -R
Your change was committed before the commit hook was installed.
Amending the commit to add a gerrit change id.
remote: Resolving deltas: 100% (10/10)
remote: Processing changes: new: 1, refs: 1, done    
remote: (W) f401308: commit subject >100 characters; use shorter first paragraph
remote: 
remote: New Changes:
remote:   https://gerrit.wikimedia.org/r/241357
remote: 
To ssh://yoonghm@gerrit.wikimedia.org:29418/mediawiki/core.git
 * [new branch]      HEAD -> refs/publish/master/T52091_postgresql_read_PQresult

Please let me know what else I can do. I performed this but I believe the code need review...

$ git push
warning: push.default is unset; its implicit value is changing in
Git 2.0 from 'matching' to 'simple'. To squelch this message
and maintain the current behavior after the default changes, use:

  git config --global push.default matching

To squelch this message and adopt the new behavior now, use:

  git config --global push.default simple

When push.default is set to 'matching', git will push local branches
to the remote branches that already exist with the same name.

In Git 2.0, Git will default to the more conservative 'simple'
behavior, which only pushes the current branch to the corresponding
remote branch that 'git pull' uses to update the current branch.

See 'git help config' and search for 'push.default' for further information.
(the 'simple' mode was introduced in Git 1.7.11. Use the similar mode
'current' instead of 'simple' if you sometimes use older versions of Git)

To ssh://yoonghm@gerrit.wikimedia.org:29418/mediawiki/core.git
 ! [rejected]        master -> master (non-fast-forward)
error: failed to push some refs to 'ssh://yoonghm@gerrit.wikimedia.org:29418/mediawiki/core.git'
hint: Updates were rejected because a pushed branch tip is behind its remote
hint: counterpart. If you did not intend to push that branch, you may want to
hint: specify branches to push or set the 'push.default' configuration variable
hint: to 'simple', 'current' or 'upstream' to push only the current branch.
spsy@eew:/home/s/src/mediawiki/core$ 
spsy@eew:/home/s/src/mediawiki/core$ git status
On branch T52091_postgresql_read_PQresult
Your branch and 'origin/master' have diverged,
and have 1 and 8 different commits each, respectively.
  (use "git pull" to merge the remote branch into yours)

nothing to commit, working directory clean

Change 241357 had a related patch set uploaded (by Brian Wolff):
Fix "pg_send_query(): There are results on this connection..."

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

Bawolff added a subscriber: Bawolff.EditedSep 26 2015, 7:16 PM

Change 241357 had a related patch set uploaded (by Brian Wolff):

The gerritbot is buggy and gets confused when people edit the commit message. I obviously was not the one who uploaded the patchset.


Just for future reference, if you include a line like Bug: T52091 at the bottom of the commit message, the patch will be automatically associated with the bug


In regards to git push. The thing you did with git review worked fine. But for future reference you can do git push instead if that makes more sense to you, you just have to have the foreign refspec be refs/for/master. So assuming you're in a branch with the commit you want to push e.g.

git push ssh://yoonghm@gerrit.wikimedia.org:29418/mediawiki/core.git HEAD:refs/for/master

But most people find that confusing. So we normally just tell people to use git review.

Change 241357 abandoned by Yoonghm:
Fix "pg_send_query(): There are results on this connection..."

Reason:
Failed code quality. Re-submit.

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

Change 241357 restored by Yoonghm:
Fix "pg_send_query(): There are results on this connection..."

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

Change 241357 abandoned by Yoonghm:
Fix "pg_send_query(): There are results on this connection..."

Reason:
Redo in another branch

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

Change 241497 had a related patch set uploaded (by Aaron Schulz):
Clear previously left-over PQresult before calling pg_get_result()

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

Change 241497 merged by jenkins-bot:
Clear previously left-over PQresult before calling pg_get_result()

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

scfc closed this task as Resolved.Nov 4 2015, 1:25 AM
scfc assigned this task to Yoonghm.
scfc set Security to None.

Change 282084 had a related patch set uploaded (by Cicalese):
Merge "Clear previously left-over PQresult before calling pg_get_result()" into REL1_26

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

Change 282084 merged by jenkins-bot:
Merge "Clear previously left-over PQresult before calling pg_get_result()" into REL1_26

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

Jdforrester-WMF added a subscriber: Jdforrester-WMF.

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