Last modified: 2012-08-04 20:48:56 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 28070 - RSS feed for watchlist throws fatal error when underlying db is postgres
RSS feed for watchlist throws fatal error when underlying db is postgres
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Watchlist (Other open bugs)
1.16.x
All All
: High critical (vote)
: ---
Assigned To: Mark A. Hershberger
:
Depends on:
Blocks: postgres 26676
  Show dependency treegraph
 
Reported: 2011-03-15 23:00 UTC by Dan Nessett
Modified: 2012-08-04 20:48 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments
Database::timestamp fix for rc_timestamp (1.05 KB, patch)
2011-03-25 19:42 UTC, Bryan Tong Minh
Details

Description Dan Nessett 2011-03-15 23:00:15 UTC
Clicking on the Toolbox link "RSS" on the "My watchlist" page throws a fatal error. The traceback is:

  1: 
  2: Warning: pg_query(): Query failed: ERROR:  invalid input syntax for type timestamp with time zone: "20110314172654" at character 327 in /czdata/dbg/phase3/includes/db/DatabasePostgres.php on line 584
  3: 
  4: Call Stack:
  5:     0.0002     673696   1. {main}() /czdata/dbg/phase3/api.php:0
  6:     0.0734   16784344   2. ApiMain->execute() /czdata/dbg/phase3/api.php:116
  7:     0.0734   16784392   3. ApiMain->executeActionWithErrorHandling() /czdata/dbg/phase3/includes/api/ApiMain.php:322
  8:     0.0734   16825512   4. ApiMain->executeAction() /czdata/dbg/phase3/includes/api/ApiMain.php:338
  9:     0.0760   17293168   5. ApiFeedWatchlist->execute() /czdata/dbg/phase3/includes/api/ApiMain.php:595
 10:     0.0762   17312384   6. ApiMain->execute() /czdata/dbg/phase3/includes/api/ApiFeedWatchlist.php:95
 11:     0.0762   17312432   7. ApiMain->executeAction() /czdata/dbg/phase3/includes/api/ApiMain.php:320
 12:     0.0778   17568576   8. ApiQuery->execute() /czdata/dbg/phase3/includes/api/ApiMain.php:595
 13:     0.0819   18536048   9. ApiQueryWatchlist->execute() /czdata/dbg/phase3/includes/api/ApiQuery.php:233
 14:     0.0819   18536048  10. ApiQueryWatchlist->run() /czdata/dbg/phase3/includes/api/ApiQueryWatchlist.php:44
 15:     0.0860   18612800  11. ApiQueryBase->select() /czdata/dbg/phase3/includes/api/ApiQueryWatchlist.php:188
 16:     0.0860   18612848  12. DatabaseBase->select() /czdata/dbg/phase3/includes/api/ApiQueryBase.php:244
 17:     0.0861   18613256  13. DatabaseBase->query() /czdata/dbg/phase3/includes/db/Database.php:874
 18:     0.0861   18614056  14. DatabasePostgres->doQuery() /czdata/dbg/phase3/includes/db/Database.php:517
 19:     0.0862   18614648  15. pg_query() /czdata/dbg/phase3/includes/db/DatabasePostgres.php:584
 20: 
 21: <?xml version="1.0"?>
 22: <?xml-stylesheet type="text/css" href="http://localhost/skins/common/feed.css?270"?>
 23: <rss version="2.0" xmlns:dc="http://purl.org/dc/elements/1.1/">
 24: 	<channel>

The problem occurs for the following reason. The timestamp provided to the pg_qurey statement is in TS_UNIX format, not TS_POSTGRES format. It appears there is a serious flaw with the logic visited as the result of the piQueryWatchlist->execute() call. In particular, at line 644 of ApiBase->getParameterFromSettingsm, thje case statement for 'timestamp' takes the TS_POSTGRES formatted timestamp and converts it first to TS_UNIX and then to TS_MW. This value is then supplied to the pg_query. A bit of thought reveals why this problem does not occur for MySQL databases. Timestamps in TS_UNIX format are valid for MySQL, so the fatal error doesn't occur.

The stack trace for the timestamp format conversion is:

org.netbeans.modules.viewmodel.TreeModelNode@24740a93[Name=, displayName=includes/api/ApiBase.php.ApiBase->getParameterFromSettings:652]	
org.netbeans.modules.viewmodel.TreeModelNode@3e624b97[Name=, displayName=includes/api/ApiBase.php.ApiBase->extractRequestParams:484]	
org.netbeans.modules.viewmodel.TreeModelNode@2750c680[Name=, displayName=includes/api/ApiQuery.php.ApiQuery->execute:229]	
org.netbeans.modules.viewmodel.TreeModelNode@281811aa[Name=, displayName=includes/api/ApiMain.php.ApiMain->executeAction:595]	
org.netbeans.modules.viewmodel.TreeModelNode@442fc476[Name=, displayName=includes/api/ApiMain.php.ApiMain->execute:320]	
org.netbeans.modules.viewmodel.TreeModelNode@1c0b41f3[Name=, displayName=includes/api/ApiFeedWatchlist.php.ApiFeedWatchlist->execute:95]	
org.netbeans.modules.viewmodel.TreeModelNode@2716c6e7[Name=, displayName=includes/api/ApiMain.php.ApiMain->executeAction:595]	
org.netbeans.modules.viewmodel.TreeModelNode@485c7bbd[Name=, displayName=includes/api/ApiMain.php.ApiMain->executeActionWithErrorHandling:338]

Value before conversion (TS_POSTGRES format) is:

2011-03-14 21:32:17 GMT

Value after conversion (TS_MW format) is:

20110314213217

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).
Comment 1 Krinkle 2011-03-15 23:07:23 UTC
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
Comment 2 Bryan Tong Minh 2011-03-16 08:16:30 UTC
(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?
Comment 3 Dan Nessett 2011-03-16 17:12:40 UTC
(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.
Comment 4 Dan Nessett 2011-03-22 23:57:45 UTC
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.
Comment 5 Dan Nessett 2011-03-23 00:42:28 UTC
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().
Comment 6 Roan Kattouw 2011-03-23 15:39:17 UTC
(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?
Comment 7 Dan Nessett 2011-03-23 17:03:59 UTC
(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.
Comment 8 Mark A. Hershberger 2011-03-25 18:30:17 UTC
marking as tarball blocker until I can verify that this isn't present there.
Comment 9 Bryan Tong Minh 2011-03-25 19:42:16 UTC
Created attachment 8327 [details]
Database::timestamp fix for rc_timestamp

Please check if the attached patch fixes this issue on Postgres.
Comment 10 Dan Nessett 2011-03-25 20:31:59 UTC
(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.
Comment 11 Bryan Tong Minh 2011-03-25 21:06:17 UTC
Fixed in r84765.
Comment 12 Roan Kattouw 2011-03-27 13:44:15 UTC
(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().
Comment 13 Dan Nessett 2012-01-11 19:09:25 UTC
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.
Comment 14 Sam Reed (reedy) 2012-01-11 19:12:36 UTC
(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
Comment 15 Dan Nessett 2012-01-11 19:46:51 UTC
(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?
Comment 16 Bawolff (Brian Wolff) 2012-01-12 15:22:10 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links