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 committer: Thirunarayanan Balathandayuthapani 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 security@mariadb.org