Re: bk commit into 6.0 tree (thek:1.2661) BUG#35997

[Rafal Somla]

Hi Kristofer,

The patch looks ok. I have some minor suggestions - consider implementing them 
before pushing but no need for extra review.

Rafal

kpettersson@stripped wrote:
> Below is the list of changes that have just been committed into a local
> 6.0 repository of thek.  When thek does a push these changes
> will be propagated to the main repository and, within 24 hours after the
> push, to the public repository.
> For information on how to access the public repository
> see http://dev.mysql.com/doc/mysql/en/installing-source-tree.html
> 
> ChangeSet@stripped, 2008-05-09 12:56:54+02:00, thek@adventure.(none) +1 -0
>   Bug#35997 Event scheduler seems to let the server crash, if it is embedded.
>   
>   Part 2.
>   This patch disable event scheduler code related to the online backup
>   subsystem introduced in 6.0.

Please don't mention online backup here. Let's pretend that si_objects code is a 
generic interface into server functionality. Thus replace it by something along 
these lines:

    This patch fixes event related methods in object services API (si_objects.c)
    so that they behave well also when events are disabled in the server.

> 
>   sql/si_objects.cc@stripped, 2008-05-09 12:56:49+02:00, thek@adventure.(none) +43 -6
>     Disable event code if compiled in embedded mode using the
>     HAVE_EVENT_SCHEDULER directive.
>  

Why mentioning the embedded mode? The HAVE_EVENT_SCHEDULER flag is all we need 
to look at.


