System: FreeBSD 8.0, psql 8.4.3, xs-httpd 3.5
Action: Drupal 7 database update from alpha2 to alpha3

Expected the update procedure to succeed without errors, but got the following message (could not find the complete message in the dblog report, this is a copy from the update script view op=results):

Update #7051

    * Failed: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near "ip" LINE 1: ALTER TABLE drupal_blocked_ips ALTER ip SET ip = CAST(ip_old... ^

My first reflex was to think that the column names should have been quoted, however a search through the keyword appendix in the PostgreSQL 8.4 documentation pointed out that "ip" is not a reserved postgresql word. Not sure what's wrong. I'm having trouble looking up the source code for the query (don't know which file to view, but that's maybe due to my Drupal inexperience).

Files: 
CommentFileSizeAuthor
#60 postgresql_changefield6.patch2.12 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,318 pass(es). View
#57 postgresql_changefield6.patch2.11 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,318 pass(es). View
#56 postgresql_changefield5.patch1.95 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,322 pass(es). View
#50 postgresql_changefield4.patch6.46 KBtpfeiffer
FAILED: [[SimpleTest]]: [MySQL] 23,302 pass(es), 0 fail(s), and 1 exception(es). View
#46 simpletest_tests_schema.diff2.39 KBtpfeiffer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_tests_schema.diff. View
#46 pgsql_schema.inc_.diff1.87 KBtpfeiffer
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pgsql_schema.inc_.diff. View
#39 postgresql_changefield3.patch1.62 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,298 pass(es). View
#38 postgresql_changefield2.patch1.62 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,245 pass(es). View
#31 postgresql_changefield.patch1.55 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 23,234 pass(es). View
#9 pgsql.761976.patch1.55 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 19,673 pass(es). View
#7 pgsql.761976.patch1.5 KBjhedstrom
PASSED: [[SimpleTest]]: [MySQL] 19,671 pass(es). View

Comments

Rob van den Bogaard’s picture

Checking against Postresql and MySQL manuals I wonder whether the produced query is actually valid sql: I cannot find accounts of using the "ALTER column SET name = value" syntax. ALTER only seems to know "SET DEFAULT".

Rob van den Bogaard’s picture

Ah, found the update source code - it's my inexperience with Drupal that I didn't find it right away: the error included a reference to the system module, so de code is in modules/system/system.install:2389 in function system_update_7051():
db_change_field('blocked_ips', 'ip', 'ip', array('description' => 'IP address', 'type' => 'varchar', 'length' => 40, 'not null' => TRUE, 'default' => ''));

If I'm correct the update script in the system module intends to do "ALTER TABLE {blocked_ips} ALTER ip TYPE varchar(40);" while it actually executes "ALTER TABLE {blocked_ips} ALTER ip SET ip = CAST(ip_old as varchar)", in the Postgresql implementation of changeField (includes/database/pgsql/schema.inc:481).

Note the reference to non-existing column "ip_old" and the omission of the type length value (40) specified by the system_update_7051() function.

Rob van den Bogaard’s picture

Ok, because there's not much traffic on this subject and I unfortunately don't have the time now to write a patch, I just executed the migration query by hand and edited includes/database/pgsql/schema.inc at line 460 to ignore the changeField call (just put a return; there) in the update (to get rid of the unfinished state of the update).

Will monitor changes in upcoming versions of 7 to see if by any chance this got solved. And should I get some spare time I'll try to fix it and to submit a patch.

Damien Tournoud’s picture

Title: Update #7051 fails for postgresql database » DatabaseSchema_pgsql::changeField() is broken
Version: 7.0-alpha3 » 7.x-dev
Component: update system » postgresql database
Priority: Normal » Critical

Hm. So it seems that changeField() is slightly broken (slightly as in "it doesn't work at all").

Promoting to critical.

andypost’s picture

Confirm this bug

Update #7051
Failed: SQLSTATE[42601]: Syntax error: 7 ERROR: syntax error at or near "ip" at character 38

EDIT: I found no logic inside changeField() ...

Also using "_old" suffix is totally wrong because there could be Field API generated fields like "field_price" and "field_price_old"

Suppose function should detect name and type changes and only then add or drop columns. Also renaming a field we should rename/rebuild related constraints.

jhedstrom’s picture

Assigned: Unassigned » jhedstrom
Status: Active » Needs review
FileSize
1.5 KB
PASSED: [[SimpleTest]]: [MySQL] 19,671 pass(es). View

This patch seems to fix the problem, in that previously failing tests in the Schema API section, now pass. However, the changes are quite dramatic, as casting to serials isn't at all supported. So the patch, in the case of changing a field to a serial will first create a sequence. The patch also addresses the sticky issue of _old andypost mentions in #5, since it uses pgsql's SET DATA TYPE (http://www.postgresql.org/docs/8.4/static/sql-altertable.html).

andypost’s picture

Status: Needs review » Needs work

SET DATA TYPE is for pg 8.4 but minimal requirements is 8.3 so only TYPE available http://www.postgresql.org/docs/8.3/static/sql-altertable.html
ALTER [ COLUMN ] column TYPE type [ USING expression ]

PS: Let's use prefix for _old field as mentioned in #685486: filter_update_7003() doesn't work for contrib modules

jhedstrom’s picture

Status: Needs work » Needs review
FileSize
1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 19,673 pass(es). View

This patch uses the 8.3-compatible syntax. Regarding the _old prefix/suffix, the only place it was left was here:

    $this->connection->query('ALTER TABLE {' . $table . '} RENAME "' . $field . '" TO "' . $field_new . '_old"');

which, upon closer inspection made no sense to rename the existing field to the new field name with '_old' suffixed. This patch simply renames the field to the new name, as I assume is desired.

andypost’s picture

+1 it works, tested on system Update #7051

I think it's ready, let's wait for DamZ review

jhedstrom’s picture

Out of curiosity, is there a way to force the test bot to run tests on a pgsql platform?

andypost’s picture

@jhedstrom this is not possible right now #710858: Meta issue: fix the remaining PostgreSQL bugs

Damien Tournoud’s picture

Status: Needs review » Needs work

This looks far from being ready, but that's probably because we have nearly no test for the schema alteration functions.

In no particular order, the current implementation:

- can't actually convert a column to bigint
- ignores $spec['pgsql_type']
- ignores $spec['default']
- ignores $spec['unsigned']
- ignores $spec['precision'] and $spec['type']
- doesn't convert the default if needed (see the notes on ALTER TABLE)
- doesn't remove the sequence when changing a 'serial' column to non serial

If someone wants to take the ball on writing tests, here is the minimum we need:

- split SchemaTestCase::testSchema() into smaller, targeted, test cases
- test converting a string column to int (and verify that the data actually makes sense)
- test converting an int column to string
- test converting a serial column to int
- test converting a string column with default to int with a different default
- test converting a NOT-NULL column to NULL, and verify that the 'default' value is used to fill-in the NULL entries
- test converting an int column to numeric
- test converting a numeric column to float
- test converting a column and adding new indexes at the same time (the $new_keys parameter)

andypost’s picture

Issue tags: +Needs tests

Also constraints should be renamed when fields are renamed.

jhedstrom’s picture

I'll write the tests outlined in #13.

jhedstrom’s picture

test converting a string column to int (and verify that the data actually makes sense)

In what context would converting from string to int ever preserve sensible data? Int to string should preserve data, but the other way around won't ever retain information unless only numbers are stored in the string field.

andypost’s picture

Suppose when converting string to int the column data should be replaced with default value.
This better to implement with drop and create but take into account an indexes and constraints

jhedstrom’s picture

This better to implement with drop and create but take into account an indexes and constraints

So in the specific case of string to int, it seems better to not code complex logic into db_change_field for an edge case, but simply use db_add_field and db_drop_field in sequence.

jbrown’s picture

subscribing

catch’s picture

Josh Waihi’s picture

subscribing

jbrown’s picture

no longer here 811434’s picture

subscribing

kecsi’s picture

FYI: I made a manual upgrade test from D6.17 site using PostgreSQL 8.3.11 to D7alpha6 + PATCH pgsql.761976.patch from #9 and my test failed with the following error message:

PDOException: SQLSTATE[55000]: Object not in prerequisite state: 7 ERROR: currval of sequence "sequences_value_seq" is not yet defined in this session: INSERT INTO sequences (value) VALUES (:db_insert_placeholder_0); Array ( ) in update_fix_d7_requirements() (line 572 of .../includes/update.inc).

Earlier error message I reported in #853688: upgrade of D6.17 to D7.0alpha6 failed using postgreSQL disappeared.

Josh Waihi’s picture

@kecsi that is strange. The only place CURRVAL is called is in /includes/database.pgsql.inc. That isn't even apart of D7. It doesn't make sense that that error is even possible.

Stevel’s picture

I'll write the tests outlined in #13.

@jhedstrom: how are the tests progressing?

jhedstrom’s picture

Assigned: jhedstrom » Unassigned

@stevel I've been fairly busy since Drupalcon, and lost momentum on the string->int test mentioned above.

chx’s picture

Edit; nevermind. I did not read far enough.

Damien Tournoud’s picture

We now have some pretty decent test coverage for the schema modification feature, at least indirectly, thanks to the upgrade path tests. If PostgreSQL passes the upgrade path tests with that, I would be happy. Can anyone confirm this?

Berdir’s picture

Assigned: Unassigned » Berdir

I'm working on this (Actually, I'm trying to get the upgrade tests working as requested in #29)

Berdir’s picture

FileSize
1.55 KB
PASSED: [[SimpleTest]]: [MySQL] 23,234 pass(es). View

Ok, short answer, the patch doesn't work. there seem to be several issues...

- The upgrade test doesn't even start because there is an issue with using currval()/nextval() as already reported in #24. I can fix that and I will do so in a separate issue.

Now, the current changeField() function seems to have several flaws (there might be even more, the upgrade path stops at some point, pretty early I think...)

- There was a small typo, a missing '"' in one of the ALTER queries. Fixed in the updated patch
- When running the upgrade path, I do now get several failures. Some examples (my postgresql install displays german errors, can someone tell me how to change that to english? I tried to translate them..)

- Casting to bytea from blob does not work:

Failed: PDOException: SQLSTATE[42804]: Datatype mismatch: 7 FEHLER: Spalte »data« kann nicht in Typ bytea umgewandelt werden: ALTER TABLE {users} ALTER data TYPE bytea; Array ( ) in db_change_field() (line 2869 of includes/database/database.inc).

Failed: PDOException: SQLSTATE[42804]: Datatype mismatch: 7 FEHLER: Spalte »variables« kann nicht in Typ bytea umgewandelt werden: ALTER TABLE {watchdog} ALTER variables TYPE bytea; Array ( ) in db_change_field() (line 2869 of includes/database/database.inc).

- No clue what's going on here exactly, strange syntax error, what's the ^ doing there??:

Failed: PDOException: SQLSTATE[42601]: Syntax error: 7 FEHLER: Syntaxfehler bei »-« LINE 1: ALTER TABLE asdfblock ALTER COLUMN e-mail TYPE bigint ^: ALTER TABLE asdfblock ALTER COLUMN e-mail TYPE bigint; Array ( ) in system_update_7016() (line 2003 of modules/system/system.install).

- I'm also having tons of notices and follow-up fails but I think that's because of the interrupted upgrade.

Regarding the cast issue, I found this: http://www.postgresonline.com/journal/index.php?/archives/29-How-to-conv...

If I'm understanding this right, we would have to write a function than can cast data to bytea and then use that USING stuff. But I don't know enough about PostgreSQL to do that... :)

jhedstrom’s picture

The error mentioned in #24 appears to be stemming from the same currval PDO issue reported and fixed here: #445214: dwr() fails when a NULL value is passed in a serial column (on PostgreSQL and SQLite)

jhedstrom’s picture

And after looking a bit more, what's really causing the issue in #24 is currval gets called, but nextval is never called because the bids in drupal-6.bare.database.php are manually being set. Possible solutions are to call nextId on each table using a sequence to initialize currval, or to not manually set the bids (and other primary keys--I tested this by removing the manual primary key inserts from drupal-6.bare.database.php, and this does work), or to put in a more general fix in database.inc.

Berdir’s picture

Yes, it is, but the linked issue only fixes it when dwr() is used and not when using db_insert(). It also doesn't update the sequence, see #890090: Inserting into a just created table fails when trying to set a value to a serial .

Next problem mine field is http://api.drupal.org/api/function/system_update_7016/7. a) It doesn't escape field names, so it fails on the "e-mail" field (second error mentioned in #24) and b) it converts all tables it can find in the database.. that could be a problem if you have a d6 and a d7 database in the same database I'm guessing. I guess that needs another issue, I have a working patch for a)

Berdir’s picture

Assigned: Berdir » Unassigned

I fear this is above my head, I have no clue on how to solve the datatype mismatch issue.

Someone can help out here?

Damien Tournoud’s picture

On cursory testing, it seems that:

ALTER TABLE $table ALTER COLUMN $name TYPE bytea USING value::bytea;

Works decently correctly, but only from [something] to bytea. The conversion *from* bytea doesn't seem to work the way I would expect it to.

Berdir’s picture

That seems to work, at least I do get new errors now :)

However, most probably not related to this issue directly:

Failed: PDOException: SQLSTATE[42883]: Undefined function: 7 FEHLER: Operator existiert nicht: character varying <> integer LINE 1: ...OUNT(*) FROM summit_simpletest816888users WHERE picture <> 0 ^ HINT: Kein Operator stimmt mit dem angegebenen Namen und den Argumenttypen überein. Sie müssen möglicherweise ausdrückliche Typumwandlungen hinzufügen.: SELECT COUNT(*) FROM {users} WHERE picture <> 0; Array ( ) in user_update_7012() (line 693 of modules/user/user.install).	

Failed: PDOException: SQLSTATE[42704]: Undefined object: 7 FEHLER: Typ »smallint_unsigned« existiert nicht: DROP DOMAIN smallint_unsigned; Array ( ) in system_update_7016() (line 2049 of modules/system/system.install).

Looking into to these. If you can help, ping me in #irc or visit me in the core sprint room at drupalcon :)

Berdir’s picture

Assigned: Unassigned » Berdir
Status: Needs work » Needs review
FileSize
1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 23,245 pass(es). View

Yay!

Together with #890090: Inserting into a just created table fails when trying to set a value to a serial and the fixes for the two errors above (will fill a separate issue for those, trivial stuff...), the upgrade tests pass on PostgreSQL...

