Can also be reproduced on Toolserver Wiki (which uses PostgreSQL [1])

Log in at https://wiki.toolserver.org/ and go to Special:Watchlist and click the RSS feed

feed:https://wiki.toolserver.org/w/api.php?action=feedwatchlist&allrev=allrev&wlowner=USERNAME&wltoken=TOKEN&feedformat=atom

A database error has occurred. Did you forget to run maintenance/update.php after upgrading? See: http://www.mediawiki.org/wiki/Manual:Upgrading#Run_the_update_script
Query: SELECT rc_namespace,rc_title,rc_timestamp,rc_cur_id,rc_this_oldid,rc_user,rc_user_text,rc_comment FROM watchlist,page,recentchanges WHERE (wl_namespace = rc_namespace) AND (wl_title = rc_title) AND (rc_cur_id = page_id) AND wl_user = '1109' AND rc_deleted = '0' AND (rc_timestamp>='20110314230557') ORDER BY rc_timestamp DESC LIMIT 51
Function: ApiQueryWatchlist::run
Error: 1 ERROR: invalid input syntax for type timestamp with time zone: "20110314230557"

Krinkle

[1] https://wiki.toolserver.org/wiki/Special:Version

Bryan.TongMinh wrote:

(In reply to comment #0)

The logic causing this problem is intricate and I cannot suggest a fix.
However, somehow the TS_POSTGRES formatted time should be supplied to the
pg_query, not the TS_MW formatted time. How to effect this change and still
maintain other valid uses of ApiQuery isn't clear (at least it isn't clear to
me).

Is this something that $this->getDB()->timestamp() can't fix?

dnessett wrote:

