Bryan.TongMinh wrote:

I only took a quick look, but some general comments. Does it really need to duplicate every single function from DatabaseBase? selectSQLText() looks completely copied. I also think there may be unnecessary duplication between the two mssql database classes. Things like SQL generation can be completely shared, only things that call the underlying database interface ought to be different.

What is exactly the advantage between using the two drivers?

Also some specific notes:

-@ini_set( "display_errors", true );
+@ini_set( "display_errors", false );

?

+ # since MSSQL doesn't recognize the infinity keyword, set date manually
+ # TO-DO: refactor for better DB portability and remove magic date
+ $dbw = wfGetDB(DB_MASTER);
+ if($dbw instanceof DatabaseMssqlnative)
+ {
+ return '3000-01-31 00:00:00.000';
+ }

This should be implemented as Database*::infinity()

• GROUP BY tl_namespace, tl_title

+ GROUP BY tl_title, tl_namespace

Why?

a-cpucci wrote:

Hi Bryan,

Thanks for the comments. I've tried to address them below:

"Does it really need to duplicate every single function from DatabaseBase? selectSQLText() looks
completely copied."

Yeah, we definitely want to minimize or remove duplication if it exists. In the method you mentioned, there are actually some slight differences in regards to limits and offsets which is why it was extended in the mssqlnative.php database class. It is something to be aware of though and I will go back and ensure that we aren't needlessly duplicating existing code.

"I also think there may be unnecessary duplication between the two mssql database classes. Things like SQL generation can be completely shared..."

This is absolutely true with one example being the tables.sql file that is basically identicle for the mssql and the mssqlnative drivers. There were basically two reasons we chose to create new files for the native driver even if they were identicle instead of simply including or pointing to the existing mssql version. The first was to maintain the existing Mediawiki file structure in regards each individual driver and the associated files for said driver. The second reason was to maintain separate files in case future changes need to be made that may affect one driver and not the other. I'm open to changing that however if necessary though as part of our goal with submitting this patch is to develop "in the light", so any community or project suggestions are appreciated.

"What is exactly the advantage between using the two drivers?"

There are actually quite a few differences and advantages to using the Native SQL Server driver. First, it is my understanding that community MSSQL driver is no longer officially maintained or supported whereas the Native driver is still in active development, is feature rich and is actively supported and maintained. There are also a few key features that the Native driver provides such as built-in UTF-8 support, (MSSQL driver requires a bridge for UTF-8), and MARS support which means it is compatible with SQL Azure which is a cloud based DB offering.

There are other benefits as well and full information about the Native driver can be found here:

http://www.microsoft.com/downloads/details.aspx?FamilyID=ccdf728b-1ea0-48a8-a84a-5052214caad9&displaylang=en

As for your other notes, I'll look into implementing the database::infinity solution. I've also updated the patch file and removed some unnecessary artifacts from testing/debugging.

Regards,

Chris

a-cpucci wrote:

Updated Native SQL Server Patch

attachment mediawiki115-sqlsrv.patch ignored as obsolete

If the old driver has been discontinued, I would be in favor of dropping support for it in favor of this. IIRC, the current MSSQL class barely even works and has been practically abandoned for some time.

Also, don't use @ for error suppression, use wfSuppressWarnings() and wfRestoreWarnings() before and after the error you wish to suppress.

ayg wrote:

If there are only slight changes to a method like selectSQLText(), you should probably adjust the parent method a little so that the differences are pulled out into separate methods you can override in child classes. Notice how selectSQLText() already calls methods like limitResult() so child classes don't have to duplicate code; it's not meant to be overridden by children.

Also, if your class has some methods identical to DatabaseMssql's, you might want to inherit from it instead of DatabaseBase. It can stay in a separate file, the autoloader will take care of include order. You should definitely share code wherever possible -- don't feel too bound by current conventions as far as file arrangement, since this is the first time we've got support for two different drivers for the same DB. If it makes more sense to use the same schema, say, then do that.