Attaching the current patch, probably not yet ready though...

Berdir’s picture

FileSize
1.62 KB
PASSED: [[SimpleTest]]: [MySQL] 23,298 pass(es). View

I figured out that the patch actually fails on the existing Schema API tests. Reason are totally wrong quotes on the nextval() default value..

Attaching a new version...

Status: Needs review » Needs work
Issue tags: -Needs tests

The last submitted patch, postgresql_changefield3.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests

#39: postgresql_changefield3.patch queued for re-testing.

jhedstrom’s picture

For the operator error in #37, the issue is that the query in user_update_7012() should be:

SELECT COUNT(*) FROM {users} WHERE picture <> ''

instead of

SELECT COUNT(*) FROM {users} WHERE picture <> 0

since the field type at the time of that query is still a varchar.

Not sure if this should go into a separate critical issue, or if that change can be rolled into this patch.

Berdir’s picture

Already fixed and committed through another issue :)

aspilicious’s picture

? What berdir? is this issue fixed?

Berdir’s picture

Sorry, I wasn't clear. #43 was a response to #42, this issue still needs to be fixed.

tpfeiffer’s picture

FileSize
1.87 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch pgsql_schema.inc_.diff. View
2.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch simpletest_tests_schema.diff. View

I've extended the tests in modules/simpletest/tests/schema.test to check for some more problems while changing the database schema. A number of issues I've already fixed in the include/database/pgsql/schema.inc file, some others are yet to come. We should consider which one of these (like dropping a primary key constraint or changing from serial back to normal int) are really necessary or will actually never happen.

