Hi, Marko! On Jan 17, Marko Mäkelä wrote: > Hi Sergei, > > On January 16, 2019, Sergei Golubchik wrote: > > On Jan 16, Thirunarayanan Balathandayuthapani wrote: > > > revision-id: d08031dfae2 (mariadb-10.0.37-45-gd08031dfae2) > > > parent(s): 12f362c3338 > > > author: Thirunarayanan Balathandayuthapani <th...@mariadb.com> > > > committer: Thirunarayanan Balathandayuthapani <th...@mariadb.com> > > > timestamp: 2019-01-16 15:46:28 +0530 > > > message: > > > > > > MDEV-17376 Server fails to set ADD_PK_INDEX, DROP_PK_INDEX if unique > > > index nominated as PK > > > > Sorry, that's way too complex. I mean the original code with > > candidate_key_count, is_candidate_key, and all that. > > And you're making it more complex. > > Yes, it is complex, but this patch already went through some rounds of > my review, and this was the best we could come up without a major > refactoring.
I didn't mean the the patch was complex, it only builds on top of the existing code. I found the existing logic around "candidate keys" quite convoluted and completely unnecessary. > > If a unique key is promoted to a primary key, this unique key is > > always be the first key in the table. > > According to some comments in InnoDB, this might not have been the > case in tables that were created in MySQL 3.23. But that would not be > the first time when a comment in InnoDB source code has been > inaccurate or incorrect. > As far as we can tell, table->s->primary_key is always either 0 or > MAX_KEY. Do you know if this has ever been different earlier? I'm not sure. I see that even 3.23 sorted unique indexes first. But later it uses a loop to find table->s->primary_key. Which doesn't mean anything, because even now in 10.4 there's lots of code redundant like if (table->s->primary_key >= MAX_KEY) while one primary_key can only be 0 or MAX_KEY. I cannot say whether that loop in 3.23 is needed or yet another case of primary_key redundancy. > > So what you need to check is > > * if the old table didn't have a primary key and the new table has it, > > then a primary key was added. As easy as > > > > if (table->s->primary_key >= MAX_KEY && > > altered_table->s->primary_key != MAX_KEY) > > It would be as easy as that if altered_table had already been > constructed. But it seems to me that it would be constructed based on > the output of this function. > Perhaps we should set the flags somewhat later based on comparing > table and altered_table? altered_table is opened (a TABLE object is created) after fill_alter_inplace_info() and it doesn't use the result of fill_alter_inplace_info(). But the frm file is created before. In create_table_impl(). There are three options to do what I suggested * move this check for a changed primary key out of fill_alter_inplace_info(), after altered_table is opened * open altered_table before fill_alter_inplace_info * don't use altered_table->s->primary_key, but derive it from key_info[] array (which shows keys as written in the new frm file). So instead of altered_table->s->primary_key one would use (key_info->flags & HA_NOSAME && !(key_info->flags & HA_NULL_PART_KEY) && !(key_info->flags & HA_KEY_HAS_PART_KEY_SEG)) ? 0 : MAX_KEY again, just as that if() with altered_table->s->primary_key, this expression is only to explain the idea, not something that should be copy-pasted verbatim into the patch :) I think, out of the three options above, the third one is the best Regards, Sergei Chief Architect MariaDB and secur...@mariadb.org _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp