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)

Event Timeline

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' );

... 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.

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"

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.

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

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

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.

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 claimed this task.