Page MenuHomePhabricator

PostgreSQL support
Closed, ResolvedPublic

Description

At the moment, the OpenID connect extension seems to be incompatible with PostgreSQL because the database schema updates use MySQL syntax:

server ~/mediawiki-extensions-OpenIDConnect # cat AddIssuer.sql AddSubject.sql 
ALTER TABLE /*_*/user ADD issuer TINYBLOB;
ALTER TABLE /*_*/user ADD subject TINYBLOB;

These columns therefor aren't added to the table in PostgreSQL. For some strange reason, I can't find any error messages about this... I had expected update.php to try to apply this ALTER TABLE anyway and fail, but it doesn't print any error message and I also don't see anything in the debug log while running MediaWiki. I also can't find any mention of a syntax error in my postgresql-9.6-main.log, but I do get an error in that log when I run the SQL commands manually in psql:

server ~ # tail -n2 /var/log/postgresql/postgresql-9.6-main.log
2018-01-20 19:33:03 GMT ERROR:  syntax error at or near "user" at character 18
2018-01-20 19:33:03 GMT STATEMENT:  ALTER TABLE user ADD issuer TINYBLOB;

I was able to work around the issue by manually adding the needed columns:

mediawiki=# ALTER TABLE mediawiki.mwuser ADD issuer TEXT;
ALTER TABLE
mediawiki=# ALTER TABLE mediawiki.mwuser ADD subject TEXT;
ALTER TABLE

Note that you need to specify the schema ($wgDBmwschema, defaults to mediawiki) and that the table is called mwuser instead of user:

www-data@4292bfd9f33b:~/html/maintenance/postgres$ grep -m1 mwuser tables.sql
CREATE TABLE mwuser ( -- replace reserved word 'user'

Event Timeline

Thanks for the information. I would welcome a patch to add PostgreSQL support. I don't have access to an environment to test PostgreSQL.

Ugh... Why does this extension modify a core table? That's a real big faux pas :(

https://www.mediawiki.org/wiki/Do_not_hack_MediaWiki_core

It should add it's own DB table, with a PK/FK of the user_id, and then the two columns it needs

CREATE TABLE /*_*/openid_connect (
  oic_user int unsigned PRIMARY KEY NOT NULL,
  oic_subject TINYBLOB,
  oic_issuer TINYBLOB
) /*$wgDBTableOptions*/;

Ugh... Why does this extension modify a core table? That's a real big faux pas :(

https://www.mediawiki.org/wiki/Do_not_hack_MediaWiki_core

It should add it's own DB table, with a PK/FK of the user_id, and then the two columns it needs

I agree with you. But the linked page doesn't actually say anything about database changes at all, and I also couldn't find any advice about this on other pages like:

https://www.mediawiki.org/wiki/Manual:Hooks/LoadExtensionSchemaUpdates
https://www.mediawiki.org/wiki/Manual:Developing_extensions
https://www.mediawiki.org/wiki/Manual:Database_access
https://www.mediawiki.org/wiki/Development_policy
https://www.mediawiki.org/wiki/Best_practices_for_extensions#Database

I would suggest that the documentation should be updated to reflect this policy.

Ugh... Why does this extension modify a core table? That's a real big faux pas :(

https://www.mediawiki.org/wiki/Do_not_hack_MediaWiki_core

It should add it's own DB table, with a PK/FK of the user_id, and then the two columns it needs

I agree with you. But the linked page doesn't actually say anything about database changes at all, and I also couldn't find any advice about this on other pages like:

https://www.mediawiki.org/wiki/Manual:Hooks/LoadExtensionSchemaUpdates
https://www.mediawiki.org/wiki/Manual:Developing_extensions
https://www.mediawiki.org/wiki/Manual:Database_access
https://www.mediawiki.org/wiki/Development_policy
https://www.mediawiki.org/wiki/Best_practices_for_extensions#Database

I would suggest that the documentation should be updated to reflect this policy.

I'm not sure why Database changes need to be singled out if we're saying not to modify core at all in the first place?

However, there's some places, such as https://www.mediawiki.org/wiki/Best_practices_for_extensions#Database that we should state this. Will file a forked task

I'm not sure why Database changes need to be singled out if we're saying not to modify core at all in the first place?

I'm not very familiar with MediaWiki development, but as far as I can tell, the mediawiki.org wiki pretty much explicitly defines "core" as the "files that are part of the main MediaWiki package" ( https://www.mediawiki.org/wiki/Core ). The "Do not hack MediaWiki core" page also explicitly refers to files several times:

editing an individual wiki installation's core MediaWiki files often leads to more problems than solutions
For the purposes of this essay, "core" is meant to include all files that belong to the original MediaWiki installation. That is to say all files except LocalSettings.php, the ones in your "extensions" folder, or other folders which you have added since your installation.
Why you should not modify core files

So this wouldn't be singling out, but rather including :)

However, there's some places, such as https://www.mediawiki.org/wiki/Best_practices_for_extensions#Database that we should state this. Will file a forked task

Thanks!

@Reedy, yes, those columns should indeed be in a separate table. This was one of the first extensions that I wrote many years ago, and I did not consider the implications at that time. It would be helpful to have this best practice explicitly documented, as @martin.von.wittich mentions. I will add to my to do list making this schema change and creating a migration script for upgrading.

Change 426095 had a related patch set uploaded (by Cicalese; owner: Cicalese):
[mediawiki/extensions/OpenIDConnect@master] Move subject and issuer columns out of user table.

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

Change 426095 merged by jenkins-bot:
[mediawiki/extensions/OpenIDConnect@master] Move subject and issuer columns out of user table.

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

CCicalese_WMF claimed this task.
CCicalese_WMF removed a project: Patch-For-Review.
CCicalese_WMF moved this task from Backlog to Closed on the MediaWiki-extensions-OpenID-Connect board.

Thanks for adding PostgreSQL support :)

