Page MenuHomePhabricator

r37313 broke parser tests for alternative databases.
Closed, ResolvedPublic

Description

Author: overlordq

Description:
Horrible patch

r37313 changed a bunch of stuff around but hard coded using backticks for delimiters which is (afaik) MySQL specific. So I created this rough patch.

It works on PG, untested on others.


Version: 1.14.x
Severity: enhancement

attachment parserTests.inc.patch ignored as obsolete

Details

Reference
bz14990

Related Objects

View Standalone Graph
This task is connected to more than 200 other tasks. Only direct parents and subtasks are shown here. Use View Standalone Graph to show more of the graph.
StatusSubtypeAssignedTask
InvalidNone
ResolvedNone

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 10:13 PM
bzimport set Reference to bz14990.
bzimport added a subscriber: Unknown Object (MLST).

I found this problem too but it's more than just backticks - the script makes some hacks to table prefixes that just won't work on PG, at least without a lot of trouble (I tried). But removing the backticks would be a nice first step to getting it compatible again.

Just looked at the patch, and I don't think we need to add specific table delimiters, we can just remove the backticks entirely. While they are needed if your table contains weird characters or MixedCase, everything in tables.sql is sane and standard, so the raw names should be fine, as far as I can tell.

ayg wrote:

The correct solution for the table delimiters is to use Database::tableName(). Due to the changing prefix, this is a little tricky, but I've done it in r38310.

It still probably will not work on pgsql, because pgsql appears not to support DB prefixes, judging by DatabasePostgres::tableName(). If pgsql doesn't *want* to support DB prefixes, we could add a $prefix argument to tableName() as an alternative solution.

overlordq wrote:

PG shouldn't/doesn't use table prefixes, it uses schemas.

Anyways:

Query: DROP TABLE page
Function:
Error: 1 ERROR: cannot drop table page because other objects depend on it

HINT: Use DROP ... CASCADE to drop the dependent objects too.

The only real problem now is the supposed 'cleanup' from aborted runs. From the docs of both PG and MySQL they mention that temp tables are dropped at the end of the connection which should drop them after normal script termination or somebody hitting ctrl-c.

Or I could be completely off-base here in assuming this is all done with one connection.

After changing the if on line 516 to include

$wgDBtype != 'postgres'

in the conditional, the tests run spitting out the usual parser test errors

Running test Bug 6200: paragraphs inside blockquotes (extra line break on open)... FAILED!
Running test Bug 6200: paragraphs inside blockquotes (extra line break on close)... FAILED!
Running test Bug 6200: paragraphs inside blockquotes (extra line break on open and close)... FAILED!

Passed 505 of 524 tests (96.37%)... 19 tests failed!

overlordq wrote:

*** Bug 14991 has been marked as a duplicate of this bug. ***

So doing what will get this issue resolved? Any takers?

overlordq wrote:

parser tests patch

The workaround in bug r15892

Warning: pg_query(): Query failed: ERROR: relation "mwuser" already exists in /var/www/wiki/includes/db/DatabasePostgres.php on line 580
A database error has occurred
Query: CREATE TABLE mwuser (LIKE mwuser INCLUDING DEFAULTS)
Function:
Error: 1 ERROR: relation "mwuser" already exists

Backtrace:
#0 /var/www/wiki/includes/db/Database.php(616): DatabasePostgres->reportQueryError('ERROR: relatio...', 1, 'CREATE TABLE m...', '', false)
#1 /var/www/wiki/maintenance/parserTests.inc(662): Database->query('CREATE TABLE m...')
#2 /var/www/wiki/maintenance/parserTests.inc(270): ParserTest->setupDatabase()
#3 /var/www/wiki/maintenance/parserTests.php(73): ParserTest->runTestsFromFiles(Array)

#4 {main}

Since PG doesn't use table prefixes not putting TEMPORARY in the table creation statements will just try to create duplicates.

After fixing bug 17992, I made the following changes (will attach as patch), and parser tests work fine with PG.

attachment ptests.patch ignored as obsolete

overlordq wrote:

Meant to say:

The workaround in bug r15892 similarly irritates this same issue.

overlordq wrote:

re-upping patch last one broken?

FWIW, I didn't test this with multi-servers && PG, YMMV.

Attached:

overlordq wrote:

Closing as fixed, works now.

Jdforrester-WMF subscribed.

Migrating from the old tracking task to a tag for PostgreSQL-related tasks.