>     Introduced a dummy implementation of ObjIterator interface.
> 
> diff -Nrup a/sql/si_objects.cc b/sql/si_objects.cc
> --- a/sql/si_objects.cc	2008-04-16 09:53:13 +02:00
> +++ b/sql/si_objects.cc	2008-05-09 12:56:49 +02:00
> @@ -17,9 +17,11 @@
>  #include "si_objects.h"
>  #include "ddl_blocker.h"
>  #include "sql_show.h"
> +#ifdef HAVE_EVENT_SCHEDULER
>  #include "events.h"
>  #include "event_data_objects.h"
>  #include "event_db_repository.h"
> +#endif
>  #include "sql_trigger.h"
>  #include "sp.h"
>  #include "sp_head.h" // for sp_add_to_query_tables().
> @@ -669,7 +671,7 @@ private:
>  };
>  
>  ///////////////////////////////////////////////////////////////////////////
> -
> +#ifdef HAVE_EVENT_SCHEDULER
>  /**
>    @class EventObj
>  
> @@ -709,6 +711,7 @@ private:
>    // These attributes are to be used only for materialization.
>    String m_create_stmt;
>  };
> +#endif  // HAVE_EVENT_SCHEDULER
>  
>  /**
>     @class TablespaceObj
> @@ -817,6 +820,20 @@ private:
>  
>  ///////////////////////////////////////////////////////////////////////////
>  
> +class ObjIteratorDummyImpl : ObjIterator
> +{
> +public:
> +  ObjIteratorDummyImpl() { return; }
> +  virtual ~ObjIteratorDummyImpl() { return; }
> +  virtual Obj *next() { return NULL; }
> +
> +protected:
> +  virtual Obj *create_obj(TABLE *t) { return NULL; }
> +
> +};
> +
> +
> +///////////////////////////////////////////////////////////////////////////
>  class DatabaseIterator : public InformationSchemaIterator
>  {
>  public:
> @@ -955,7 +972,7 @@ protected:
>  };
>  
>  ///////////////////////////////////////////////////////////////////////////
> -
> +#ifdef HAVE_EVENT_SCHEDULER
>  class DbEventIterator : public InformationSchemaIterator
>  {
>  public:
> @@ -975,7 +992,7 @@ protected:
>  private:
>    String m_db_name;
>  };
> -
> +#endif
>  
>  ///////////////////////////////////////////////////////////////////////////
>  
> @@ -1292,6 +1309,7 @@ Obj *DbStoredFuncIterator::create_sr_obj
>    return new StoredFuncObj(db_name, sr_name);
>  }
>  
> +#ifdef HAVE_EVENT_SCHEDULER
>  ///////////////////////////////////////////////////////////////////////////
>  
>  //
> @@ -1299,7 +1317,6 @@ Obj *DbStoredFuncIterator::create_sr_obj
>  //
>  
>  ///////////////////////////////////////////////////////////////////////////
> -
>  EventObj *DbEventIterator::create_obj(TABLE *t)
>  {
>    String db_name;
> @@ -1315,7 +1332,7 @@ EventObj *DbEventIterator::create_obj(TA
>  
>    return new EventObj(&db_name, &event_name);
>  }
> -
> +#endif
>  ///////////////////////////////////////////////////////////////////////////
>  
>  //
> @@ -1486,9 +1503,11 @@ template
>  DbStoredFuncIterator *
>  create_is_iterator<DbStoredFuncIterator>(THD *, enum_schema_tables, const
> String *);
>  
> +#ifdef HAVE_EVENT_SCHEDULER
>  template
>  DbEventIterator *
>  create_is_iterator<DbEventIterator>(THD *, enum_schema_tables, const String
> *);
> +#endif
>  
>  ObjIterator *get_db_tables(THD *thd, const String *db_name)
>  {
> @@ -1515,10 +1534,17 @@ ObjIterator *get_db_stored_functions(THD
>    return create_is_iterator<DbStoredFuncIterator>(thd, SCH_PROCEDURES,
> db_name);
>  }
>  
> +#ifdef HAVE_EVENT_SCHEDULER
>  ObjIterator *get_db_events(THD *thd, const String *db_name)
>  {
>    return create_is_iterator<DbEventIterator>(thd, SCH_EVENTS, db_name);
>  }
> +#else
> +ObjIterator *get_db_events(THD *thd, const String *db_name)
> +{
> +  return (ObjIterator *)new ObjIteratorDummyImpl;
> +}
> +#endif
>  

I would put this #ifdef inside the function body - function header does not change.

>  
>  ///////////////////////////////////////////////////////////////////////////
> @@ -2257,7 +2283,7 @@ bool StoredFuncObj::drop(THD *thd)
>  //
>  
>  /////////////////////////////////////////////////////////////////////////////
> -
> +#ifdef HAVE_EVENT_SCHEDULER
>  EventObj::EventObj(const String *db_name,
>                     const String *event_name)
>  {
> @@ -2415,6 +2441,7 @@ bool EventObj::drop(THD *thd)
>                            &m_db_name,
>                            &m_event_name));
>  }
> +#endif // HAVE_EVENT_SCHEDULER
>  
>  ///////////////////////////////////////////////////////////////////////////
>  
> @@ -2584,11 +2611,19 @@ Obj *get_stored_function(const String *d
>    return new StoredFuncObj(db_name, sf_name);
>  }
>  
> +#ifdef HAVE_EVENT_SCHEDULER
>  Obj *get_event(const String *db_name,
>                 const String *event_name)
>  {
>    return new EventObj(db_name, event_name);
>  }
> +#else
> +Obj *get_event(const String *db_name,
> +               const String *event_name)
> +{
> +  return NULL;
> +}
> +#endif
>

Again, I suggest putting #ifded inside function body.

>  ///////////////////////////////////////////////////////////////////////////
>  
> @@ -2657,6 +2692,7 @@ Obj *materialize_stored_function(const S
>    return obj;
>  }
>  
> +#ifdef HAVE_EVENT_SCHEDULER
>  Obj *materialize_event(const String *db_name,
>                         const String *event_name,
>                         uint serialization_version,
> @@ -2667,6 +2703,7 @@ Obj *materialize_event(const String *db_
>  
>    return obj;
>  }
> +#endif
>  
>  Obj *materialize_tablespace(const String *ts_name,
>                              uint serialization_version,
> 

Rafal