Package home | Report new bug | New search | Development Roadmap Status: Open | Feedback | All | Closed Since Version 0.8.6

Bug #11604 Primary Key is removed when updating a table against its XML
Submitted: 2007-07-13 19:51 UTC
From: floele Assigned: ifeghali
Status: Closed Package: MDB2_Schema
PHP Version: Irrelevant OS: Irrelevant
Roadmaps: (Not assigned)    
Subscription  


 [2007-07-13 19:51 UTC] floele (Florian Schmitz)
Description: ------------ Hi. Sorry to bother you again, but I found a second problem when upgrading a database. The following table (just one out of many) CREATE TABLE `flyspray_admin_requests` ( `request_id` int(5) NOT NULL auto_increment, `project_id` int(5) NOT NULL default '0', `task_id` int(5) NOT NULL default '0', `submitted_by` int(5) NOT NULL default '0', `request_type` int(2) NOT NULL default '0', `reason_given` text, `time_submitted` int(11) NOT NULL default '0', `resolved_by` int(5) NOT NULL default '0', `time_resolved` int(11) NOT NULL default '0', `deny_reason` text, PRIMARY KEY (`request_id`), KEY `reminders_taskid` (`task_id`), KEY `reminders_project` (`project_id`) ) results in the following XML (which is correct so far): <table> <name><variable>db_prefix</variable>admin_requests</name> <declaration> <field> <name>request_id</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> <autoincrement>1</autoincrement> </field> <field> <name>project_id</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>task_id</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>submitted_by</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>request_type</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>reason_given</name> <type>text</type> <notnull>false</notnull> </field> <field> <name>time_submitted</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>resolved_by</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>time_resolved</name> <type>integer</type> <length>4</length> <notnull>true</notnull> <default>0</default> </field> <field> <name>deny_reason</name> <type>text</type> <notnull>false</notnull> </field> <index> <name>reminders_taskid</name> <field> <name>task_id</name> <sorting>ascending</sorting> </field> </index> <index> <name>reminders_project</name> <field> <name>project_id</name> <sorting>ascending</sorting> </field> </index> <index> <name><variable>db_prefix</variable>admin_requests_pKey</name> <primary>true</primary> <field> <name>request_id</name> <sorting>ascending</sorting> </field> </index> </declaration> </table> However, when importing this XML and the table already exists (with $db->setOption('idxname_format', '%s') ) the following query is executed DROP INDEX primary ON flyspray_assigned which causes a syntax error and the upgrade is aborted. In fact dropping this key is not possible in MySQL anyway. If idxname_format is not set, it attempts to remove the key primary_idx, which is not found and upgrader aborts again. A workaround is to comment out line 1625 in Schema.php (0.7.2). Then it does not abort if an index could not be deleted. Any help or a fix if neccessary is appreciated. Test script: --------------- The importing script looks like this: $db->setOption('idxname_format', '%s'); $schema =& MDB2_Schema::factory($db); $schema->setOption('force_defaults', false); $previous_schema = $schema->getDefinitionFromDatabase(); $res = $schema->updateDatabase($file, $previous_schema, array('db_prefix' => $conf['database']['dbprefix'], 'db_name' => $conf['database']['dbname'])); Expected result: ---------------- MDB2 does not attempt to drop the primary index. Actual result: -------------- MDB2 attempts to remove a key called "primary" which results in a syntax error and is not possible anyway.

Comments

 [2007-07-16 01:47 UTC] ifeghali (Igor Feghali)
Have you tried with option "quote_identifier" set to TRUE ?
 [2007-07-16 07:26 UTC] floele (Florian Schmitz)
I tried that now, and as expected I get a similar error: DROP INDEX `primary` ON `flyspray_admin_requests`] [Native code: 1075] [Native message: Incorrect table definition; there can be only one auto column and it must be defined as a key] As I said before, the key cannot be deleted anyway.
 [2007-07-16 11:27 UTC] ifeghali (Igor Feghali)