(In reply to comment #2)

(In reply to comment #0)

The logic causing this problem is intricate and I cannot suggest a fix.
However, somehow the TS_POSTGRES formatted time should be supplied to the
pg_query, not the TS_MW formatted time. How to effect this change and still
maintain other valid uses of ApiQuery isn't clear (at least it isn't clear to
me).

Is this something that $this->getDB()->timestamp() can't fix?

I'm not sure. I don't have a full understanding of the traversed code. The RSS Watchlist logic uses the API subsystem. This may be used in other ways that require the timestamp to be in TS_MW format. Someone with more intimate knowledge of this part of MW will have to determine what is going on and how to solve the problem.

dnessett wrote:

I am investigating the code paths that cause in this bug. The problem occurs in getParameterFromSettings(), which is a method of the ApiBase class. The ApiQueryWatchlist class (which implements the RSS feeds for WatchLists) extends the ApiQueryGeneratorBase class, which extends the ApiQueryBase class, which extends the ApiBase class. The getParameterFromSettings() method in ApiQueryWatchlist comes directly through this class hierarchy from ApiBase.

It seems clear that getParameterFromSettings() should be overridden in ApiQueryWatchlist and the switch case code for 'timestamp' modified to return the database formated value. However, there is a lot of code in getParameterFromSettings() that would be duplicated if the whole method is copied to ApiQueryWatchlist and then modified. This would introduce significant and unnecessary maintenance burden.

So my guess is to use one of two other possibilities, which are:

+ Pull out the switch statement in getParameterFromSettings() into a separate protected method. Only this method need be overridden in ApiQueryWatchlist.

+ Add a parameter to getParameterFromSettings() that instructs it not to convert timestamps into TS_MW format.

The former possibility seems the best from an architectural point of view, but requires the largest code change. The latter possibility requires the least amount of change to the code, but is dirtier.

I need some guidance from someone (an MW architect? Does anyone have that title in this open source project?) about the best way to proceed. Either approach is fairly easy to implement, but since I do not understand this part of the code very well, there may be reasons why one or the other approach is best.

dnessett wrote:

After thinking about it a bit more, a variation of the first possibility is to encapsulate the code currently in the case "timestamp' arm of the switch statement into a protected method and then override it in ApiQueryWatchlist. This minimizes code changes and doesn't mess up the method signatures with bandaids. Furthermore, it probably changes the code even less than the first option, since the latter requires modifying the method signatures of both extractRequestParams() and getParameterFromSettings().

(In reply to comment #4)

I am investigating the code paths that cause in this bug. The problem occurs in
getParameterFromSettings(), which is a method of the ApiBase class. The
ApiQueryWatchlist class (which implements the RSS feeds for WatchLists) extends
the ApiQueryGeneratorBase class, which extends the ApiQueryBase class, which
extends the ApiBase class. The getParameterFromSettings() method in
ApiQueryWatchlist comes directly through this class hierarchy from ApiBase.

It seems clear that getParameterFromSettings() should be overridden in
ApiQueryWatchlist and the switch case code for 'timestamp' modified to return
the database formated value.

Please don't! Just convert timestamps to DB format using $this->getDB(
)->timestamp( $ts ), where $ts can be in any timestamp format recognized by MediaWiki. Is there any reason this won't work?

dnessett wrote:

(In reply to comment #6)

(In reply to comment #4)

I am investigating the code paths that cause in this bug. The problem occurs in
getParameterFromSettings(), which is a method of the ApiBase class. The
ApiQueryWatchlist class (which implements the RSS feeds for WatchLists) extends
the ApiQueryGeneratorBase class, which extends the ApiQueryBase class, which
extends the ApiBase class. The getParameterFromSettings() method in
ApiQueryWatchlist comes directly through this class hierarchy from ApiBase.

It seems clear that getParameterFromSettings() should be overridden in
ApiQueryWatchlist and the switch case code for 'timestamp' modified to return
the database formated value.

Please don't! Just convert timestamps to DB format using $this->getDB(
)->timestamp( $ts ), where $ts can be in any timestamp format recognized by
MediaWiki. Is there any reason this won't work?

It may work to fix the bug, but the conversion of timestamps to TS_MW format in getParameterFromSettings() has been around much longer than the RSS Watchlist code. My concern is there are other uses of getParameterFromSettings() that rely on timestamps being converted to TS_MW format. If the case 'timestamp' code is changed to call $this->getDB()->timestamp(), who knows what other code will break.

That is why someone who understands the API classes and their uses needs to determine whether a change to the APIBase abstract class method code is the correct way to fix this bug. Are you that person? Are there uses of getParameterFromSettings() that are for purposes other than an ultimate call to a db select?

Obviously, I could just change the code according to your suggestion, but I would only test the RSS/Watchlist case. I wouldn't know if the fix breaks other things.

marking as tarball blocker until I can verify that this isn't present there.

Bryan.TongMinh wrote:

Database::timestamp fix for rc_timestamp

Please check if the attached patch fixes this issue on Postgres.

Attached:

dnessett wrote:

(In reply to comment #9)

Created attachment 8327 [details]
Database::timestamp fix for rc_timestamp

Please check if the attached patch fixes this issue on Postgres.

Yes, it appears to. I have installed the patch on http://test.citizendium.org (although you need to be a CZ registered user to test it there). Perhaps some other postgres based wikis might test the patch as well.

Note: our test wiki uses MW 1.16.2. I am unable to test the patch for current trunk.

Attached:

Bryan.TongMinh wrote:

Fixed in r84765.

(In reply to comment #7)

My concern is there are other uses of getParameterFromSettings() that
rely on timestamps being converted to TS_MW format. If the case 'timestamp'
code is changed to call $this->getDB()->timestamp(), who knows what other code
will break.

Yeah, so that shouldn't be done. Instead, the watchlist code should convert the timestamp.

That is why someone who understands the API classes and their uses needs to
determine whether a change to the APIBase abstract class method code is the
correct way to fix this bug.

It's not. API modules cannot rely on the timestamp format returned by getParameterFromSettings() being TS_MW (or anything else, for that matter), they should *always* convert it to the desired format with wfTimestamp().

Are you that person?

I'd say I'm one of them. I used to be the primary API dev for two years.

Are there uses of
getParameterFromSettings() that are for purposes other than an ultimate call to
a db select?

I'm sure there are.

Obviously, I could just change the code according to your suggestion, but I
would only test the RSS/Watchlist case. I wouldn't know if the fix breaks other
things.

I was talking about converting the timestamp in the watchlist module (which is what Bryan did in his fix), not converting it in getParameterFromSettings().

dnessett wrote:

This fix was not carried forward into 1.16.5. I don't know whether 1.16 is still being maintained, but if so, this should be corrected.

(In reply to comment #13)

This fix was not carried forward into 1.16.5. I don't know whether 1.16 is
still being maintained, but if so, this should be corrected.

https://www.mediawiki.org/wiki/Version_lifecycle

dnessett wrote:

(In reply to comment #14)

(In reply to comment #13)

This fix was not carried forward into 1.16.5. I don't know whether 1.16 is
still being maintained, but if so, this should be corrected.

https://www.mediawiki.org/wiki/Version_lifecycle

OK, so 1.16 is no longer being maintained. However, unless I misunderstand the maintenance process (which is certainly possible), this fix should have been integrated into 1.16.5, which raises a process question.

The fix was integrated into r84765. 1.16.5 is r87484. Since the latter is subsequent to the former, why wasn't the fix carried forward?

Only specific fixes get merged into maintianance branches (Now a days we tag things to merge on CR with things like 1.18, i don't know if we did that back then), I suppose nobody thought to merge this fix into the 1.16 branch back in the day.

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