Page MenuHomePhabricator

1.28-alpha / Error: 42P01 ERROR: table "unittest_user_groups" does not exist
Closed, ResolvedPublic

Description

Follow-up on https://gerrit.wikimedia.org/r/#/c/317863/:

This used to work, now it just throws:

....................................[4333d361e722e82fc5489679] [no req]   DBQueryError from line 1054 of /home/travis/build/SemanticMediaWiki/mw/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: DROP TABLE "mediawiki"."unittest_user_groups" CASCADE
Function: Database::dropTable
Error: 42P01 ERROR:  table "unittest_user_groups" does not exist
Backtrace:
#0 /home/travis/build/SemanticMediaWiki/mw/includes/libs/rdbms/database/DatabasePostgres.php(247): Database->reportQueryError(string, string, string, string, boolean)
#1 /home/travis/build/SemanticMediaWiki/mw/includes/libs/rdbms/database/Database.php(912): DatabasePostgres->reportQueryError(string, string, string, string, boolean)
#2 /home/travis/build/SemanticMediaWiki/mw/includes/libs/rdbms/database/Database.php(3344): Database->query(string, string)
#3 /home/travis/build/SemanticMediaWiki/mw/includes/db/CloneDatabase.php(122): Database->dropTable(string)

Details

Related Gerrit Patches:
mediawiki/core : REL1_28Fix Postgres support
mediawiki/core : REL1_29Fix Postgres support
mediawiki/core : masterFix Postgres support

Event Timeline

mwjames created this task.Oct 28 2016, 9:59 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 28 2016, 9:59 PM

Looking at CloneDatabase it contains tableName( $tbl, 'raw' ) which causes issues in connection with the mediawiki schema similar to what has been outlined in https://gerrit.wikimedia.org/r/#/c/317863/ .

self::changePrefix( $this->oldTablePrefix );
$oldTableName = $this->db->tableName( $tbl, 'raw' );

self::changePrefix( $this->newTablePrefix );
$newTableName = $this->db->tableName( $tbl, 'raw' );
Paladox triaged this task as High priority.Oct 28 2016, 11:46 PM

... patch and T149454. This patch affects schema prefixes; that bug is about the user_groups table not being cloned by the unittests. As frustrating as it is to deal with MediaWiki's half-hearted Postgres support,

Just to be clear, this is not about user_groups table it just so happens that this table to be first that chokes on the CloneDatabasedestroy. This is issue about the mediawiki schema that get's mingled and prevents the drop action with the given example to be a symptom not a cause.

MediaWiki's half-hearted Postgres support,

Whether "half-hearted Postgres support" or not is hardly the discussion here.

@aaron would you be able to look into this please?

mw 1.28 is almost about to be released.

I belive we have this problem because prefix support was broken in 1.28. So we should fix this before 1.28 otherwise we will be having broken postgres support.

Krenair added a subscriber: Krenair.Nov 8 2016, 2:38 PM

Is there a full stack trace for this? I don't have problems with the core unit tests, so I can't see it.

Are you saying the "mediawiki." prefix in the DROP TABLE is the problem? Or that the clone tables have the wrong schema themselves (e.g. "public")?

aaron@AARON-ASPIRE-S7:~/mediawiki$ php maintenance/eval.php
> var_dump( wfGetDB(DB_MASTER)->tableName( 'user', 'raw' ) );
string(6) "mwuser"

> var_dump( wfGetDB(DB_MASTER)->tableName( 'page', 'raw' ) );
string(4) "page"
demon added a subscriber: demon.EditedNov 25 2016, 6:31 PM

I cannot actually replicate this on MW core using Postgres, so it's definitely something with how extensions are using the test framework. I'm not sure unit test failures on Postgres for a non-bundled extension is worth blocking the release.

@aaron I think it is the prefix, but not really sure.

mwjames added a comment.EditedNov 25 2016, 10:17 PM

Are you saying the "mediawiki." prefix in the DROP TABLE is the problem? Or that the clone tables have the wrong schema themselves (e.g. "public")?

As noted in T148628#2754647 (after a quick test) I'm not sure that the $this->tableExists( $tableName ) in Postgres returns the correct results.

The quick fix was to use DROP TABLE IF EXISTS just in case the tableExists did not return the expected result, which let to the question whether there is an issue or not with that function.

Maybe the schema needs to get the db prefix like MySQL?

so it's definitely something with how extensions are using the test framework.

I may repeat myself here as in the previous thread T148628, our tests and test framework does work with MW 1.26 (including Postgres) [0], yet with the expected upcoming release it no longer does for the Postgres version.

After changes to the Database class in MW 1.28 tests (the same that pass on MW 1.26) no longer pass given that we use the same framework and extension (by git sha1) with the only difference [1] being the selected MW version, it allows to make a conjecture that some changes in MW 1.28 are incompatible at best.

I'm not sure unit test failures on Postgres for a non-bundled extension is worth blocking the release.

What are you saying here, that just because we are a non-WMF affiliate extension means we are ultimate excluded from the process? Is that the correct reading?

[0] https://travis-ci.org/SemanticMediaWiki/SemanticMediaWiki/jobs/178520196
[1] https://en.wikipedia.org/wiki/Ceteris_paribus

demon added a comment.Nov 26 2016, 1:12 AM

I'm not sure unit test failures on Postgres for a non-bundled extension is worth blocking the release.