Status: Needs review » Needs work

The last submitted patch, pgsql_schema.inc_.diff, failed testing.

Berdir’s picture

Any chance that you can roll a proper patch for both files?

You already have a git checkout, just use "git diff --no-prefix".

Josh Waihi’s picture

otherwise you can use 'diff -up'

tpfeiffer’s picture

FileSize
6.46 KB
FAILED: [[SimpleTest]]: [MySQL] 23,302 pass(es), 0 fail(s), and 1 exception(es). View

OK, sorry for the crappy patches, I was in a hurry. Attached you find the patch created with git diff. I don't know how often schema changes actually happen in the Drupal code, but to call the schema alteration functionality "complete", a lot is still missing, like "set/drop NOT NULL" for columns, "drop PK constraint" etc.; in particular one needs to pay attention that indexes and sequences are properly renamed, cf. #826552. Unfortunately, right now, I don't have the time to work on this further, but I propose to lower the priority as soon as all schema changes that appear in core work fine.

aspilicious’s picture

Status: Needs work » Needs review

Putting the status to needs review, starts the testbot.
Thnx for the patch.

Status: Needs review » Needs work

The last submitted patch, postgresql_changefield4.patch, failed testing.

Berdir’s picture

The fail raises a valid point...

Is there any reason why we try to support multiple serial fields in PostgreSQL, given that there can be only one in MySQL?

Berdir’s picture

Ok, the test doesn't actually add a second AUTO_INCREMENT column, it renames the existing and changes it to AUTO_INCREMENT at the same point. Looks like that's not supported on MySQL either....

No, that wasn't correct. The problem is actually the second part of the error. We need to define a primary key (or any other key would work too) on the column before it can be defined as AUTO_INCREMENT.

So, the changeField() implementation of MySQL is incomplete as well and can't handle this case either. Not sure what to do.

Berdir’s picture

Working on the tests, I'm having a hard time to figure out what's a bug and what would be a new feature...

The above isn't correct either, because the documentation of db_change_field() states that you need to define the primary key when changing a column to serial. And it also states that all indexes need to be removed before a column is changed.

The second part is actually not necessary any more with this patch but it's still necessary to keep the index names in sync with the hook_schema() if they match the name of the field. For example, if you have a field named user_id and a PRIMARY KEY on it, you can rename it to uid and the PRIMARY KEY will be updated to use that field... but still be called user_id.

One of the issues I see is if you for example want to change the name of a serial field. The following does not work on MySQL (but works fine on postgresql and sqlite):

db_drop_primary_key('test_table');
// Rename test_serial to test_serial2.
db_change_field('test_table', 'test_serial', 'test_serial2', array('type' => 'serial', 'not null' => TRUE, 'description' => 'Changed column description.'));

This doesn't work because it is not allowed to remove they key on the auto_increment column.

This doesn't work at all:

// Rename test_serial to test_serial2.
db_change_field('test_table', 'test_serial', 'test_serial2', array('type' => 'serial', 'not null' => TRUE, 'description' => 'Changed column description.'), array('primary key' => array('test_serial2')));

That tries to create a second primary key and that doesn't work.

This does work, but it doesn't rename the primary key:

// Rename test_serial to test_serial2.
db_change_field('test_table', 'test_serial', 'test_serial2', array('type' => 'serial', 'not null' => TRUE, 'description' => 'Changed column description.'));

It seems the only proper way to do it would be the following:
1. switch column to int
2. remove indexes
3. change column like you wanted
4. Re-add indexes (can be part of 3)
5. Change column back to serial

This is not something that happens often but still, it could happen and the above is rather complex...

Can I safely assume that the current MySQL driver works correct and everything that would need to be extended there would be a new feature? Or should we broad the scope of this issue and make it about bringing all the different changeField() implementations to the same (probably higher than current) level?

Berdir’s picture

Status: Needs work » Needs review
FileSize
1.95 KB
PASSED: [[SimpleTest]]: [MySQL] 23,322 pass(es). View

I talked with DamZ about this and we agreed that the critical part of this issue is to get the upgrade tests passing on PostgreSQL.

This is already the case, I just verified again: "311 passes, 0 fails, and 0 exceptions".

We can downgrade this issue or create a separate one as soon as the critical part is committed. Re-rolled the patch without test changes and improved comments.

Berdir’s picture

FileSize
2.11 KB
PASSED: [[SimpleTest]]: [MySQL] 23,318 pass(es). View

Made the "is serial field" condition a bit easier to read...

gustavb’s picture

(I'm not that familiar with the database layer, so sorry if this is irrelevant...)

I get a feeling that the column names aren't properly quoted here:

+      $this->connection->query("SELECT setval('" . $seq . "', MAX(" . $field . ")) FROM {" . $table . "}");
+      $this->connection->query('COMMENT ON COLUMN {' . $table . '}.' . $field_new . ' IS ' . $this->prepareComment($spec['description']));

Shouldn't this be:

+      $this->connection->query("SELECT setval('" . $seq . '\', MAX("' . $field . '")) FROM {' . $table . '}');
+      $this->connection->query('COMMENT ON COLUMN {' . $table . '}."' . $field_new . '" IS ' .  $this->prepareComment($spec['description']));

? The other queries put quotes ("...") around field names.

gustavb’s picture

double submit, sorry.

Berdir’s picture

FileSize
2.12 KB
PASSED: [[SimpleTest]]: [MySQL] 23,318 pass(es). View

Yes, you are right. Note that we currently only escape fields in schema functions.

Josh Waihi’s picture

Status: Needs review » Reviewed & tested by the community

That looks good to me!

webchick’s picture

What happened to the expanded test coverage from earlier patches?

Berdir’s picture

Since you probably missed my response in IRC:
See #56. Basically, we (I/DamZ) think that the critical part of this patch is getting the existing (upgrade) tests working on PostgreSQL. Once that is commited, we can degrade this issue and continue working on these tests. Because when I worked on the tests, I actually wasn't sure anymore where to put the line between bugs/new features. For example, converting a varchar column to int, or how you're supposed to rename a serial field...

So IMHO the *tests* need more discussion but they don't need to hold the actualy bugfixing patch back.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, fair enough.

Committed to HEAD. Thanks! Not sure if we should re-purpose this issue for tests or start a new one, but for now, marking this one as fixed.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

moshe weitzman’s picture

hey. remember the tests!

Rob van den Bogaard’s picture

Sorry for not being very active since posting the problem: But a heap of thanks for the work! The more because I'm a big Postgres fan. ;)