[Commits] b97452a6204: MDEV-19491 update query stopped working after mariadb upgrade 10.2.23 -> 10.2.24

Sergey Vojtovich svoj at mariadb.org
Thu May 30 22:24:06 EEST 2019


Sergei,

On Thu, May 30, 2019 at 09:03:24PM +0200, Sergei Golubchik wrote:
> Hi, Sergey!
> 
> On May 30, Sergey Vojtovich wrote:
> > > +#
> > > +# MDEV-19521 Update Table Fails with Trigger and Stored Function
> > > +#
> > > +create table t1 (a int, b varchar(50), c varchar(50));
> > > +insert t1 (a,b) values (1,'1'), (2,'2'), (3,'3');
> > > +create function f1() returns varchar(50) return 'result';
> > > +create trigger tr before update on t1 for each row set new.c = (select f1());
> > > +create table t2 select a, b from t1;
> > > +update t1 join t2 using (a) set t1.b = t2.b;
> > > +drop table t1, t2;
> > > +drop function f1;
> > I don't completely understand why this test was failing. It doesn't request
> > backoff action, right? Or is this patch fixing more than just backoff retry?
> 
> More.
> 
> open_tables() starts from
> 
>     has_prelocking_list= thd->lex->requires_prelocking();
> 
> when open_tables() was called twice, the first value of
> has_prelocking_list was lost. Later when triggers were opened in the
> second open_tables() call, routines were not opened, because
> has_prelocking_list on the second invocation was true.
> 
> This could've been fixed by passing has_prelocking_list as an argument
> to open_tables().
> 
> But by looking at this code I've noticed the backoff issue, so I took
> a different approach, that fixed both bugs.
Ok, it explains why you added
Multiupdate_prelocking_strategy::has_prelocking_list.

> 
> > > diff --git a/sql/sql_base.h b/sql/sql_base.h
> > > index ea92e880db7..439052a28f5 100644
> > > --- a/sql/sql_base.h
> > > +++ b/sql/sql_base.h
> > > @@ -426,6 +426,7 @@ class Prelocking_strategy
> > >  public:
> > >    virtual ~Prelocking_strategy() { }
> > >  
> > > +  virtual void reset(THD *thd) { };
> > >    virtual bool handle_routine(THD *thd, Query_tables_list *prelocking_ctx,
> > >                                Sroutine_hash_entry *rt, sp_head *sp,
> > >                                bool *need_prelocking) = 0;
> > > @@ -433,6 +434,7 @@ class Prelocking_strategy
> > >                              TABLE_LIST *table_list, bool *need_prelocking) = 0;
> > >    virtual bool handle_view(THD *thd, Query_tables_list *prelocking_ctx,
> > >                             TABLE_LIST *table_list, bool *need_prelocking)= 0;
> > > +  virtual bool handle_end(THD *thd) { return 0; };
> > >  };
> > May be just end()? It looks inconsistent with reset().
> 
> handle_xxx is called when something happens, to "handle" it.
> "handle_end" means to handle the end of the one pass over the table
> list. Say, the table list has tables t1, t2, t3. handle_table is called
> for each. Then routines are opened, they may add t4 and t5 to the list.
> But after handling t1, t2, t3 handle_end is called. Then t4 and t5 are
> opened, handle_table is called for them, then handle_end again.
Ok, It is probably just me not being a fan of handle/handler/handlerton.

> 
> > Also end() sounds multi-update specific. Isn't it "next nesting level dive"?
> > May be level_end()? Or next_level()?
> 
> It's not really a level, a table list is a linked list, not a hierarchy.
> one_pass_end() ? no, not good either.
> I don't have a good name, sorry.
Sigh.

> 
> > I wonder if handle_table() alone can handle this? Or do we need to open all
> > tables before we can tell which ones are go ing to be updated?
> 
> I've spent a lot of time trying to do it in handle_table().
> The idea was to get the last handle_table() call to do the job.
> That's why the whole interface is a Prelocking_strategy, may be it
> wouldn't have been, if it hadn't started from handle_table().
> 
> The problem was that it's fairly difficult to detect the last
> handle_table() call. It's the call for the last updatable table which
> isn't a view, and isn't information_schema table. Then there was some
> issue with routines that are opened after tables, and it was too late.
> I was fixing all those problems but finally I've basically given up.
> I'm not sure anymore what the last problem was, probably the one with
> information_schema tables (handle_table() isn't called for them).
I think it implies that we have to open all query tables before evalutation.
My second question was: do we actually have to do it this way?

> 
> > ...skip...
> 
> The rest is fixed. Thanks!
One question re is_stmt_prepare() is not answered.

Ok to push anyway.

Regards,
Sergey


More information about the commits mailing list