Seems a MDB2 bug to me. Maybe the autoincrement should be dropped before the key ? Looping to Lorenzo to see what he thinks.
 [2007-07-16 12:05 UTC] floele (Florian Schmitz)
I still think that it shouldn't be dropped at all. Either only the whole table, for the other indices. But thanks for investigating regarding this issue.
 [2007-07-16 15:57 UTC] ifeghali (Igor Feghali)
Hello, I will take a deeper look at this in a few days. Anyway the primary key issue is also something that requires our attention. I see two bugs here. regards, iGor.
 [2007-07-20 08:54 UTC] quipo (Lorenzo Alberton)
Sorry, I haven't looked at the actual issue yet, but maybe the problem is you're trying to drop an *INDEX* with dropIndex() while you should drop the PK *constraint* with dropConstraint()...
 [2007-07-22 08:43 UTC] floele (Florian Schmitz)
I have investigated a little more to get this fixed a little faster. In alterDatabaseIndexes(), Schema.php is doing a loop foreach ($changes['remove'] as $index_name => $index) ... and then checks empty($index['primary']) to decide whether to drop a constraint or an index. However, $changes['remove'] only looks like this when var_dump'ed: array(2) { ["any_usual_index"]=> bool(true) ["primary"]=> bool(true) } So there is no 'primary' key in $index at any time. This again seems to be expected, if you consider the code foreach ($previous_definition as $index_previous_name => $index_previous) { if (empty($defined_indexes[$index_previous_name])) { $changes['remove'][$index_previous_name] = true; } } in compareTableIndexesDefinitions(). So a possible fix would be to replace 'true', so that it looks like this: foreach ($previous_definition as $index_previous_name => $index_previous) { if (empty($defined_indexes[$index_previous_name])) { $changes['remove'][$index_previous_name] = $index_previous; } } I still wonder why this should me an MDB2 instead of an MDB2_Schema bug.
 [2007-07-22 13:44 UTC] quipo (Lorenzo Alberton)
Thanks for the analysis, Florian. Moving bug report back to MDB2_Schema.
 [2007-07-22 14:34 UTC] ifeghali (Igor Feghali)
This bug has been fixed in CVS. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better.
 [2007-07-22 14:46 UTC] floele (Florian Schmitz)
Hm, I can confirm that the fix in CVS now generates the expected query (which is "ALTER TABLE xyz DROP PRIMARY KEY"), however, the actual problem (it attempts to delete the primary key) still remains. Now the error message is "Incorrect table definition; there can be only one auto column and it must be defined as a key". This happens because there still is an auto-increment column in the table which (as the message says) must be defined as primary key. So I'd say that unless there is a reason to delete the primary key, it shouldn't be deleted. Is a new bug required for that?
 [2007-07-22 15:03 UTC] floele (Florian Schmitz)
The proble is that the current / previous definitions in compareTableIndexesDefinitions are different. Only the latter one contains the "primary" key. Current: Array ( [reminders_taskid] => Array ( [fields] => Array ( [task_id] => Array ( [sorting] => ascending ) ) [was] => reminders_taskid ) [reminders_project] => Array ( [fields] => Array ( [project_id] => Array ( [sorting] => ascending ) ) [was] => reminders_project ) ) Previous: Array ( [reminders_taskid] => Array ( [fields] => Array ( [task_id] => Array ( [sorting] => ascending ) ) ) [reminders_project] => Array ( [fields] => Array ( [project_id] => Array ( [sorting] => ascending ) ) ) [primary] => Array ( [primary] => 1 [fields] => Array ( [request_id] => Array ( [sorting] => ascending ) ) ) )
 [2007-07-22 16:10 UTC] ifeghali (Igor Feghali)
Hello Florian, Yes that's why I said it is probably a MDB2 bug. Dropping a primay key shouldn't return an error, even though we are talking about MySQL. The dependencies should be handled by the MySQL driver. So you are trying to update a table, that doesn't have its primary key anymore. Right ? Or both schemas do have the PK but one of them are being dumped incorrectly ? Regards, iGor.
 [2007-07-22 17:18 UTC] floele (Florian Schmitz)
