Cannot define PRIMARY KEY constraint on nullable column

At first I thought this would be easy to sort out, it looks like the __pk field, on creation should be set to NOT NULL (patch attached).

But still the update will not working, showing the same message and the __pk field appers to be nullable even after using the patched Schema.inc

After analysing the upgrade path, the "flagging" table comes from renaming the flag_content table, wich was created prior to aplying the Schema.inc Patch and thus has a __pk set to NULLABLE.

I believe the proposed patch is OK, but still tables created prior to the patch are, let's say, "wrong".

I can update the createTechnicalPrimaryColumn() method to check if the column is nullable and update it accordingly:

protected function createTechnicalPrimaryColumn($table) {
if (!$this->fieldExists($table, '__pk'))
{
$this->connection->query('ALTER TABLE {' . $table . '} ADD __pk UNIQUEIDENTIFIER DEFAULT NEWID() NOT NULL');
}
else
{
$this->connection->query('ALTER TABLE {' . $table . '} ALTER COLUMN __pk UNIQUEIDENTIFIER DEFAULT NEWID() NOT NULL');
}
}

But i'm not sure this is a good idea, any feedback is welcome.

To Upgrade the flag Module:

NOTE: There is no need to patch the driver in order to succesfully perform the update. Actually it must be patched or it will fail!

1. Put site in maintenance mode
2. * (optional) Disable de module before updating the code, otherwise you application is dead due to incompatible database structure
3. Update module source code
4.1 **Make sure that, if there is any column named "__pk" in table "flag_content" you set it to NOT NULL manually
4.2 Completely disable (comment) updates 7305 and 7306, for these updates to run properly the flag module must be enabled again, we will do that later.
5. ***Modify update 7303 in flag.install, comment lines 519 through 542:

// A serial field must be defined as a key, so make a temporary index on
// 'fcid' so we can safely drop the primary key.
// @see http://drupal.org/node/190027
db_add_index('flagging', 'temp', array('fcid'));
// Drop the primary key so we can rename the field.
db_drop_primary_key('flagging');

// Change field 'fcid' to 'flagging_id'.
db_change_field('flagging', 'fcid', 'flagging_id',
// Spec of the field. Identical to our current hook_schema(): we're not
// changing anything except the name.
array(
'description' => 'The unique ID for this particular tag.',
'type' => 'serial',
'unsigned' => TRUE,
'not null' => TRUE,
),
// Keys spec. Identical to current hook_schema().
array(
'primary key' => array('flagging_id'),
)
);
// Drop our temporary index.
db_drop_index('flagging', 'temp');

6. Run your update
7. Manually rename the column "fcid" to "flagging_id" in table "flagging", to be able to do so you must delete the linked constraint (CK__flag_conte__fcid__058EC7FB in my case) and recreate it after renaming with the script:

ALTER TABLE [dbo].[flagging] WITH CHECK ADD CHECK (([flagging_id]>=(0)))

8. Enable the flag module.
9. Uncomment what was commented in step 4.2
10. Run update again
11. You are ready to go!

COMMENTS:

* This is required at least when running with SQL Server, but I guess would do no harm on other databases. By the way, it helps make sure you have no dependencies on flag that get broken after the upgrade, such as Friend Flag.

** I cannot believe this has never showed up before, it looks as a problem within the driver itself because it is trying to set a PRIMARY KEY constraint on a NULLABLE COLUMN it has created itself (technical primary key AKA '__pk'). This was addressed in the patch from my first post, but that patch does not cope with the problem that some __pk columns have already been created without NOT NULL clausule. Yet, setting NOT NULL on the column programatically is not trivial because you have to drop and recreate all indexes, constraints and computed columns that depend on the __pk column (in this case both 3 things were actually happening...). I programmed a script to perform this change on all tables, but it is much easier to manually do it from SQL Management Studio.

*** This was a good one.... so after digging, trying and programming for many hours it turns out that it is imposible to perform (at least on SQL SERVER) a changeField with a target type of "serial" unless you drop and recreate the whole table. This is because serial type is treated as IDENTITY, and there is no way to perform UPDATE statements on IDENTITY fields, neither to apply/remove the IDENTITY property to an existing column.

I am proposing a patch that does not really solve any of the discussed issues, but will help make the driver more robust:

- Always use NOT NULL when creating Technical Primary Keys
- In changeField: check if target column type is "serial", if so, throw a descriptive exception
- In addField: check if target column type is "serial" and default vaules are provided, if so, throw a descriptive exception.

One of the issues just got diluted with the change of description:

It is imposible to perform (at least on SQL SERVER) a changeField with a target type of "serial" unless you drop and recreate the whole table. This is because serial type is treated as IDENTITY, and there is no way to perform UPDATE statements on IDENTITY fields, neither to apply/remove the IDENTITY property to an existing column.

Should this be moved to a new issue?