DatabaseSchema_pgsql::changeField() is broken

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

Promoting to critical.

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.

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

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

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.

+1 it works, tested on system Update #7051

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

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

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

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)

Also constraints should be renamed when fields are renamed.

I'll write the tests outlined in #13.

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.

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

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.

subscribing

#826562: db_change_field doesn't isn't right on PostgreSQL was duplicate.

subscribing

This is blocking #690746: Text column type doesn't reliably hold serialized variables.

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.

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

I'll write the tests outlined in #13.

@jhedstrom: how are the tests progressing?

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

Edit; nevermind. I did not read far enough.

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?

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

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

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)

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.

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)

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

Someone can help out here?

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.

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 :)

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

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

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

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.

Already fixed and committed through another issue :)

? What berdir? is this issue fixed?

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

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.

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

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

otherwise you can use 'diff -up'

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.

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

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?

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.

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?

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.

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

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

double submit, sorry.

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

That looks good to me!

What happened to the expanded test coverage from earlier patches?

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.

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.

hey. remember the tests!