Page MenuHomePhabricator

rebuildrecentchanges.inc fails with SQL error (regression in 1.5.7)
Closed, ResolvedPublic

Description

Author: griff.phillips

Description:
Function "insertSelect" in Database.php has been modified recently and now contains:

$sql = "INSERT $options INTO $destTable (" . implode( ',', array_keys( $varMap )
) . ')' .
' SELECT ' . implode( ',', $varMap ) .
" FROM $srcTable";

This is invalid SQL syntax ... it is not valid to place column names between
INSERT and INTO.

This causes rebuildall.php to crash with "Error: 1064 You have an error in your
SQL syntax..."


Version: 1.5.x
Severity: normal

Details

Reference
bz5195

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 9:08 PM
bzimport set Reference to bz5195.
bzimport added a subscriber: Unknown Object (MLST).

The column names are clearly after INTO, not between INSERT and INTO.
Can you explain what you mean and what you think is the correct syntax?

griff.phillips wrote:

Yes you are absolutely right, I meant the values, not the column names. These
are generated from $options inbetween INSERT and INTO, ie "INSERT $options
INTO". Removing the $options variable allows rebuildall.php to complete
correctly. Whether this workaround version completes exactly as the programmer
intended I don't know. But looking at the syntax guide in the MySQL docs, there
doesn't seem to be any situations where it is considered valid to insert tokens
of any kind between INSERT and INTO, so the placing of $option looks to be
incorrect.

The documentation clearly shows that LOW_PRIORITY, HIGH_PRIORITY, and
IGNORE may appear between INSERT and INTO on an INSERT ... SELECT
statement.

http://dev.mysql.com/doc/refman/5.0/en/insert-select.html

The IGNORE option is commonly used when merging things which might
have duplicate (already-existing) entries which would violate unique
index constraints.

It all looks correct to me; this is a total non-sequitur which
obscured the actual error. The offending SQL is:

INSERT rev_timestamp 5000 INTO recentchanges
(rc_timestamp,rc_cur_time,rc_user,rc_user_text,rc_namespace,rc_title,r
c_comment,rc_minor,rc_bot,rc_new,rc_cur_id,rc_this_oldid,rc_last_oldid
,rc_type) SELECT
rev_timestamp,rev_timestamp,rev_user,rev_user_text,page_namespace,page
_title,rev_comment,rev_minor_edit,0,page_is_new,page_id,rev_id,0, IF
(page_is_new != 0, 1, 0) FROM page,revision WHERE (rev_timestamp

  1. AND (rev_page=page_id)

The calling code uses these options:
array( 'ORDER BY' => 'rev_timestamp', 'LIMIT' => 5000 )

The problem here seems to be that this code is expecting to set
options for the SELECT portion, but Database::insertSelect uses them
for the INSERT. However since this function didn't *HAVE* an $options
parameter previously, it never would have worked -- the options would
have been silently dropped and never made it into the SQL.

It might be a good idea to add another parameter for SELECT options
and treat it as for the various SELECT wrappers.

griff.phillips wrote:

I think "non-sequitur" is overstating the case somewhat! OK, I've now learnt it
is allowable to put certain keywords inbetween INSERT and INTO... but still not
"rev_timestamp 5000". So the value contained in $options was the problem, rather
than the placement of $options between the keywords. Hey, I'm just trying to
tell people about a bug I found which stopped my MediaWiki installation working.
If I'm going to get shot down, I won't bother reporting it next time.

If digging to find out the exact cause of the error you saw and recommending a
fix for it is getting "shot down", I don't want to know what us being helpful
looks like. :)

griff.phillips wrote:

OK, well for future reference "this is a total non-sequitur which obscured the
actual error" kind of reads like "get lost and stop wasting our time". But if
that's not what you meant by it, fair enough. I'm not actually a programmer
(these days) so I'm not in a position to try and fix up the code - I'm just
trying to get a MediaWiki installation up and running in our company so that I
can persuade them that Wikis are a good thing. So I thought that it might be
useful to you if I raised the error. Out of interest, is this kind of bug
something that Wikimedia would be likely to fix up in a forthcoming release, or
does it require someone in the open-source community to volunteer to do it ? As
far as I can tell, it seems like a reasonable showstopper if people can't run
rebuildall.php anymore ?

I'd expect most people in your situation to be running a release version; if this
is a recent change in CVS it shouldn't be affecting you. Can you provide some detail
of your installation, such as what version you're running, if you're performed any
upgrades, etc?

If this is in a release version, it's a regression and thus a bit more important than
if you're running this on the development branch. (rebuildall is not something we use,
ever, on our own servers.)

griff.phillips wrote:

We usually run rebuildall.php after a big Import, because we get no Search
matches on the imported data until we have done so.

Regarding the version of MediaWiki, I've got the package from "Current version"
on the MediaWiki download page, file mediawiki-1.5.7.tar.gz. So I guess that
counts as a release version ? I think this is a regression, because we managed
to run rebuildall.php fine with earlier versions of MediaWiki (but I can't tell
you exactly which ones, sorry.)

Looks like the change was backported to 1.5.7; changed bug summary to target the
problem area.

Griff, as a workaround try rebuildtextindex.php instead of rebuildall.php. If all
you want is to update the search index, this will skip the other unnecessary
scripts.

griff.phillips wrote:

Thanks Brion, I'll give it a try and let you know.

stefan wrote:

Running into the "same" problems with running rebuildall on a 1.5.7 installation:

  • Rebuilding recentchanges table:

Loading from page and revision tables...
A database error has occurred
Query: INSERT rev_timestamp 5000 INTO wiki_recentchanges (rc_timestamp,rc_cur_time,rc_user,rc_user_text,
rc_namespace,rc_title,rc_comment,rc_minor,rc_bot,rc_new,rc_cur_id,rc_this_oldid,rc_last_oldid,rc_type)
SELECT rev_timestamp,rev_timestamp,rev_user,rev_user_text,page_namespace,page_title,rev_comment,
rev_minor_edit,0,page_is_new,page_id,rev_id,0, IF(page_is_new != 0, 1, 0) FROM wiki_page,wiki_revision
WHERE (rev_timestamp > 20060301164330) AND (rev_page=page_id)
Function: rebuildRecentChangesTablePass1
Error: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server
version for the right syntax to use near '5000 INTO wiki_recentchanges (rc_timestamp,rc_cur_time,rc_user,
rc_user_text,rc' at line 1 (localhost)

griff.phillips wrote:

Hi Brion,

Yes, rebuildtextindex.php works fine for situations after I have just run an
Import, thanks for that.

(But I still need rebuildall.php to run in situations after I have made changes
to Language.php.)

robchur wrote:

(In reply to comment #12)

(But I still need rebuildall.php to run in situations after I have made changes
to Language.php.)

Again, as a workaround; give rebuildMessages.php a whirl.

Fixed on trunk in r13453.
Will be in 1.6.0; could be backported to 1.5.9 if desired.

ms wrote:

i vote for a backport to 1.5.9 because i use the unchanged distribution
from debian.