[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Page MenuHomePhabricator

Provide capability for a DB to be added in an extension
Closed, DeclinedPublic

Description

The supported DBTypes are hard-coded into a protected static array in the Installer class.

I'm working on a DB backend for Percona and I need a way to make it available to a user of the web installer.

The CLI-based installer works since the user can just say "percona" and MediaWiki will find the provided DatabasePercona class. The web installer, though, provides a list of possible backends that has been hard-coded in.

Optional:

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This comment by @Anomie is related here T191231#4998548 but I'm not sure because Installer can't find any extension since there's no LocalSettings.php to load any extension (yet). So how that can be done?

So how that can be done?

Right, when using the built-in installer to install MediaWiki, the only possible way to extend the auto-loader so that MediaWiki can find the code to use other databases is by using Composer's autoloader.

I know a decision was made in the past (T467, which @Anomie .mentions in gerrit) that MW wouldn't rely on composer to load extension code, so that means (from what I can see) we have three options:

  1. revisit the decision made in T467 in light of new information and new plans;
  2. write the DB support as a composer package, not as an extension; or
  3. write new code for MW that will allow extensions to provide capabilities for non-MySQL databases.

It is apparent that a lot of extension developers are not aware of T467 or are ignoring it entirely. I don't think we should double-down on the dogma therein.

This comment by @Anomie is related here T191231#4998548 but I'm not sure because Installer can't find any extension since there's no LocalSettings.php to load any extension (yet). So how that can be done?

Installer::findExtensions(), same as it does during the "Options" step to find skins and extensions to offer to enable. This is really just an "offer to enable" at an earlier step.

It could do ->findExtensions() and then filter for extensions that provide a DB implementation, or we could make a new class of "database extensions" with their own subdirectory. Personally I'd lean towards the former, but that can be bikeshedded later.

Change 519681 abandoned by markahershberger:
Provide a way to add different DB types to Installer

https://gerrit.wikimedia.org/r/519681

CCicalese_WMF renamed this task from Provide a Installer::addDBType() so DB types can be added via an extension to Provide capability for a DB to be added in an extension.Feb 21 2020, 4:45 PM

@Ladsgroup were you able to set this up using Percona from the web installer? I was able to do it through cli (with the --dbtype percona flag), but later ran into similar issues when trying to update the tables after installing the Semantic MediaWiki extension: https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4724

I've tried adding the PercondaDB extension ("mediawiki/percona-db": "~1") to both the require as well as autoload sections of the composer.local.json after setting the MW_INSTALL_PATH. Both of these didn't seem to work even though I was able to select PerconaDB extension in the Extensions:Options page of the web installer. Would love to know if you or anyone can provide some suggestions / guide me on how to work around this.

Specifically, with the above steps, when I run composer update, I see the following error:

Could not insert main page: A database query error has occurred. Did you forget to run your application's database schema updater after upgrading? Query: SELECT GET_LOCK(<MY_HASH>) AS lockstatus Function: MediaWiki\Storage\PageEditStash::getAndWaitForStashValue Error: 1105 Percona-XtraDB-Cluster prohibits use of GET_LOCK with pxc_strict_mode = ENFORCING (<MY_HOST>:<MY_PORT>)

Apologies in advance if this should be a question in the forums (etc) instead of here; still quite new to this community. I will add / update / delete this accordingly based on feedback I receive!

PerconaDB should override locks. Granted, I haven't tried installing into an empty Percona DB using this extension.

Could you file a new bug @Nnaka1 ?

The bug seems very much with the library and how it's being implemented/installed. And seems to demonstrate why T249573: Remove support for extensions requiring a MediaWiki version via Composer and T250406: RFC: Hybrid extension management need to happen

This weird installation into extension and autoloading/enabling/whatever is very much not supported by MediaWiki, nor is it suggested, encouraged or otherwise.

The bug seems very much with the library and how it's being implemented/installed. And seems to demonstrate why T249573: Remove support for extensions requiring a MediaWiki version via Composer and T250406: RFC: Hybrid extension management need to happen

It would be helpful if you could provide direction here.

How should a database driver be implemented for MW so that it can be used during installation? Is this something that should only be done in core MW?

AFAICS, the only way to provide a DB driver outside of core is to use composer's autoloader since it works with the installer.

Well, surely that's the point of this task? To formally define a way this could/should work and implement it as such?

Change 604477 had a related patch set uploaded (by Art-Baltai; owner: Art-Baltai):
[mediawiki/core@master] Add a new type of database to the installer from extension

https://gerrit.wikimedia.org/r/604477

Change 605931 had a related patch set uploaded (by Peter.ovchyn; owner: Art-Baltai):
[mediawiki/core@master] Add a new type of database to the installer from extension

https://gerrit.wikimedia.org/r/605931

Change 605931 abandoned by Peter.ovchyn:
Add a new type of database to the installer from extension

https://gerrit.wikimedia.org/r/605931

Change 604477 merged by jenkins-bot:
[mediawiki/core@master] Add a new type of database to the installer from extension

https://gerrit.wikimedia.org/r/604477

CCicalese_WMF reopened this task as Open.
CCicalese_WMF updated the task description. (Show Details)
CCicalese_WMF subscribed.

Meant to close the implementation subtask. This will remain open pending documentation.

Change 608752 had a related patch set uploaded (by Cicalese; owner: Cicalese):
[mediawiki/core@master] Add database extension to LocalSettings.php.

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608752

Change 608752 merged by jenkins-bot:
[mediawiki/core@master] Add database extension to LocalSettings.php.

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /608752

Change 609231 had a related patch set uploaded (by Cicalese; owner: Cicalese):
[mediawiki/core@master] Fix parameter name / variable name clash.

https://gerrit.wikimedia.org/r/c/mediawiki/core/ /609231

Change 609231 merged by jenkins-bot:
[mediawiki/core@master] Fix parameter name / variable name clash.

https://gerrit.wikimedia.org/r/609231

Change 615686 had a related patch set uploaded (by Legoktm; owner: Legoktm):
[mediawiki/core@master] [WIP] Allow extensions to add database types

https://gerrit.wikimedia.org/r/615686

Is this a 1.35 release blocker or can it land in 1.36? I have a basically 75% complete patch series in Gerrit right now, it'll take another day or two for me to finish it..

Notable features/improvements:

  • Using existing extension.json infrastructure, especially the caching and parsing
  • Use dependency injection into Database
  • Don't execute any PHP code from the extension until the user has selected it

I do have some concerns though, namely that I don't believe DatabaseInstaller and DatabaseUpdater have been reviewed for subclassing in extensions, and whether the API they provide right now is appropriate, especially given the abstract schema work.

Yes, this is a blocker for the 1.35 release. It is intended to allow support for the databases removed from core last year (Oracle and Microsoft SQL Server) to be reintroduced as extensions. The maintainers for that database support do so on LTS releases.

Sorry for not responding sooner; I'm out on vacation this week. Thank you for working on this.

@Krinkle and I discussed this on IRC today (and a bit in Gerrit). I think both of us are concerned with trying to backport such a large feature into REL1_35 this late. Back in 2015 I thought this project had a large enough scope that it was RfC-worthy. From my perspecitve I think there are some unresolved questions/issues, like what extensions should do if they encounter a non-supported db type (some throw an exception, others will assume it's MySQL-compatible, probably causing other failures). How do database extension developers support extensions that don't want to be compatible and carry their schemas (see $wgResourceModuleSkinStyles which allowed non-core skins to support extensions without requiring extension changes). I also think we need more insight on how this will integrate with the abstract schema project, when a lot of these concerns will go away (though this may be needed before that project is completed).

Krinkle pointed out that currently it's possible to have a custom database type as long as the class is appropriately named 'Database' . ucfirst( $type ). The missing part (this task) is that it's not possible to bootstrap a new MediaWiki instance using the installer (web and install.php) and that custom type, nor is it possible to update that instance via update.php (or its web counterpart).

There are 3 main integrations needed:

  • Database::factory() - already supports $class = 'Database' . ucfirst( $dbType );
  • Installer::getDBInstallerClass() - already supports ucfirst( $type ) . 'Installer'; BUT there's no way to load an extension before the database selection process
  • DatabaseUpdater::newForDB() - supports $class = ucfirst( $type ) . 'Updater'; ONLY IF the type is in Installer::getDBTypes(), which is the hardcoded core list.

Properly fixing all of these 3 points is non-trivial, e.g. my DatabaseFactory patch has significant architectural impacts given how deep it reaches into LBFactory/LoadBalancer. It's also pretty hot code given the significant performance regression we saw, so modifications to it need to be done carefully. We probably need to add a DatabaseUpdaterFactory as well. I'm likely willing to do the work/help but I don't really want to put together something that's half-baked.

I also think that no one wants to re-add Oracle/MSSQL support to core just for 1.35 given that we ultimately want to get rid of it (I think it was a mistake to remove the support without already having its replacement in place, as its now forcing us to compromise on deadline). Also I suspect core changes have already been made that it would be a significant amount of work for very little benefit. We do want to continue supporting existing installs, so that's what I propose we do.

Proposal: If we remove the check in DatabaseUpdater for the dbtype being one of the hardcoded ones and just check if the class exists, then I believe it should be possible for an extension to provide an Oracle or MSSQL database type for existing installs to upgrade. New installs would not be possible without manual bootstrapping (creating the database by hand and writing your own LocalSettings.php). The database extension feature will be targeted for 1.36 instead.

I think that's a reasonable compromise in that we can theoretically still support existing installs but we're not rushing to introduce this feature without being able to fully consider all the architectural implications. Thoughts?

Hey @Legoktm and @Krinkle, thanks for the write up. For now, we're going to leave this reverted and likely reschedule it for a later point.

We're also going to retro on it this Thursday 30th at 2100 UTC. @Krinkle you should be able to see this on our team calendar, @Legoktm feel free to reach out to myself or @AMooney for an invite.

Otherwise, we'll also have a team office hours on August 6th at 1700 UTC - on irc and hangouts - feel free to drop in there as well to discuss further(again reach out for an invite or more details).

Hi @roman-stolar, are you planning to work on this? I am not even sure this is something we want to do anymore, given the radio silence since I asked questions about how it should work functionally.

And then last week when I brought up this task, @tstarling said (at T315396#8192570):

I don't think moving database types to extensions makes sense. Database types need to be supported by core and every extension, so they belong in core. The cross-cutting nature of database types and the need for support everywhere in the MediaWiki ecosystem is also why I don't think they're worth having.

Which I mostly agree with given that there is seemingly no longer enthusiasm for other DB types (again, evidenced by no one wanting to move this forward).

@roman-stolar: Could you please answer the last comment? Thanks in advance!

Per my comment at T352113#9362386, DatabaseInstaller is a bit of a nightmare with its mix of web UI and database backend code. Right now I'm very glad that this task was never done. Extension subclasses of DatabaseInstaller would make things much harder. Marking declined per legoktm's comment above.