Page MenuHomePhabricator

File moves throw error when using postgres
Closed, ResolvedPublic

Description

@0xAFFF notes:
If I try to rename a file I get this SQL error:

ERROR:  FOR UPDATE is not allowed with aggregate functions
STATEMENT:  SELECT /* LocalFileMoveBatch::verifyDBUpdates  */  COUNT(*)  FROM "oldimage"    WHERE oi_name = 'Ssh-schlüssel-zu-gitlab-hinzufügen.webm'  LIMIT 1   FOR UPDATE

The used software versions:

SoftareVersion
MediaWiki1.28.0
PHP7.0.13 (apache2handler)
PostgreSQL9.6.1
TimedMediaHandler0.5.0 (64461e9) 21:18, 25. Okt. 2016
MwEmbedSupport0.3.0 (2e24ff4) 00:03, 25. Okt. 2016

@TheDJ notes
This part of the code was recently changed by @aaron in ad95e0182356224dce27f53f7bbea24ffbcb22a7, so I think it's likely to be a regression in 1.28

This was originally reported at T157424: TimedMediaHandler does not support PostgreSQL

Event Timeline

TheDJ created this task.Mar 20 2017, 1:19 PM
Restricted Application added projects: Multimedia, Commons. · View Herald TranscriptMar 20 2017, 1:19 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
TheDJ updated the task description. (Show Details)Mar 20 2017, 1:23 PM
Paladox updated the task description. (Show Details)
dr0ptp4kt triaged this task as Low priority.Aug 7 2017, 4:45 PM
dr0ptp4kt moved this task from Untriaged to Triaged on the Multimedia board.
dr0ptp4kt added a subscriber: dr0ptp4kt.

We're doing triaging this morning.

IijimaYun added a subscriber: IijimaYun.EditedOct 5 2017, 4:43 PM

File moving is still broken in 1.29.1:

2017-10-05 16:38:41 SELECT /* LocalFileMoveBatch::verifyDBUpdates  */ COUNT(*)  FROM "oldimage"    WHERE oi_name = 'Awuwei.png'  LIMIT 1   FOR UPDATE
2017-10-05 16:38:41 ROLLBACK /* Wikimedia\Rdbms\DatabasePostgres::reportQueryError  */
2017-10-05 16:38:41 SQL ERROR: ERROR:  FOR UPDATE is not allowed with aggregate functions

COUNT() is indeed an aggregate function and locking COUNT() rows is impossible.

In includes/filerepo/file/LocalFile.php

$oldRowCount = $dbw->selectField(
  'oldimage',
  'COUNT(*)',
  [ 'oi_name' => $this->oldName ],
  __METHOD__,
  [ 'FOR UPDATE' ]
);

The [ 'FOR UPDATE' ] line should be removed as even if it could lock rows, it would not be needed in this case. This is likely a copy and paste mistake. Without it everything works fine.

Change 425731 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] rdbms: make selectField() handle COUNT() with FOR UPDATE better

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

Change 425760 had a related patch set uploaded (by Aaron Schulz; owner: Aaron Schulz):
[mediawiki/core@master] Fix LocalFileMoveBatch query that was incompatibile with Postgres

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

Change 425760 merged by jenkins-bot:
[mediawiki/core@master] Fix LocalFileMoveBatch query that was incompatibile with Postgres

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

Change 425731 merged by jenkins-bot:
[mediawiki/core@master] rdbms: make select() warn when FOR UPDATE is used with aggregation

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

aaron closed this task as Resolved.Apr 23 2018, 10:10 PM
aaron claimed this task.