As you can see in my original comment, the table in the database still has the primary key. The table is dumped correctly when I export it as XML, but during database update there seems to be something wrong. I'm not quite sure where to look for the problem though. It might be that MDB2 needs to handle the dependencies for dropping a primary key, however, as I said multiple times before, there *is no need* to drop the key at all.
 [2007-07-22 17:27 UTC] floele (Florian Schmitz)
So in other words, the current-array from above does not contain the primary key (although it should).
 [2007-07-22 17:40 UTC] ifeghali (Igor Feghali)
Could you please provide the CREATE TABLE statement for both table schemas (previous and current) ? Or better yet, the XML of current schema and the var_dump of $previous_schema (only the section of the table in question). Thank you, iGor.
 [2007-07-22 17:54 UTC] floele (Florian Schmitz)
Current === Previous I'm actually updating the database without expecting anything to be changed. So the create table statement in my very first comment is both the current and previous table. Here is the content of $previous_schema in my example script: array(5) { ["name"]=> string(8) "clean099" ["create"]=> bool(true) ["overwrite"]=> bool(false) ["tables"]=> array(28) { ["flyspray_admin_requests"]=> &array(2) { ["fields"]=> array(10) { ["request_id"]=> array(8) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["autoincrement"]=> bool(true) ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["project_id"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["task_id"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["submitted_by"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["request_type"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["reason_given"]=> array(7) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["default"]=> NULL ["type"]=> string(4) "text" ["mdb2type"]=> string(4) "text" ["choices"]=> array(2) { [0]=> array(6) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["default"]=> NULL ["type"]=> string(4) "text" ["mdb2type"]=> string(4) "text" } [1]=> array(5) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["type"]=> string(4) "clob" ["mdb2type"]=> string(4) "clob" } } } ["time_submitted"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["resolved_by"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["time_resolved"]=> array(7) { ["notnull"]=> bool(true) ["nativetype"]=> string(3) "int" ["length"]=> int(4) ["unsigned"]=> int(0) ["default"]=> string(1) "0" ["type"]=> string(7) "integer" ["mdb2type"]=> string(7) "integer" } ["deny_reason"]=> array(7) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["default"]=> NULL ["type"]=> string(4) "text" ["mdb2type"]=> string(4) "text" ["choices"]=> array(2) { [0]=> array(6) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["default"]=> NULL ["type"]=> string(4) "text" ["mdb2type"]=> string(4) "text" } [1]=> array(5) { ["notnull"]=> bool(false) ["nativetype"]=> string(4) "text" ["fixed"]=> bool(false) ["type"]=> string(4) "clob" ["mdb2type"]=> string(4) "clob" } } } } ["indexes"]=> array(3) { ["reminders_taskid"]=> array(1) { ["fields"]=> array(1) { ["task_id"]=> array(1) { ["sorting"]=> string(9) "ascending" } } } ["reminders_project"]=> array(1) { ["fields"]=> array(1) { ["project_id"]=> array(1) { ["sorting"]=> string(9) "ascending" } } } ["primary"]=> array(2) { ["primary"]=> bool(true) ["fields"]=> array(1) { ["request_id"]=> array(1) { ["sorting"]=> string(9) "ascending" } } } } }
 [2007-07-22 19:48 UTC] ifeghali (Igor Feghali)
This is a collateral effect of validateTable(). Since we have an autoincrement field, validateTable() skip the primary key when creating the definition array. This is a desirable behaviour when creating a table but now it is clear that it can be a problem when updating tables. Working on the fix.
 [2007-07-23 23:05 UTC] ifeghali (Igor Feghali)
This bug has been fixed in CVS. If this was a documentation problem, the fix will appear on pear.php.net by the end of next Sunday (CET). If this was a problem with the pear.php.net website, the change should be live shortly. Otherwise, the fix will appear in the package's next release. Thank you for the report and for helping us make PEAR better. please let me know if you still have any issue.
 [2007-07-24 08:03 UTC] floele (Florian Schmitz)
It works fine now, thanks.