Defaults not changed in changeField on postgresql

1) With "upgrade", do you mean upgrading from D6 or merely an earlier version of D7?

2) On which database is this? Doesn't look like MySQL?

It's possible that you found a Drupal core bug -- in which case this issue should be moved into the core queue.

The module update tries to perform the following (which works as intended on MySQL):

  // Change {mollom}.quality from varchar into float.
  // Since some data rows are using an empty string to denote no 'quality'
  // value, we need to insert a temporary value that is converted to NULL
  // afterwards.
  db_update('mollom')
    ->fields(array('quality' => 9))
    ->condition('quality', '')
    ->execute();
  db_change_field('mollom', 'quality', 'quality', array(
    'description' => 'Text analysis quality check result.',
    'type' => 'float',
    'size' => 'tiny',
    'not null' => FALSE,
  ));
  db_update('mollom')
    ->fields(array('quality' => NULL))
    ->condition('quality', 9)
    ->execute();

The db_change_field() leads to the error message:

PDOException: SQLSTATE[42804]: Datatype mismatch: 7 ERROR: default for column "quality" cannot be cast to type real:

ALTER TABLE {mollom} ALTER "quality" TYPE real USING "quality"::real;

Looking through the postgres driver, I see that code in DatabaseSchema_pgsql::changeField():

    $map = $this->getFieldTypeMap();
    $typecast = $map[$spec['type'] . ':' . $spec['size']];
    if (in_array($typecast, array('serial', 'bigserial', 'numeric'))) {
      $typecast = 'int';
    }
    $this->connection->query('ALTER TABLE {' . $table . '} ALTER "' . $field . '" TYPE ' . $typecast . ' USING "' . $field . '"::' . $typecast);

But based on the error message, it looks like neither ::createFieldSql() nor ::fieldSetNoDefault() is invoked from ::changeField().

In other words:

1) A previously specified 'default' is not removed or changed prior to changing the column type.

2) Somehow, the passed in $spec and ::createFieldSql() is not invoked or used, so I wonder how more subtle column differences are actually applied on PostgreSQL? E.g., nothing seems to happen when varchar(64) is changed to varchar(255)...?

doh! Subscribing this so I can come back to this when I have some more time.

How is this for changing defaults?

The second remark from sun was fixed in #951116: db_change_field() fails to convert int to varchar on PostgreSQL I believe.

New patch to account for 0, false and other 'empty' variables as default values.

#6: 935258-change-default-nulls.patch queued for re-testing.

Well, how's that... A change in the DatabaseSchema for Postgresql causes test failures on mysql...

Changing title to reflect the actual bug. Also raising to major there are contrib modules (e.g. mollom) for which the upgrade path fails because of this.

+++ includes/database/pgsql/schema.inc	30 Dec 2010 11:59:18 -0000
@@ -562,6 +565,11 @@ class DatabaseSchema_pgsql extends Datab
+    if (isset($spec['default']) || array_key_exists($spec['default'])) {

That's not the right syntax for array_key_exists(). See:
http://www.php.net/array_key_exists

I think array_key_exists('default', $spec) should be sufficient.

Powered by Dreditor.

Updated the patch. The isset || array_key_exists seems to be a common pattern in drupal core for efficienty reasons (as far as I understood it).

+++ includes/database/pgsql/schema.inc	30 Dec 2010 11:59:18 -0000
@@ -528,6 +528,9 @@ class DatabaseSchema_pgsql extends Datab
+    // Remove old default

Missing trailing period.

+++ includes/database/pgsql/schema.inc	30 Dec 2010 11:59:18 -0000
@@ -562,6 +565,11 @@ class DatabaseSchema_pgsql extends Datab
+    // Add default if necessary.
+    if (isset($spec['default']) || array_key_exists('default', $spec)) {
+      $this->fieldSetDefault($table, $field, $spec['default']);
+    }

Actually, I don't think that NULL is a valid value for 'default', as that would translate into 'not null' => FALSE instead. Therefore, the isset() should be sufficient.

Powered by Dreditor.

Yes, you are right. NULL is the default default (twice, yes), so we don't have to set a default explicitly if it is NULL. Updated patch again.

Unless @Crell disagrees, this looks ready to fly for me.

We need someone to test the pgsql upgrade path.

I get as far as visting update.php and get the following error:
Update:
DatabaseSchemaObjectDoesNotExistException: Cannot set default value of field actions.description: field doesn't exist. in DatabaseSchema_pgsql->fieldSetDefault() (line 397 of /home/josh/drupal/935258/includes/database/pgsql/schema.inc).

Are you sure you don't get the error without the patch applied?

How are indexes related to default values? This belongs in it's own issue I think (is this the same bug as #935570: Cannot add "system list" because "system" already exists?)

sorry, that was the wrong error (is was after I refreshed). Its actually:
DatabaseSchemaObjectDoesNotExistException: Cannot set default value of field actions.description: field doesn't exist. in DatabaseSchema_pgsql->fieldSetDefault() (line 397 of /home/josh/drupal/935258/includes/database/pgsql/schema.inc).

Was a php variable error. $field was being used after $field had been renamed to $new_field.

doh, of course :) If you don't have any further comments, I guess this is RTBC then.

I don't have PostgreSQL to test on, but decided to commit this to 8.x and 7.x. Thanks.

Thanks Dries :)