What are you saying here, that just because we are a non-WMF affiliate extension means we are ultimate excluded from the process? Is that the correct reading?

Nope, it's not a WMF-affiliated thing at all. It's the list of extensions/skins we bundle with MW tarball releases. There's a lot of WMF extensions that aren't bundled for which I would say the same thing re: postgres.

Anyway, 1.28 is a behind schedule and I'm not considering this a blocker anymore. If a fix is found and committed, I'm more than willing to backport and do a 1.28.1 release for it.

@mwjames and @aaron the reason why https://travis-ci.org/paladox/SemanticMediaWiki/builds/181018385 fails for postgres

is because we doint create a user table called user in postgres since it is a reserved word in postgres, instead we call it mwuser.

It seems some how this detection was broken in mw 1.28+ but works in 1.27 or lower.

@aaron @Krinkle I've been tracking this issue for some time now and as I suspected something changed on the validation of tableExists in PostgreSQL during 1.27 and 1.28 (respectively 1.29).

Re-adding $table = $this->realTableName( $table, 'raw' ); from 1.27 [0] to 1.29 allows to pass all our tests on PostgreSQL as well as having DatabasePostgres::relationExists to return the correct boolean value.

@@ -1049,10 +1049,11 @@ __INDEXATTR__;
 			$types = [ $types ];
 		}
 		if ( $schema === false ) {
 			$schema = $this->getCoreSchema();
 		}
+		$table = $this->realTableName( $table, 'raw' );
 		$etable = $this->addQuotes( $table );
 		$eschema = $this->addQuotes( $schema );
 		$sql = "SELECT 1 FROM pg_catalog.pg_class c, pg_catalog.pg_namespace n "
 			. "WHERE c.relnamespace = n.oid AND c.relname = $etable AND n.nspname = $eschema "
 			. "AND c.relkind IN ('" . implode( "','", $types ) . "')";

MediaWiki 1.29.0-alpha (6517f21)
PHP 5.6.8 (apache2handler)
PostgreSQL 9.3.12
ICU 54.1

[0] https://github.com/wikimedia/mediawiki/blob/REL1_27/includes/db/DatabasePostgres.php#L1276-L1293

Is it possible to fix this with 1.29, given the following message [0]?

[0] https://lists.wikimedia.org/pipermail/wikitech-l/2017-April/088019.html

demon added a comment.Apr 27 2017, 5:53 AM

If someone works on patches and gets them merged in time, absolutely.

If someone works on patches and gets them merged in time, absolutely.

I no longer have a valid gerrit setup, so count me out (of course I could do a github pull).

The analysis in T149454#3142728 contains a comment about a single line that went missing with 1.28/129 and the question is why it went missing. If it is a mere oversight then it should be re-added and of not what was the reason for the removal that eventual causes all our tests to fail on postgres after 1.27.

If someone works on patches and gets them merged in time, absolutely.

I no longer have a valid gerrit setup, so count me out (of course I could do a github pull).
The analysis in T149454#3142728 contains a comment about a single line that went missing with 1.28/129 and the question is why it went missing. If it is a mere oversight then it should be re-added and of not what was the reason for the removal that eventual causes all our tests to fail on postgres after 1.27.

You can submit patches on gerrit.wikimedia.org. If you want, other users can submit patches on your behalf if you create the pull or a paste of the diff.

a paste of the diff.

Here you go, the diff:

You can submit patches ...

Again, this is not just about a patch or a missing line. Why did the line disappeared in the first place? Did someone thought it is no longer necessary? If so, why?

Change 350562 had a related patch set (by Paladox) published:
[mediawiki/core@master] Fix Postgres support

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

@mwjames For future reference: you can also use https://tools.wmflabs.org/gerrit-patch-uploader/ to upload patches to Gerrit.

matmarex removed a subscriber: matmarex.Apr 27 2017, 5:47 PM
Krinkle removed a subscriber: Krinkle.Apr 29 2017, 3:09 AM
Seb35 added a subscriber: Seb35.May 26 2017, 10:07 PM

Possibly this issue is linked to T155147, or at least the underlying code: initialisation of the database in unit tests is managed by two properties, one static and one non-static, so possibly it creates a disorder in the initialisation state of the database.

Change 358640 had a related patch set uploaded (by Reception123; owner: Mwjames):
[mediawiki/core@REL1_29] Fix Postgres support

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

Change 350562 merged by jenkins-bot:
[mediawiki/core@master] Fix Postgres support

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

Change 358923 had a related patch set uploaded (by Paladox; owner: Mwjames):
[mediawiki/core@REL1_29] Fix Postgres support

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

Change 358924 had a related patch set uploaded (by Paladox; owner: Mwjames):
[mediawiki/core@REL1_28] Fix Postgres support

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

@mwjames can you confirm that everything here works now please? Patch has been back ported to 1.29 and 1.28 now.

Change 358923 merged by jenkins-bot:
[mediawiki/core@REL1_29] Fix Postgres support

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

Change 358924 merged by jenkins-bot:
[mediawiki/core@REL1_28] Fix Postgres support

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

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

@mwjames can you confirm that everything here works now please?

I was once again able to run our tests after the mentioned PR (but requires T167927) without the observed issue on:

MediaWiki	1.30.0-alpha (1ba29a5)
PHP	7.1.1 (apache2handler)
PostgreSQL	9.3.12
ICU	57.1
demon closed this task as Resolved.Jun 15 2017, 4:13 PM
demon claimed this task.