maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sergei,

On Thu, Dec 15, 2016 at 04:38:04PM +0100, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On Dec 15, Sergey Vojtovich wrote:
> > > > +  /*
> > > >      An upgradable shared metadata lock which blocks all attempts to update
> > > >      table data, allowing reads.
> > > >      A connection holding this kind of lock can read table metadata and read
> > > > diff --git a/sql/sql_base.cc b/sql/sql_base.cc
> > > > index 1ca7e9c..d3f4517 100644
> > > > --- a/sql/sql_base.cc
> > > > +++ b/sql/sql_base.cc
> > > > @@ -1496,6 +1496,7 @@ bool open_table(THD *thd, TABLE_LIST *table_list, Open_table_context *ot_ctx)
> > > >      to general/slow log would be disabled in read only transactions.
> > > >    */
> > > >    if (table_list->mdl_request.type >= MDL_SHARED_WRITE &&
> > > > +      table_list->mdl_request.type != MDL_SHARED_READ_ONLY &&
> > > 
> > > if you're going to keep read and write locks mixed,
> > > then I'd suggest to move this condition into a small inline helper,
> > > like table_list->mdl_request.is_write_lock().  it's used few times
> > > already in this very file and there will be only one place to change
> > > if we'll need to add, say, MDL_SHARED_WRITE_LOW_PRIO
> > I can merge this from MySQL, it is called is_write_lock_request() there. In fact
> > MySQL didn't have to change this condition for MDL_SHARED_WRITE_LOW_PRIO.
> 
> I think it'd be good. There were three or four places in sql_base.cc
> where it should replace the >= check.
Ok.

> 
> > > > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > > > index 6dc9192..0d740bb 100644
> > > > --- a/sql/sql_parse.cc
> > > > +++ b/sql/sql_parse.cc
> ...
> > > > +        thd->pop_internal_handler();
> > > > +
> > > > +        if (deadlock_handler.need_reopen())
> > > > +        {
> > > > +          /*
> > > > +            Deadlock occurred during upgrade of metadata lock.
> > > > +            Let us restart acquring and opening tables for LOCK TABLES.
> > > 
> > > will you use MDL_SHARED_READ_ONLY right away when retrying?
> > > or it'll do the upgrade thing again?
> > Upgrade thing again, which is farily stupid. That's how it was in original code
> > and I didn't change it in a hurry.
> 
> Can you do it?  It's probably just updating the lock in the
> corresponding table_list entry.
> 
> Still, better do it as a separate change, after you push this MDEV-11227
> commit, so that 10.2.3 wouldn't have to wait for it.
Ok. Should I consider this patch is approved?
It is possible to push it now, but taking into account complexity and bb state:
should we postpone it until bb is greener?

Regards,
Sergey