You should consider requesting commit access here:

http://www.mediawiki.org/wiki/Commit_access_requests

Someone who wants to do general-purpose commits normally needs to get a bunch of patches approved first, but if your only aim is to maintain support for a database, you can probably get commit access for that purpose right away, or at least that's been the case sometimes in the past. But it can sometimes take a while before new requests get looked at.

Comments:

diff -rupN /home/chris/Desktop/RemoteDesktop/mediawiki-1.15.1.tar/mediawiki-1.15.1/mediawiki115/config/index.php

Patches should be submitted against SVN trunk so they apply cleanly. See http://www.mediawiki.org/wiki/Subversion. On Windows, your best bet is probably to use TortoiseSVN, check out http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3 someplace, make your changes, right-click and select "Create Patch" (or whatever, haven't used it for years). Failing that, it would help if you at least made your patches against nightly tarballs: http://toolserver.org/~vvv/mw-nightly/

• if( $conf->DBtype == 'mysql' ) {

...
+ switch($conf->DBtype) {
+ case 'mysql':

This is a good place to use a switch, you're right, but it's best if you don't make unrelated changes like this in your patches. They make it harder to review. Especially since you've changed indentation, so all the lines are changed and it's hard to see which ones are actually different.

+ 'SearchMssqlnative' => 'includes/SearchMSSQLNative.php',
...
+ 'DatabaseMssqlnative' => 'includes/db/DatabaseMssqlnative.php',

Probably SearchMssqlNative, DatabaseMssqlNative, DatabaseMssqlNative.php would be more expected capitalizations.

+ # since MSSQL doesn't recognize the infinity keyword, set date manually
+ # TO-DO: refactor for better DB portability and remove magic date
+ $dbw = wfGetDB(DB_MASTER);
+ if($dbw instanceof DatabaseMssqlnative)
+ {
+ return '3000-01-31 00:00:00.000';
+ }

I think the easiest way to handle this is how DatabaseOracle does it:

		if ( preg_match( '/^timestamp.*/i', $col_type ) == 1 && strtolower( $val ) == 'infinity' ) {
			$val = '31-12-2030 12:00:00.000000';
		}

That just has that in DatabaseOracle::insertOneRow(). You might need it in a couple other methods, but this is easier to do than changing all callers, and should be reliable.

I notice you aren't consistently following http://www.mediawiki.org/wiki/Manual:Coding_conventions. In particular, indentation should be one tab (no spaces), and all parentheses should have a space inside (e.g., is_object( $v ) not is_object($v)). You can run stylize.php http://svn.wikimedia.org/viewvc/mediawiki/trunk/tools/code-utils/stylize.php?view=markup on your file to fix a lot of this stuff, or just leave it and someone else can do it before committing.

+class DatabaseMssqlnative extends Database {

You should extend DatabaseBase. Database is a legacy-compat class that's the same as DatabaseMysql.

+ global $wgOut;
+ # Can't get a reference if it hasn't been set yet
+ if ( !isset( $wgOut ) ) {
+ $wgOut = NULL;
+ }

Surely you mean to set $this->mOut here? But you never seem to use $this->mOut, so maybe you should just not set it at all. I'm not sure why you'd want it, anyway.

+ lotsa components seem to think that all databases support limits via LIMIT N after the WHERE clause
+ well, MSSQL uses SELECT TOP N, so instead of going and rewriting all of those extensions we attempt
+ to do a more roboust mapping here
+ if (strpos($sql,'LIMIT ') > 0) {
+ massage LIMIT -> TopN
+ $sql = $this->LimitToTopN($sql) ;
+ }

This is wrong -- you should use limitResult() instead. Callers that use LIMIT directly should be changed to use limitResult(). LIMIT also breaks for Oracle and non-native Mssql.

+ MSSQL doesn't have EXTRACT(epoch FROM XXX)
+ if (strpos($sql, "EXTRACT(epoch FROM ") > 0) {
+ This is same as UNIX_TIMESTAMP, we need to calc # of seconds from 1970
+ $sql = str_replace("EXTRACT(epoch FROM ", "DATEDIFF(s,CONVERT(datetime,'1/1/1970'),", $sql);
+ }

This is currently only called in two places, includes/specials/SpecialAncientpages.php and includes/specials/SpecialUnusedimages.php. Both of those use switch ( $wgDBType ) and feed totally different results to different DBs. This is bad, we should add a new method for this, but for now you should just update those two callers (unless you want to add a method that abstracts this logic).

+ echo $message;

Why are you doing this? Just throw the exception like all the other DBs. That will print out the error message along with a stack trace by default.

+ function select( $table, $vars, $conds='', $fname = 'Database::select', $options = array(), $join_conds = array() )
...
+ function selectSQLText( $table, $vars, $conds='', $fname = 'Database::select', $options = array(), $join_conds = array() ) {

You shouldn't need to override either of these. If there are differences, please adjust the parent method to account for them, and add an extra method, the way limitResult() is currently used.

+ function useIndexClause( $index ) {
+ return '';
+ }

This is the same as DatabaseBase, you don't need to override it. It's basically a MySQL-specific hack.

+ function lowPriorityOption() {
+ return '';
+ }

Ditto.

+ } else {
+ return preg_replace('/SELECT(\s*DISTINCT)?/Dsi', 'SELECT$1 TOP '.$limit, $sql);
+ }
+ } else {
+ $sql = preg_replace('/SELECT(\s*DISTINCT)?/Dsi', 'SELECT$1 TOP(10000000) ', $sql);

You probably want to only replace the first occurrence. Otherwise this will result in really weird bugs when you have a user whose name contains "select" or something.

+ // this massages a LIMIT OFFSET,COUNT clause into a TOP N clause
+ function LimitToTopN($sql) {

Why is this needed? Callers should not be concatenating LIMIT onto anything. If they are, the callers should be fixed, the database layer shouldn't be trying to parse SQL queries using explode() -- that's unnecessarily difficult and will surely cause bugs.

+ function limitResultForUpdate($sql, $num) {
+ return $sql;
+ }

I guess this isn't implemented? Probably not a big deal. You should note in a comment if MSSQL doesn't support this.

+ function conditional( $cond, $trueVal, $falseVal ) {
+ return " (CASE WHEN $cond THEN $trueVal ELSE $falseVal END) ";
+ }

This is also identical to the parent class.

I'm not familiar with SearchEngine and didn't review SearchMssqlnative.

• } -
• $res = $dbr->query( $sql, METHOD );

• if( $res->numRows() == 0 )

+ $res = $dbr->query($sql, METHOD );
+
+ if( $res->numRows() == 0 )

			$this->mResultEmpty = true;

+
+ }

I don't understand what this is meant to do.

a-cpucci wrote:

Aryeh,

Thanks for the comments and thorough review. I will work on your suggestions and update when complete.

A couple comments on search:

+ $namespaces = implode(',', $this->namespaces);

You should use DatabaseBase::makeList() for that.

+ "FROM $page,FREETEXTTABLE($searchindex , $match, LANGUAGE 'English') as ftindex " .

This is pretty i10n-unfriendly. If MSSQL doesn't support universal Unicode collation, you should use $wgContLang to determine the correct collation.

ryan wrote:

DatabaseMssqlNative and supporting files

I've taken the recommendations and implemented them as best I could against the current code base (r.65560). Some of the issues still exist, but the inline comments reflect why some portions are still not quite in line with the suggestions.

A few notes:

DatabaseMssqlNative::select() and DatabaseMssqlNative::selectSQLText()
AryehGregor says that I shouldn't need to override either of these, and I agree that the old patch was horridly verbose and duplicated the logic several times. Instead of following the suggestion, which would have caused my patch to affect all of the Database* classes that are currently working, I took a cue from MaxSem's suggestion in b#23330 and re-wrote the methods from the original patch to inherit the logic from the parent and either adjust the setup beforehand or the output afterhand where appropriate. I think this helps isolate the change-set and uses inheritance much better than the patch did previously

DatabaseMssqlNative::LimitToTopN()
I agree that this should not be needed, and that its implementation was less than ideal, duplicating logic and doing a lot of extra work, but because we still can't depend on extensions to use the SQL-generation functions, I think this functionality is still needed as a last resort. I rewrote the function to parse a LIMIT clause, remove it, and then pass the resulting parameters to the existing DatabaseMssqlNative::limitResult() method, which I feel is a much cleaner and lighter-weight way of implementing a necessary evil.

DatabaseMssqlNative::doQuery() - Epoch magic.
I don't think this is the optimal solution, but since we can't rely on extensions to not use this, I'm leaving it in place. The two known caller functions have been updated to make the right calls.

This patch is against TRUNK rev 65560 and for feedback only. I'll be spending the next few days going through and doing extensive bug-testing and will do my best to implement your suggestions before committing to trunk.

I appreciate your feedback.

-yaauie

attachment mw.mssqlnative.svn65560.diff ignored as obsolete

Few comments on patch:

• Other Database*.php files list their main database classes first, you should do it too.

$sql = str_replace("EXTRACT(epoch FROM ", "DATEDIFF(s,CONVERT(datetime,'1/1/1970'),", $sql);

This is unreliable, as it doesn't take into account possible variations in spacing and casing. Use regex?

class result_mssqlnative {

This name doesn't follow our naming conventions.

wfDebug("SQL: [$sql]<br/>\n");

Debug log is in plaintext, tags make no sense here.

if ( preg_match( '/\bLIMIT\s*/i', $sql ) )  > 0) {

Ideally, extensions not using limitResult() should be fixed instead.

            $message = "A database error has occurred<br/>\n" .
	                "Query: $sql<br/>\n" .

You're mixing tags with unescaped data - are you sure that there is no XSS or tag breakage by overescaping?

if ((strpos(" ".$sql, "SELECT") > 0)

Use strict comparison (===) with FASLE instead, it will be more readable.

$bAllOk

We don't use Hungarian notation.

1. Copyright (C) 2004 Brion Vibber <brion@pobox.com>

Not anymore :)

"FROM $page,FREETEXTTABLE($searchindex , $match, LANGUAGE 'English') as ftindex " .

Use $wgContLang to determine the correct language instead of assuming that it is always English.

• Our coding conventions (http://www.mediawiki.org/wiki/Manual:Coding_conventions) aren't strictly followed, use stylize.php from trunk/tools/code-utils/ to at least fix spacing problems.

ayg wrote:

Sorry for the long delay, I got caught up in finals and then procrastinated. :( I should be able to review any further revisions quickly, now that the summer has started. I agree with most of what Max said. No comments on the Installer, search-related stuff, or the schema.

As for the rest, generally there are lots of places where you don't follow MediaWiki style. Most of these should be fixable by running stylize.php. I've noted some places that will have to be fixed manually, but omitted a bunch of inconsequential stylistic issues that can be easily fixed after commit, since the patch is already so large.

+ # BEGIN DatabaseMssqlNative hack
+ # Since MSSQL doesn't recognize the infinity keyword, set date manually.
+ # TO-DO: Refactor for better DB portability and remove magic date
+ $dbw = wfGetDB(DB_MASTER);
+ if( $dbw instanceof DatabaseMssqlNative ) {
+ return '3000-01-31 00:00:00.000';
+ }
+ # End DatabaseMssqlNative hack

This is indented one tab too much.

+class result_mssqlnative {
+
+ public function result_mssqlnative($queryresult=false) {
+ $this->mCursor = 0;
+ $this->mRows = array();
+ $this->mNumFields = sqlsrv_num_fields( $queryresult);

The name of this class and most of its methods don't follow MediaWiki naming conventions. I'd use Result_MssqlNative here, and arrayToObj() etc. for the methods.

Also, indentation here is weird. Should be:

class result_mssqlnative {

public function result_mssqlnative($queryresult=false) {

		$this->mCursor = 0;
		$this->mRows = array();
		$this->mNumFields = sqlsrv_num_fields( $queryresult);

Indentation is weird in a bunch of other places too, and stylize.php can't fix all of this. Try switching your tab stops to 8 briefly so you can spot it easily, or configure your development environment to flag indentation with spaces. Your new patch is really hard to read at my browser's tab settings, and the last patch wasn't.

+ if (!strlen($user)) { ## e.g. the class is being loaded
+ return;
+ }

More explanation here, please -- why would any caller call open() with an empty $user, particularly when it will just return immediately?

+ ($ntAuthPassTest == 'ntauth' && $ntAuthUserTest == 'ntauth') ? $this->mConn = @sqlsrv_connect($server, array('Database'=>$dbName)) : $this->mConn = @sqlsrv_connect($server, array('Database'=>$dbName,'UID'=>$user,'PWD'=>$password));

Use if/else here, please, on separate lines.

+ several extensions seem to think that all databases support limits via LIMIT N after the WHERE clause
+ well, MSSQL uses SELECT TOP N, so to catch any of those extensions we'll do a quick check for a LIMIT
+ clause and pass $sql through $this->LimitToTopN() which parses the limit clause and passes the result to
+ $this->limitResult();
+ if ( preg_match( '/\bLIMIT\s*/i', $sql ) ) > 0) {

This is evil, but I'm not sure there's a better solution. :( Maybe make your check a little more careful, though -- this might cause all sorts of crazy breakage if you do SELECT * FROM user WHERE user_name='LIMIT' or something.

+ MSSQL doesn't have EXTRACT(epoch FROM XXX)
+ if (strpos($sql, "EXTRACT(epoch FROM ") > 0) {
+ This is same as UNIX_TIMESTAMP, we need to calc # of seconds from 1970
+ $sql = str_replace("EXTRACT(epoch FROM ", "DATEDIFF(s,CONVERT(datetime,'1/1/1970'),", $sql);
+ }

I think you can drop this code. I wouldn't worry about extensions using EXTRACT() unless you know of some that actually do (and which can't be easily changed). First of all, it's not a common need. And second of all, EXTRACT() doesn't work for anything other than PostgreSQL anyway AFAICT, so it's unlikely that anything would be hardcoding it, except for those two cases in core (which probably started by splitting MySQL to its own case then pgsql to the default case, and other DBs added to their own cases).

+ $message = "A database error has occurred<br/>\n" .
+ "Query: $sql<br/>\n" .
+ "Function: ".__FUNCTION__."<br/>\n";
+ // process each error (our driver will give us an array of errors unlike other providers)
+ foreach( sqlsrv_errors() as $error) {
+ $message .= $message."ERROR[".$error['code']."] ".$error['message']."<br/>";
+ }
+
+ throw new DBUnexpectedError($this, $message);

Does this actually work as expected? It looks like DBUnexpectedError takes a plaintext string for $message, not HTML, so you shouldn't be using <br /> and such. nl2br( htmlspecialchars() ) will be run for HTML output.

Also, if this did accept HTML output, you'd want to htmlspecialchars() on $error['code'] and $error['message'] before outputting.

+ if ((strpos(" ".$sql, "SELECT") > 0) ||
+ ((strpos(" ".$sql, "INSERT") > 0) && (strpos(" ".$sql, 'OUTPUT INSERTED') > 0)))

Use strpos( $haystack, $needle ) !== false instead of strpos( " $haystack", $needle ) > 0.

+ if ( !(isset( $arrToInsert[0] ) && is_array( $arrToInsert[0] )) ) {Not multi row
+ $arrToInsert = array(0=>$arrToInsert);make everything multi row compatible
+ }

Maybe this would work:

$arrToInsert = (array)$arrToInsert;

+ $bAllOk = true;

As Max says, use $allOk here.

+ $pregIgnoreIdentity = '/recentchanges|webauth_user|site_stats|watchlist|user_groups|externallinks|image|pagelinks|categorylinks|templatelinks/i';
...

				// we want to figure out the identity column, so we examine each column name in the array

+ foreach($a as $k => $v) {
+ // all mediawiki columns (except for cl_from) are in format of XX_id and XX_XXX_id

Ouch. There's really no better way to do this?

+ $sql = preg_replace('/\bSELECT(\s*DISTINCT)?\b/Dsi', 'SELECT$1 TOP(10000000) ', $sql,1);

I don't get what this line does.

+ function deadlockLoop() {

This appears to be copied line-by-line in its entirety from DatabaseBase, with only these lines changed:

+ $this->rollback($myFname);
...
+ $this->commit($myFname);

Just change those two lines in DatabaseBase, please, instead of copying the function. Your new versions should work there too.

+ function addQuotes( $s ) {
+ if ( is_null( $s ) ) {
+ return 'NULL';
+ } else if ($s instanceof Blob) {
+ return "'".$s->fetch($s)."'";
+ }
+ return "'" . $this->strencode( $s ) . "'";
+ }

Better for clarity to do:

function addQuotes( $s ) {

		if ($s instanceof Blob) {
			return "'".$s->fetch($s)."'";
		} else {
			return parent::addQuotes( $s );
		}

}

+ function getDBname() {
+ return $this->mDBname;
+ }
+
+ function getServer() {
+ return $this->mServer;
+ }

These seem to be duplicates of the parent methods and could be dropped.

(In reply to comment #10)

+ MSSQL doesn't have EXTRACT(epoch FROM XXX)
+ if (strpos($sql, "EXTRACT(epoch FROM ") > 0) {
+ This is same as UNIX_TIMESTAMP, we need to calc # of seconds
from 1970
+ $sql = str_replace("EXTRACT(epoch FROM ",
"DATEDIFF(s,CONVERT(datetime,'1/1/1970'),", $sql);
+ }

I think you can drop this code. I wouldn't worry about extensions using
EXTRACT() unless you know of some that actually do (and which can't be easily
changed). First of all, it's not a common need. And second of all, EXTRACT()
doesn't work for anything other than PostgreSQL anyway AFAICT, so it's unlikely
that anything would be hardcoding it, except for those two cases in core (which
probably started by splitting MySQL to its own case then pgsql to the default
case, and other DBs added to their own cases).

Note that a number of QueryPage subclasses use EXTRACT(). In the querypage-work2 branch, I've refactored this so it no longer uses EXTRACT(), but the point is we have code using EXTRACT() in core right now.

ayg wrote:

(In reply to comment #11)

Note that a number of QueryPage subclasses use EXTRACT(). In the
querypage-work2 branch, I've refactored this so it no longer uses EXTRACT(),
but the point is we have code using EXTRACT() in core right now.

Only two, AFAICT, Ancientpages and Unusedimages, and those are fixed in the patch. Are there more? I don't see them.

Another issue: I haven't tried, but after reading MSDN I think that Article::incViewCount() wil not work on MSSQL, so this should be also eventually fixed.

ryan wrote:

DatabaseMssqlNative

The suggestions have been implemented and the code has been tested on MS SQL Server 2008 with Fulltext, as well as on MySQL 5.1 to ensure it doesn't break anything. There are two small changes to DatabaseBase (suggested by Aryeh Gregor), which cause ::deadlockLoop to call ::commit and ::rollback instead of executing SQL directly.

The patch is based on today's r70030.

RE: Aryeh Gregor "why would any caller call open() with an empty $user"
::open() is called when the class is instantiated. Is there anywhere I'm not finding that instantiates a database for any purpose without credentials?

RE: Aryeh Gregor "This is evil, but I'm not sure there's a better solution."
+ several extensions seem to think that all databases support limits via LIMIT N after the WHERE clause
+ well, MSSQL uses SELECT TOP N, so to catch any of those extensions we'll do a quick check for a LIMIT
+ clause and pass $sql through $this->LimitToTopN() which parses the limit clause and passes the result to
+ $this->limitResult();
+ if ( preg_match( '/\bLIMIT\s*/i', $sql ) ) > 0) {

This LIMIT check is intentionally lax, since LimitToTopN() has a much more stringent parsing and stripping of the LIMIT/OFFSET clause and will return the SQL untouched if it doesn't match.

RE: Aryeh Gregor "Ouch. There's really no better way to do this?" in ::insert()
Yes, there is absolutely a better way of doing this, and I believe I have implemented it this time around, bu asking the database for the $table's IDENTITY column and handling it appropriately. The old way was inherited and worked, so I didn't mess with it until you brought it up, but yes, definitely ugly.

On another note: Microsoft would like to see this mainlined (as would I :) ), and I realize that there will need to be some documentation on MediaWiki.org showing how to set up MS SQL Server, revealing the ntauth scenario etc., but don't want to cause confusion by people accidentally accessing these articles until it is released. How is documentation on future features typically handled?

-yaauie

attachment MediaWiki.ProjectWillow.from70030.diff ignored as obsolete

How is documentation on future features typically handled?

We tag such pages, e.g. http://www.mediawiki.org/wiki/Manual:$wgAllowImageTag

You've also got a lot of whitespace changes in there, that make the diff a little cluttered..

As for "mainline" support..

As long as it's maintained, and it works, having it in core shouldn't be too much of an issue.. Though, the "3rd party" drive might be a bit of an issue

You will of course need to probably do some work for the new installer ;)

Note that we're going to ditch the current installer pretty soon, so it's worth concentrating on new-installer instead to avoid a double effort.

-if( !defined( 'MEDIAWIKI_INSTALL' ) ) {
+if ( !defined( 'MEDIAWIKI_INSTALL' ) ) {

Your patch has lots of whitespace-only changes, this makes reviewing MUCH harder. Committing changes of such scale combimed with reformatting is completely out of the question.

• ## DB2 specific:

+ # # DB2 specific:

This is even weirder, did you use an automated source formatter? If so, it should be fixed not to screw things up.

+ else if ( $conf->DBtype == 'mssqlnative' ) {
+ error_reporting( E_ALL );

Why do you clear E_STRICT here?

+ # BEGIN DatabaseMssqlNative hack
+ # Since MSSQL doesn't recognize the infinity keyword, set date manually.
+ # TO-DO: Refactor for better DB portability and remove magic date
+ $dbw = wfGetDB( DB_MASTER );
+ if ( $dbw instanceof DatabaseMssqlNative ) {

1. Don't connect to master just to find out your DB type. DB_SLAVE is more than sufficient.
2. Use getType() instead of instanceof

+ $res = $this->doQuery( "SELECT NAME AS idColumn FROM SYS.IDENTITY_COLUMNS WHERE OBJECT_NAME(OBJECT_ID)='{$tableRaw}'" );

Needs to be cached.

You don't need to reimplement stuff such as DatabaseBase::ping() and getLag().

ryan wrote:

RE: Whitespace
Hm. It looks like those are artifacts of running it through stylize.php. I'll create a new diff.

Bryan.TongMinh wrote:

I suggest that Ryan is given commit access, once all comments have been addressed. I would not want to have Yet Another Database Driver that is will be checked in and then be forgotten about and removed in some later release.

(In reply to comment #17)

This is even weirder, did you use an automated source formatter? If so, it
should be fixed not to screw things up.

stylize.php fixed in r70050.

ryan wrote:

DatabaseMssqlNative

I've gone through and reverted the whitespace changes caused by stylize.php

Attached:

(In reply to comment #21)

I've gone through and reverted the whitespace changes caused by stylize.php

Thanks, looks much clearer now.

ayg wrote:

(In reply to comment #14)

RE: Aryeh Gregor "why would any caller call open() with an empty $user"
::open() is called when the class is instantiated. Is there anywhere I'm not
finding that instantiates a database for any purpose without credentials?

Wouldn't it be kind of pointless? The MySQL code doesn't seem to have this check. I don't see why it's needed. But it probably doesn't hurt anything.

This LIMIT check is intentionally lax, since LimitToTopN() has a much more
stringent parsing and stripping of the LIMIT/OFFSET clause and will return the
SQL untouched if it doesn't match.

Okay.

On another note: Microsoft would like to see this mainlined (as would I :) )

At this point it seems like it should be okay to commit. The bar isn't so high for a new database driver, as long as it doesn't break anything else. It's easier to fix up the issues once it's committed anyway. Did you say you had commit access, or would you like someone else to commit it?

and I realize that there will need to be some documentation on MediaWiki.org
showing how to set up MS SQL Server, revealing the ntauth scenario etc., but
don't want to cause confusion by people accidentally accessing these articles
until it is released. How is documentation on future features typically
handled?

There are templates that say what version things are available from. Documentation is usually added well before releases, when it's added at all.

christian wrote:

Add myself to watch list.

*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*

DJ, can you take a peek at this and see if the old patches here need keeping or are overruled by the other stuff you've been working on?

dj.bauch wrote:

(In reply to comment #26)

DJ, can you take a peek at this and see if the old patches here need keeping or
are overruled by the other stuff you've been working on?

Yes. I've been working with the PDO variation of the Microsoft native driver, but otherwise the goal is the same. I've had everything working up through version 1.16.5. Additionally, 1.17, 1.18, and 1.19alpha are all mostly working. Still some kind of problem with objectcache for some things I try to store and some issues with some non US English characters, but I will get that ironed out. My initial impression is that this patch makes heavier use of NVARCHAR vice VARCHAR than I have, and there are not a bunch of views -- so perhaps there is a better approach here to handling the conversion of date/time values that will be more in line with the way that you and Tim Starling have both taken up with me. Oh, and the new installer needs to be brute-forced into allowing additional databases other than the ones that are already there -- the _CompiledDBs setting is forgetful. Max has seen this problem demonstrated to him at the hackathon, so I think this will be addressed as well.

(In reply to comment #27)

Oh, and the new installer needs to be brute-forced into
allowing additional databases other than the ones that are already there -- the
_CompiledDBs setting is forgetful. Max has seen this problem demonstrated to
him at the hackathon, so I think this will be addressed as well.

Reverting r95023 locally on your machine seemed to fix this - just commit it with your other changes.

sumanah wrote:

Ryan Biesemeyer, thank you for your patch. Adding the "reviewed" keyword here because it looks like DJ and others have reviewed it and DJ will follow your approach in his work -- DJ, can you point to that so Ryan can follow and collaborate with you?

We've never had solid support and we've effectively been completely broken since the new installer landed and we didn't include Mssql support there.

I'm removing the broken Mssql support from core in gerrit change 95651.

Considering Mssql support for MediaWiki is a story of cookie licking and broken promises, I'm being bold and WONTFIXing this for good.

Reverted and reopened. I did work this past summer on supporting SQL Server and MS has asked for this as well, so it isn't just cookie-licking.

Is there anyone willing to actually develop MSSQL support? Not just commit a patch and run away, but to be a caring maintainer that continuously keeps the thing up to date? So far there were several attempts, but all of them were former, not latter. Until a proof of long-term commitments can be demonstrated, I agree with Chad.

I agree that we need a maintainer. That we don't currently have one does not make this bug WONTFIX -- it makes it a valid, open bug.

Meh, that's what LATER was. However, I seriously disagree with your revert of the removal - we shouldn't have deadbeef in our code base.

Again: WONTFIX means "actively against fixing the issue". If you're not against MSSQL support, this is not a WONTFIX. I'd recommend to exclude lowest priority tickets in your search queries instead.

installer and updater support was added with gerrit 105138