maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Nikita!

On Jan 13, Nikita Malyavin wrote:
> Hello! Thanks for the review
> 
> First commit is reworded, and new changes are added on top.
> Also, I am reverting "handler: remove store/restore record from
> check_duplicate_long_entry_key", where I was trying to reflect
> handler->position fix to unique blobs. The reason is that
> main.long_unique_bugs test fails, and I did not find a quick solution for
> that :(

Monty is rewriting the code around long uniques and the update_handler.
We'll have to wait until he's done.

> On Tue, 17 Dec 2019 at 21:14, Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> 
> > > diff --git a/sql/handler.cc b/sql/handler.cc
> > > index 46a0c313c80..ed36f3c5bd3 100644
> > > --- a/sql/handler.cc
> > > +++ b/sql/handler.cc
> > > @@ -6402,6 +6407,14 @@ int handler::ha_external_lock(THD *thd, int lock_type)
> > >        mysql_audit_external_lock(thd, table_share, lock_type);
> > >    }
> > >
> > > +  if (lock_type == F_UNLCK && check_overlaps_handler)
> > > +  {
> > > +    check_overlaps_handler->ha_external_lock(table->in_use, F_UNLCK);
> > > +    check_overlaps_handler->close();
> > > +    check_overlaps_handler= NULL;
> > > +    overlaps_error_key= -1;
> > > +  }
> >
> > I'm still thinking about how avoid this overhead or at least to
> > simplify the code.
> >
> > One option is to use HA_EXTRA_REMEMBER_POS
> > It doesn't nest, you're right, but it can be fixed.
> >
> > Another, simpler, option is to use TABLE::update_handler.  This is a
> > second auxillary handler, a clone, exactly as yours, created for to
> > check long uniques. So I don't see a need to create a yet another
> > clone when you can simply use TABLE::update_handler. It's never used
> > for scanning, only for point lookups, so there is no position that
> > you can disrupt.
> >
> > upd: you seem to have got the same idea. and you're right, it should
> > be in the handler class, not in the TABLE as I originally wanted.
> 
> We operate with top-level handlers only, for partitioning. In this sense
> TABLE is better -- but it is a bad composition. This handler is only used
> in other handler, and never used in TABLE.

Okay. I think Monty has moved update_handler from TABLE to the handler.

> I have moved handler cloning code into handler class, and renamed the
> field to `lookup_handler`, but didn't touch the memory buffer. We use
> different sizes there (I need additional memory for key storage).
> Maybe it's better to make a common buffer as well, and allocate a
> maximum, i.e.  reclength+keylength?

I guess so. But let's wait with this unification until Monty is done
with his refactoring.

> > diff --git a/sql/table.cc b/sql/table.cc
> > > index 7ed5121a9c6..8b84fb3035d 100644
> > > --- a/sql/table.cc
> > > +++ b/sql/table.cc
> > @@ -1726,11 +1725,26 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
> > >    keyinfo= &first_keyinfo;
> > >    thd->mem_root= &share->mem_root;
> > >
> > > +  auto err= [thd, share, &handler_file, &se_plugin, old_root](){
> > > +    share->db_plugin= NULL;
> > > +    share->error= OPEN_FRM_CORRUPTED;
> > > +    share->open_errno= my_errno;
> > > +    delete handler_file;
> > > +    plugin_unlock(0, se_plugin);
> > > +    my_hash_free(&share->name_hash);
> > > +
> > > +    if (!thd->is_error())
> > > +      open_table_error(share, OPEN_FRM_CORRUPTED, share->open_errno);
> > > +
> > > +    thd->mem_root= old_root;
> > > +    return HA_ERR_NOT_A_TABLE;
> > > +  };
> >
> > Revert that too
> 
> I have a problem that all the time i'm debugging corrupted frm's, or
> anything else with lots of goto err's, the second thing I am doing (the
> first one is I'm breaking on err: label and making sure that no telepathy
> today i have) is rewriting goto's to function calls, to have an additional
> stack frame, to see where is err() called from. This is why I want this
> patch so much -- because I'm suffering all the time I face this kind of
> problem. Same to REPLACE code (see write_row), and other places with over
> 9K gotos.

I just put a breakpoint on every 'goto err' inside init_from_binary_frm_image.

But ok, I agree that this one has its merits. Good that you have it in a
separate commit. But please add the justification to the commit comment.
That is, it's to simplify debugging, one can put a breakpoint on
open_table_error(), for example, and go up the stack to see where
exactly in init_from_binary_frm_image() the error was discovered.
Unlike goto, that you cannot go backward and see what goto has jumped to
the err: label.

> > > +  /*
> > > +   @return -1,    lhs precedes rhs
> > > +            0,    lhs overlaps rhs
> > > +            1,    lhs succeeds rhs
> > > +   */
> >
> > ah, a comment, good :) but move it to the function definition, please
> 
> Okey. Still do not understand, why not in headers

because jumping to the tag jumps to the definition.
because when looking at the function definition one usually wants to
know what it does and what it returns.

> > > diff --git a/sql/unireg.h b/sql/unireg.h
> > > index 419fbc4bd80..873d6f681fc 100644
> > > --- a/sql/unireg.h
> > > +++ b/sql/unireg.h
> > > @@ -177,7 +177,8 @@ enum extra2_frm_value_type {
> > >
> > >    EXTRA2_ENGINE_TABLEOPTS=128,
> > >    EXTRA2_FIELD_FLAGS=129,
> > > -  EXTRA2_FIELD_DATA_TYPE_INFO=130
> > > +  EXTRA2_FIELD_DATA_TYPE_INFO=130,
> > > +  EXTRA2_PERIOD_WITHOUT_OVERLAPS=131,
> >
> > Please, try to create a table that uses WITHOUT OVERLAPS
> > and open it in 10.4. Just as a test, to make sure it works as expected.
> 
> No it doesn't open. Is it the behavior you expect?

Yes :)
EXTRA2_PERIOD_WITHOUT_OVERLAPS > EXTRA2_ENGINE_IMPORTANT, so 10.4
is expected to fail with an error message and not open a table.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx