system_update_6033 fix for pgsql

Yes, it seems that field is redundant -- doesn't make sense at all. +1

Easy.

Good catch! Committed.

This patch fails schema.module testing. On pgsql, the committed patch implicitly (via db_change_field) drops the primary key on node_comment_statistics without re-creating it, resulting in the table having no primary key; obviously, this is a problem.

Fixing this on pgsql is easy: db_drop_primary_key() then db_change_field() then db_add_primary_key(), like we do in many other places.

Unfortunately, that won't work on mysql because we CANNOT drop the primary key! Prior to this patch, nid is auto-increment. On mysql, auto-increment fields must be included in at least one key or index. In this table the primary key is the only index nid is involved in, so we can't drop it until after we call db_change_field(). But on pgsql, after we call db_change_field() is the pkey is already dropped so calling db_drop_primary_key() again will result in an error.

There are two solutions, and both suck:

1. Don't call db_drop_primary_key() at all. After db_change_field(), only call db_add_primary_key() if $db_type == 'pgsql'. This sucks because it puts a $db_type dependency back into the code.

2. Create a temporary index on nid so we can drop the pkey:

db_add_index('node_comment_statistics', 'ncs_nid_idx', array('nid'))
db_drop_primary_key()
db_change_field()
db_add_primary_key()
db_drop_index('ncs_nid_idx')

This sucks because it is extra (possibly time-consuming on a large site) database work to accomplish a goal that can be done more simply on either db individually. And, while it does not reference $db_type explicitly, the fact is that it is written specifically to accommodate one particular db (mysql).

I suggest the first solution and have attached a patch that uses it. Tested on both mysql and pgsql, schema.module reports no conflicts.

PS: Yes, this little snafu will strengthen chx's opinion that we should drop support for non-mysql databases, increase my concern that the switch to using auto-incrementing columns (which I advocated) was a mistake, and make everyone wish that we already had an automated test suite to catch these things. :-)

Well, as core only supports mysql and pgsql still, I think this is a reasonable compromise. I'd love if someone else could also test this though.

My patch in #4 still applies to head but with a large offset; I've re-rolled it.

I tested it again on pgsql which is where the bug occurs and the only $db_type affected, and it works fine. Here is the update log showing the new query:

Update #6033

* ALTER TABLE {node_comment_statistics} RENAME nid TO nid_old
* ALTER TABLE {node_comment_statistics} ADD COLUMN nid int_unsigned default 0
* UPDATE {node_comment_statistics} SET nid = nid_old
* ALTER TABLE {node_comment_statistics} ALTER nid SET NOT NULL
* ALTER TABLE {node_comment_statistics} DROP COLUMN nid_old
* ALTER TABLE {node_comment_statistics} ADD PRIMARY KEY (nid)

Following this upgrade, schema.module reports 0 inconsistent tables.

Of course, this is my patch so someone else should review it.

Great work spotting and patching! I've tested the patch and it works. Even though it works I believe this can be done in a better way to handle this situation.

I reckon the change needs to be made to the DB abstraction (schema) code (i.e db_change_field()). We want to avoid having this problem creep up with third-party modules don't we? Marking as patch needs work.

Another thing to note is the index for last_comment_timestamp has a name that doesn't adhere to the index naming convention. We'd need to patch that in 5 and change the schema in 6 to suit. Might as well tackle that in the same issue. I'm happy to review it.

--
Sammy Spets
Synerger
http://synerger.com

Just remembered one other thing. Shouldn't updates to node_comment_statistics be placed in comment.install rather than system.install?

Sammy, thanks for the review. However, this issue is not about fixing the Schema API to elegantly handle changing a column from type serial to something else while being part of the primary key. I re-opened this issue because, as core currently stands, a PostgreSQL site upgraded from D5 to D6 will have no primary key on the node_comment_statistics table which will absolutely kill the performance of node_load.

Making Schema API handle this situation well, if even possible, will require at the very least a new dedicated function and possibly substantial changes to the way Schema API works (e.g. database introspection in core which is NOT happening in D6). I explained the difficulties in #4. We cannot ship D6 with no workable upgrade path from D5 and my most recent patch provides that path. Saying that we need to find a possibly non-existent change to Schema API during code-freeze before D6 can be released just isn't practical.

I created a new issue regarding this limitation of Schema API so we can fix it for D7: http://drupal.org/node/190027.

Regarding the index name, that is not a critical issue but the missing primary key is. Now that indexes are given explicit names in the schema structure, it isn't that important that they follow the same naming convention. I certainly would support a patch to fix it if you submitted it.

Regarding your question about putting the update in comment.install, I agree with you but for D6 all core table changes are happening in system.install.

Finally, you said that you tested the patch and that it works so I'm marking this RTBC.

As I said, I think this is a reasonable compromise for now, so committed, thanks. D7 might see a better API-wise solution.

Compromise ok. Since I haven't said it yet, good work on Schema API Barry. :)