maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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 <thiru@xxxxxxxxxxx>
> > > committer: Thirunarayanan Balathandayuthapani <thiru@xxxxxxxxxxx>
> > > 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@xxxxxxxxxxx