Unfortunately, I think I've found a small bug: the database fields are named incorrectly in the migration;

https://gerrit.wikimedia.org/r/#/c/426095/8/maintenance/MigrateOIDCSubjectAndIssuerFromUserTable.php

'oic_user' => 'user_id',
'oic_subject' => 'subject',
'oic_issuer' => 'issuer'

They should be named oidc_*:

https://gerrit.wikimedia.org/r/#/c/426095/8/sql/postgres/AddTable.sql

CREATE TABLE openid_connect (
  oidc_user INTEGER NOT NULL PRIMARY KEY,
  oidc_subject VARCHAR(255) NOT NULL,
  oidc_issuer VARCHAR(255) NOT NULL
);

This causes an exception during update.php:

...openid_connect table already exists.
[ca18f8da7af08f8c5c8dc466] [no req]   Wikimedia\Rdbms\DBQueryError from line 1149 of /var/www/html/includes/libs/rdbms/database/Database.php: A database query error has occurred. Did
you forget to run your application's database schema updater after upgrading?
Query: INSERT  INTO "openid_connect" (oic_user,oic_subject,oic_issuer) SELECT  user_id,subject,issuer  FROM "mwuser"    WHERE (subject IS NOT NULL) AND (issuer IS NOT NULL)
Function: MigrateOIDCSubjectAndIssuerFromUserTable::doDBUpdates
Error: 42703 ERROR:  column "oic_user" of relation "openid_connect" does not exist
LINE 1: ...tes www-data@599b90... */  INTO "openid_connect" (oic_user,o...
                                                             ^


Backtrace:
#0 /var/www/html/includes/libs/rdbms/database/DatabasePostgres.php(262): Wikimedia\Rdbms\Database->reportQueryError(string, string, string, string, boolean)
#1 /var/www/html/includes/libs/rdbms/database/Database.php(979): Wikimedia\Rdbms\DatabasePostgres->reportQueryError(string, string, string, string, boolean)
#2 /var/www/html/includes/libs/rdbms/database/Database.php(2455): Wikimedia\Rdbms\Database->query(string, string)
#3 /var/www/html/includes/libs/rdbms/database/DatabasePostgres.php(725): Wikimedia\Rdbms\Database->nativeInsertSelect(string, string, array, array, string, string, array, array)
#4 /var/www/html/includes/libs/rdbms/database/Database.php(2388): Wikimedia\Rdbms\DatabasePostgres->nativeInsertSelect(string, string, array, array, string, array, array, array)
#5 /var/www/html/extensions/OpenIDConnect/maintenance/MigrateOIDCSubjectAndIssuerFromUserTable.php(86): Wikimedia\Rdbms\Database->insertSelect(string, string, array, array, string)
#6 /var/www/html/maintenance/Maintenance.php(1581): MigrateOIDCSubjectAndIssuerFromUserTable->doDBUpdates()
#7 /var/www/html/extensions/OpenIDConnect/src/OpenIDConnect.php(467): LoggedUpdateMaintenance->execute()
#8 /var/www/html/includes/installer/DatabaseUpdater.php(472): OpenIDConnect::migrateSubjectAndIssuer(PostgresUpdater, PostgresUpdater)
#9 /var/www/html/includes/installer/DatabaseUpdater.php(440): DatabaseUpdater->runUpdates(array, boolean)
#10 /var/www/html/maintenance/update.php(204): DatabaseUpdater->doUpdates(array)
#11 /var/www/html/maintenance/doMaintenance.php(92): UpdateMediaWiki->execute()
#12 /var/www/html/maintenance/update.php(249): require_once(string)
#13 {main}

Change 431558 had a related patch set uploaded (by Cicalese; owner: Cicalese):
[mediawiki/extensions/OpenIDConnect@master] Fixed column names in maintenance script.

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

Yep, I've manually edited my extensions/OpenIDConnect/maintenance/MigrateOIDCSubjectAndIssuerFromUserTable.php and added the missing d to those three entries, and that seems to resolve the issue :)

# docker container exec -u www-data mediawiki php /var/www/html/maintenance/update.php --quick
[...]
...openid_connect table already exists.                                                                 
Migrated data from user table to openid_connect table.                                                                                
[...]
Purging caches...done.                                                
                                                                
Done in 0.7 s.

EDIT: also tested the OpenID login, works fine too.

Change 431558 merged by jenkins-bot:
[mediawiki/extensions/OpenIDConnect@master] Fixed column names in maintenance script.

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