Wikimedia Bugzilla is closed!

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

(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?

(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.

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.

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?

(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.

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

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

(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.

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().

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

(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.