brion@svn.wikimedia.org wrote:
Revision: 35867 Author: brion Date: 2008-06-04 16:56:31 +0000 (Wed, 04 Jun 2008)
Log Message:
Revert r35857, 35858, 35859 for the moment. Some notes:
- Calling $user->isActiveUser() looks a bit redundant. :) Consider either shortening it to isActive() or changing the last word to be clearer, say $user->isActiveEditor()
- $wgActiveUserEditcount <- consider fully casing here; "editcount" isn't a word. :D -> $wgActiveUserEditCount
- $oldTime is dumped into SQL without any escaping or quoting. This will happen to work on MySQL but will fail on PostgreSQL, and is generally a bad practice. Use addQuotes() to ensure the formatted timestamp string is escaped and quoted for your raw SQL construct
There's also the slight problem that any attempt to use this User::isActiveUser() for the bugs referenced (13225 and 13585) would cause instant death of the site due to DB overload.
- the database result object is not freed, which will leak resources if used in a long-running script
It turns out that was just a misunderstanding on the part of the people who wrote the mysql extension for PHP. There is no need to free the underlying data for resource objects manually. The PHP manual is incorrect and mysql_free_result() is useless.
Underlying data for resources is deleted when the reference count for the resource goes to zero. This means assigning the variable to something else, or letting it go out of scope, works well enough.
You can easily test this by comparing the memory usage of this:
for ($i=0; $i<100000; $i++) { $res = $dbr->query('select * from revision limit 1000'); }
with this:
$res = array(); for ( $i=0; $i<100000; $i++) { $res[] = $dbr->query('select * from revision limit 1000'); }
The former uses virtually no memory, the latter uses lots.
-- Tim Starling
On Thu, Jun 5, 2008 at 12:58 AM, Tim Starling tstarling@wikimedia.org wrote:
brion@svn.wikimedia.org wrote:
Revision: 35867 Author: brion Date: 2008-06-04 16:56:31 +0000 (Wed, 04 Jun 2008)
Log Message:
Revert r35857, 35858, 35859 for the moment. Some notes:
- Calling $user->isActiveUser() looks a bit redundant. :) Consider either shortening it to isActive() or changing the last word to be clearer, say $user->isActiveEditor()
- $wgActiveUserEditcount <- consider fully casing here; "editcount" isn't a word. :D -> $wgActiveUserEditCount
- $oldTime is dumped into SQL without any escaping or quoting. This will happen to work on MySQL but will fail on PostgreSQL, and is generally a bad practice. Use addQuotes() to ensure the formatted timestamp string is escaped and quoted for your raw SQL construct
There's also the slight problem that any attempt to use this User::isActiveUser() for the bugs referenced (13225 and 13585) would cause instant death of the site due to DB overload.
- the database result object is not freed, which will leak resources if used in a long-running script
It turns out that was just a misunderstanding on the part of the people who wrote the mysql extension for PHP. There is no need to free the underlying data for resource objects manually. The PHP manual is incorrect and mysql_free_result() is useless.
Underlying data for resources is deleted when the reference count for the resource goes to zero. This means assigning the variable to something else, or letting it go out of scope, works well enough.
You can easily test this by comparing the memory usage of this:
for ($i=0; $i<100000; $i++) { $res = $dbr->query('select * from revision limit 1000'); }
with this:
$res = array(); for ( $i=0; $i<100000; $i++) { $res[] = $dbr->query('select * from revision limit 1000'); }
The former uses virtually no memory, the latter uses lots.
-- Tim Starling
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
As discussed on IRC last night, this method (later recommitted with Brion's suggestions as User::isActiveEditor(), less ambiguous name), is not designed to directly fix 13225 and 13585. Running it over all 7.5 million users on enwiki would kill it dead, you're right.
It was moreso intended to be a utility function, used to determine if a user is active for non-batch options (ie: on a single user basis it's ok, but god forbid someone foreach() using it). For example with RecentChanges, it opens the possibility of adding a new column similar to rc_bot (called rc_activeeditor, name's unimportant) that can be filled when the edit is made. With this, you could easily solve bug 13225.
Just some thoughts.
-Chad
Tim Starling wrote:
It turns out that was just a misunderstanding on the part of the people who wrote the mysql extension for PHP. There is no need to free the underlying data for resource objects manually. The PHP manual is incorrect and mysql_free_result() is useless.
Thanks for sharing this. That function struck me as too Cish for the php way, but if the fine manual sais it is needed, we all use it :) Perhaps you should note it at http://php.net/mysql_free_result ?
wikitech-l@lists.wikimedia.org