Drupal install error on PostgreSQL 9.0 database

The "Table variable already exists" message suggests to me that Drupal was already partially installed... is it possible you did not completely remove a previous installation?

Or is it possible that you double-clicked the submit button while installing? (See: #881494: Double-clicking the "Install Drupal" button completely breaks everything)

Please try a fresh installation again and see if you can replicate this error reliably, or if it was a one-time thing only. Thanks!

Get same error on install drupal 7.0-beta1:

Notice: unserialize(): Error at offset 0 of 59 bytes in install_verify_completed_task() (line 784 of C:\inetpub\wwwroot\drupal7\includes\install.core.inc).
Table variable already exists.

Configuration:
- IIS on Windows 7
- PostgreSQL 9.0.1
- PHP 5.3.3 / 5.2.14 (tried both) via FastCGI

It's not one-time thing. Tried fresh install 4-more times.

I bet that if you test this with current 7.x-dev, the problem is still there and you change the version of this thread, more poeple will see your problem.

unic - could you try and replicate this with 7.x-dev ?

Changing the version as suggested by mokko to get more eyes on this.

This very much sounds like a PHP or postgres bug on Windows XP, until someone can reproduce on another operating system.

We need to find out which versions are affected so this can be documented, but since transcendx successfully installed after upgrading PHP it looks like 1. Not a Drupal bug 2. Not a release blocker.

Downgrading this to 'major' and moving to the postgresql queue in the hope that finds some more testers.

This actually sounds like it could be the same as #784062: 2006 MySQL server has gone away trying to install on Network Solutions....

If so, trying it on the latest 7.x means it should now fail with a friendly error message instead (due to #818374: Add a requirements check error if PECL PDO is being used). Guess we'll see.

> unic - could you try and replicate this with 7.x-dev ?
I tried this with 7.x-dev from October 11, 2010. Same error.

unic this was fixed on october 12, so you need the *latest* version ;).

I'm using the same configuration as unic (Windows 7, PostgreSQL 9...). beta1 has the problem, I've just downloaded -dev and it still has it.

Just tested drupal-7.x-dev (Last updated: October 19, 2010 - 06:07). Same error.

I'm seeing the same issue on Debian Lenny using the PHP build from http://www.dotdeb.org/2009/11/30/php-5-3-1-packages-for-debian-lenny-the..., PostgreSQL 9.0.1 (the platform independent installer from EnterpriseDB) with the dev version of Drupal downloaded just a few minutes ago.

subscribe

A likely cause is that your PostgreSQL database has not been created using UTF-8 as the encoding.

Can you try to recreate the database with the proper encoding and report back?

Damien, assuming that's the problem, can we display a useful error message in this condition?

@webchick: I think Drupal 6 used to, we lost that somewhere along the way.

Chatted with Damien on IRC and we figured out that the cause of this issue is the new bytea output format used in PostgreSQL 9.0 (see http://www.postgresql.org/docs/9.0/static/datatype-binary.html). To fix, set:

bytea_output = 'escape'

That can be done in postgresql.conf, or you can do:

ALTER DATABASE drupal SET bytea_output = 'escape';

It would be good if Drupal could do that of course, until such time as it learns about the new output format.

This impacts strings because we need some kind of message to tell pgsql users what to do here (other than ping DamZ on IRC ;)).

pgsnake or transcendx, any suggestions on an error that would've helped you here?

Here is a potential patch, which shows an error when trying to install with bytea_output set to 'hex'.

I'm unable to test this though because I don't a machine to install postgresql 9 on.

forgot a bracket

#21 looks OK to me, with the possible exception of putting code tags around the proposed query itself. Or some other way of setting it off from the main text.

Let's do that, and then I'm OK with this is DamZ and fiasco are. I'll let them set it RTBC.

Here's an updated patch that displays the query inside <pre>-tags (<code> is only for comments). Also extended the comment a bit to explain why the if-condition is necessary.

Maybe we should instead refer to INSTALL.pgsql.txt as in checkEncoding()?

Updated patch, replacing %query with !query in the arguments array as well.

Apparently I was wrong about the <code>-tag being only for comments, didn't know about this one in HTML, so rerolled with <code> instead of <pre>.

+    if (version_compare(DatabaseConnection::getConnection()->version(), '9') >= 0) {
+      $bytea_output = db_query('SHOW bytea_output')->fetchField();
+      if ($bytea_output != 'escape') {
+        $replacements = array(
+          '%setting' => 'bytea_output',
+          '%currentvalue' => $bytea_output,
+          '%neededvalue' => 'escape',
+          '!query' => "ALTER DATABASE your_database SET bytea_output = 'escape';",
+        );
+        $this->fail(st("The %setting setting is currently set to '%currentvalue', but needs to be '%neededvalue'. Change this by running the following query: !query", $replacements));
+      }
+    }

I'm not happy with this. The issue we have here is that PDO_pgsql has not been updated to take into account the new bytea output method introduced by PostgreSQL 9. What we should actually check here is: "does the roundtrip 'insert a bytea, read the value' works?" If it doesn't, we can point the user toward the temporary solution.

We don't need and shouldn't enforce a specific bytea output.

I like Damien's proposal. It's much cleaner than a hard setting dependency. Stevel, can you reroll accordingly?

I've been trying to see what is actually going on, and it seems that when bytea_output = 'hex', all binary data is represented using their hex value, but the procedure 'enter bytea data and retrieve it' keeps working either way.

Unserialize expects the 'serialized data format' (s:4:"text" or a:0:{}). Without forcing bytea_output to escape, we'll need to check the format and potentially decode the hex format everywhere the data is retrieved from the database, which really isn't an option.

Perhaps a different option is to just execute "SET bytea_format = 'escape';" when connecting to PostgreSQL > 9, and as such the database default doesn't matter any more.

A new patch is attached.

Zero-byte patch is no good...

I've been trying to see what is actually going on, and it seems that when bytea_output = 'hex', all binary data is represented using their hex value, but the procedure 'enter bytea data and retrieve it' keeps working either way.

Unserialize expects the 'serialized data format' (s:4:"text" or a:0:{}). Without forcing bytea_output to escape, we'll need to check the format and potentially decode the hex format everywhere the data is retrieved from the database, which really isn't an option.

I cannot make sense out of this. The problem we have *is* that the 'enter bytea data and retrieve it' doesn't work in some cases (maybe only when some characters are present in the input). We need to identify when it happens and add a check for it.

Reroll using the roundtrip-method to see if the data are returned correctly.

I downloaded and played with pgsql for the first time today and once I figured out how to escape from the &*@#ing prompt (thanks, davehimself in #drupal! :)), I tested the installer with PostgreSQL 9.0, and hit this bug. Yay!

The patch got me out of the problem once I performed the alter query on the right database. One nit-pick with the string:

+ '!query' => "<code>ALTER DATABASE your_database SET bytea_output = 'escape';",

You already have the name of my database in the form. Can't you just insert it into the string?

Also note that webchick reported The bytea_output setting is currently set to 'x657363617065', which is e-s-c-a-p-e in hex. So what gives...?

Also, is there a reason we don't just run this query for them automatically?

We should not be running the ALTER DATABASE query automatically, because the database could be shared with another application which expects to have bytea_output = 'hex'.

Attached a reroll of the previous patch, fixing the error message and used another string (encoding instead of escape) to verify the roundtrip with.

We can set it for the connection automatically (SET bytea_output = 'escape', just as we do SET names 'UTF8') when connecting, which is what happens in #30, but DamZ doesn't seem to like that. It would make a better UX imho.

DamZ, could you clearify whether #30 looks good to you or why not?

Thanks

#30 is exactly what I'm trying to avoid: it's up to the connection library (here PDO_pgsql) to do the decoding of the strings, so PDO_pgsql should one day be updated to support hex encoding transparently. This new encoding is more efficient to decode, and we should allow people to take advantage of it when available.

#36 looks very good to me. Discussing with Stevel on IRC, I asked him if we could try to run the query directly. Stevel is afraid that some client application might requires hex encoding, but I think it is a false assumption: every client application that supports the newer hex encoding also supports the older escape encoding, but the opposite is not true.

Updated the patch to run the alter query directly, and show an error only when it fails.

Reroll changing the method name not to start with an underscore, and prefixing the method calls with $this, as per irc.

This patch tries to force a database in the escape encoding mode if we detected that a round trip of to binary storage doesn't work. It is future proof in that whenever PDO_pgsql is updated we will not force the user to change the encoding mode of the database.

Tested and approved.

Hm. I applied this and tested it and got:

"Fatal error: Call to undefined function _checkBinaryOutputRoundtrip() in /Users/webchick/Sites/core/includes/database/pgsql/install.inc on line 91"

Oh, ok. #40 seems to function properly, though it was not able to execute the ALTER query even though I'm the "postgres" user. Hrm.

Minor touch-ups:

+      if (! $this->checkBinaryOutputRoundtrip()) {

(two places) no space between ! and $this

+        // telling the user to do it himself.

himself -> themselves. And/or some other way to re-write that comment without the use of gender-specific pronouns.

+          db_query("ALTER DATABASE " . $connection_options['database'] . " SET bytea_output = 'escape';");

This is direct user input. Shouldn't this be escaped?

+  private function checkBinaryOutputRoundtrip() {

Could we come up with a better name for this function that's more explicitly clear about what it does? "roundtrip" is not a common name for "check to see if the alter query you just did worked or not" from what I know.

Also, I know that this will tick some people off, but I really think the fact that D7, one of whose most compelling improvements was the new DB abstraction layer to allow for better non-MySQL database support, cannot install on the latest version of pgsql, is a critical bug.

/me runs and hides under the blankets

Oh, and:

+          $this->fail(st("The %setting setting is currently set to '%currentvalue', but needs to be '%neededvalue'. Change this by running the following query: !query", $replacements));

%current_value, %needed_value. We don'trunwordstogether in core.

Fixed all but "This is direct user input. Shouldn't this be escaped?" as I don't know how to address it. untested.

Also, I renamed the function to checkBinaryOutputSuccess, not sure if that's a better name.

+          db_query("ALTER DATABASE " . $connection_options['database'] . " SET bytea_output = 'escape';");

This is direct user input. Shouldn't this be escaped?

It doesn't need to be from a security perspective, but it would be better to escape the identifier with double quotes.

This patch addresses the escaping-issue as well.

+++ includes/database/pgsql/install.inc	15 Dec 2010 15:18:50 -0000
@@ -76,6 +81,45 @@ class DatabaseTasks_pgsql extends Databa
+          db_query("ALTER DATABASE \"" . $connection_options['database'] . "\" SET bytea_output = 'escape';");

I don't think that line is fully secure. We should be using the quote() method for the connection object, shouldn't we, rather than just blindly throwing " around it? Also, should we be using the connection object directly rather than db_query() here? (Probably, if we have to get it to call quote() anyway...)

Other than that, this looks structurally fine. I defer to Damien on its Postgres-validity.

Powered by Dreditor.

quote() is not valid here (it is for values, not for identifiers). We could use one of the escape*(), but I don't believe that's necessary. This is administrator input and happens after a successful authentication.

Doesn't pgsql ship with a default user/pass? So couldn't any D7 site that was left unattended and the install uncompleted be compromised by some random anonymous passer-by?

webchick postgresql does not ship with a default user/pass and it's the distros job to have a post install method to allow the user to create the first superuser for postgresql.

As Damien already said, the input for the query is only used when a connection is already established (and thus the database name is something correct). Back to needs review...

@webchick: An anonymous passer-by would be better of choosing sqlite, which doesn't need a database username/password at all to install.

Stevel: Ah. Can you add a comment to that effect to avoid scaring people down the road? I could see potential false security reports otherwise.

This patch adds a comment to the query string building. I splitted the query into a separate variable to eliminate a bit of duplicate code.

#56: 926636-execute-alter-query-comment.patch queued for re-testing.

The cause of the previous failing:

Failed: PDOException: SQLSTATE[42S01]: Base table or view already exists: 1050 Table './drupaltestbotmysql/simpletest449928menu_router' already exists: ALTER TABLE {menu_router} DROP `block_callback`; Array ( ) in db_drop_field() (line 2724 of /var/lib/drupaltestbot/sites/default/files/checkout/includes/database/database.inc).

Not sure how a change in DatabaseTasks_pgsql can trigger this kind of error, or why "base table already exists" is so wrong for an ALTER TABLE statement...

#34 was answered back then in IRC has been explained as bogus reporting, nothing else. My only remaining question is why catch {} and then re-check instead of moving the fail inside the catch? Is it possible that pgsql does not throw an exception on failure??

Here's the patch, jsut to expidite things

*forehead slap*

Stevel? DamZ? Bueller?

+++ includes/database/pgsql/install.inc	19 Dec 2010 05:09:38 -0000
@@ -76,6 +81,49 @@ class DatabaseTasks_pgsql extends Databa
+    if (version_compare($database_connection->version(), '9') >= 0) {

Not worth a "needs work" but I think it make sense to use the $operator argument of version_compare() here, ie:

if (version_compare($database_connection->version(), '9', '>=')) {

Powered by Dreditor.

My only remaining question is why catch {} and then re-check instead of moving the fail inside the catch? Is it possible that pgsql does not throw an exception on failure??

Actually, the previous patches implemented the correct behavior: we detect a faulty condition, try a command that might fix it, recheck. Moving the fail inside the catch is making the assumption that the success of the command will guarantee that the faulty condition is fixed, which is just wrong.

#56 is RTBC (NOT #61).

I tested this and was still getting the error about the ALTER DATABASE statement having failed, even though I'm the 'postgres' user. Damien walked me through this patch on Skype and I think we figured out the problem.

The checkOutputSuccess() function is still using the info from the current connection, which initiated with the thing in the wrong encoding. We need that to instead test a new connection with the new encoding. If you ignore the error and hit submit anyway, the installation goes fine, but most people won't know to do that.

So marking back down to "needs work" but I think we're really close.

Complete shot in the dark.

Better comments (suggested by chx).

I guess this is good to go but I still do not understand why we added exceptions to Drupal 7 causing a ton of problems if we can't use them like here?

I tested the hell out of this with DamZ over Skype and #66 successfully installed without warnings for the 'postgres' user, the 'peon' user when he was set to be owner of the DB, and gave the notice for a 'peon' user who was merely granted all privileges to a database owner by 'postgres'.

Since #66 passed the testbot, and the only difference between #66 and #67 is comments and whitespace, Committed #67 to HEAD!

One critical down! One to go! :)