Re: bk commit into 5.1 tree (kostja:1.2588) BUG#27430

[Konstantin Osipov]

* Davi Arnaut <davi@stripped> [08/05/10 00:05]:
> >   sql/mysqld.cc@stripped, 2008-04-08 20:01:15+04:00, kostja@dipika.(none) +6 -3
> >     Implement a status variable for the number of reprepared statements.
> 
> Add a comment regarding the new default and minimum size of the TDC.

In order to change the changeset comments I will have to resubmit
the patch. There is very little time for that left now, so unless
something unexpected happens I won't be able to address this
note.

> > +  collation.set(src->collation.collation);
> 
> collation.set(src->collation);

Fixed.

> > +  /** Swap two my_decimal values */
> > +  void swap(my_decimal &rhs)
> > +  {
> > +    swap_variables(my_decimal, *this, rhs);
> > +    /* Swap the buffer pointers back */
> > +    swap_variables(decimal_digit_t *, buf, rhs.buf);
> 
> Shouldn't buf point to the same position but now relatively to the new
> buffer?

No, buf always points to the beginning of the buffer.

> > +  @sa check_and_update_metadata_version()
> 
> check_and_update_metadata_version does not exist, please correct all
> references. I prefer check_and_update_metadata_version instead of
> check_and_update_table_version.

Fixed to follow the spec. 
> >    load_defaults(conf_file_name, groups, &argc, &argv);
> > @@ -6757,7 +6759,8 @@ The minimum value for this variable is 4
> >    {"table_definition_cache", OPT_TABLE_DEF_CACHE,
> >     "The number of cached table definitions.",
> >     (uchar**) &table_def_size, (uchar**) &table_def_size,
> > -   0, GET_ULONG, REQUIRED_ARG, 128, 1, 512*1024L, 0, 1, 0},
> > +   0, GET_ULONG, REQUIRED_ARG, TABLE_DEF_CACHE_DEFAULT, TABLE_DEF_CACHE_MIN,
> > +   512*1024L, 0, 1, 0},
> 
> This must be thoroughly documented in the WL (reasons, impact, etc) to
> provided the Docs team with information to update the manual.

Added "Limitations" chapter to the spec.

> > +{
> > +  if (! tables->is_metadata_version_equal(table_share))
> > +  {
> > +    if (thd->m_metadata_observer &&
> > +        thd->m_metadata_observer->check_metadata_change(thd))
> > +    {
> > +      /*
> > +        Version of the table share is different from the
> > +        previous execution of the prepared statement, and it is
> > +        unacceptable for this SQLCOM. Error has been reported.
> > +      */
> 
> Add assert to ensure that a error has been reported.

Done.
> > +#if 0
> > +#ifndef DBUG_OFF
> > +  /* Spuriously reprepare each statement. */
> > +  if (thd->m_metadata_observer &&
> thd->stmt_arena->is_reprepared == FALSE)
> > +  {
> > +    thd->m_metadata_observer->check_metadata_change(thd);
> > +    return TRUE;
> > +  }
> > +#endif
> > +#endif
> 
> Move this dead code under a DBUG_EXECUTE_IF and add test cases like:

Will do.

> >    if (share->is_view)
> >    {
> > +    if (check_and_update_table_version(thd, table_list, share))
> > +      goto err;
> 
> Please add a comment why the check is at this point (like the below)

Done.
> > +    if (check_and_update_table_version(thd, tables, tables->table->s))
> 
> Please add a comment why the check is at this point (like the above)

Done.

> > +class Metadata_version_observer
> > +{
> > +protected:
> > +  virtual ~Metadata_version_observer();
> 
> Please consider using a protected constructor as it is a more common
> idiom to prevent class instantiation. Preventing explicit deletion here
> is useless.

Fixed by making all methods public.

> > +    Check if a change of metadata is OK. In future
> > +    the signature of this method may be extended to accept the old
> > +    and the new versions, but since currently the check is very
> > +    simple, we only need the THD to report an error.
> > +  */
> > +  virtual bool check_metadata_change(THD *thd)= 0;
> 
> Please choose a more appropriate name as this method does not check  for
> metadata changes.

Renamed to report_error().


> > @@ -2777,6 +2860,7 @@ public:
> >  #define CF_STATUS_COMMAND	4
> >  #define CF_SHOW_TABLE_COMMAND	8
> >  #define CF_WRITE_LOGS_COMMAND  16
> > +#define CF_REEXECUTION_FRAGILE 32
> 
> Please add comment explaining this flag.

Done.

> > +                                            CF_REEXECUTION_FRAGILE;
> > +  sql_command_flags[SQLCOM_SELECT]=         CF_REEXECUTION_FRAGILE;
> 
> Missing CF_REEXECUTION_FRAGILE flag for SQLCOM_CREATE_TABLE, SQLCOM_DO
> and SQLCOM_SET_OPTION commands. Please add testing for the
> aforementioned commands (CREATE TABLE .. SELECT .., etc).

This has been done already, in a separate patch.

> > +bool
> > +Execute_observer::check_metadata_change(THD *thd)
> > +{
> > +  bool save_thd_no_warnings_for_error= thd->no_warnings_for_error;
> > +  DBUG_ENTER("Execute_observer::notify_about_metadata_change");
> > +
> > +  thd->no_warnings_for_error= TRUE;
> > +  my_error(ER_NEED_REPREPARE, MYF(0));
> 
> Hum, I wonder about a possible interference of a SP continue handler..

Fixed by adding ME_NO_WARNING_FOR_ERROR and ME_NO_SP_HANDLER
my_error() flags.

> > +/**
> > +  Setup data conversion routines using an array of parameter
> > +  markers from the original prepared statement.
> > +  Move the parameter data of the original prepared
> 
> s/Move/Swap/

Done.

> > -  error= stmt->execute(&expanded_query,
> > -                       test(flags & (ulong) CURSOR_TYPE_READ_ONLY));
> 
> error now is unused, please remove it.

Done.

> > -  :Statement(&main_lex, &main_mem_root,
> > +  :Statement(0, &main_mem_root,
> 
> Use NULL instead of 0.

Done.

> > +  {
> > +    /* SQL prepared statement */
> > +    if (set_params_from_vars(this, thd->lex->prepared_stmt_params,
> > +                                   expanded_query))
> > +      goto set_params_data_err;
> 
> This could return on success and not goto is necessary.

Done, as well as in all other places.

> > +bool
> > +Prepared_statement::execute_loop(String *expanded_query,
> > +                                 bool open_cursor,
> > +                                 uchar *packet,
> > +                                 uchar *packet_end)
> > +{
> > +  const int MAX_REPREPARE_ATTEMPTS= 3;
> 
> Wouldn't it be better to be user specified with defaults?

Short answer: no, this is not part of specification.

My opinion, you could guess it since I prepared
the spec, is also no. In future MAX_REPREPARE_ATTEMPTS will be 1,
and that limit will be sufficient.
The reason we need a higher limit now is that we can't
keep the metadata locks between reprepare and subsequent execute, 
and thus have to account for the short window reprepare when
a DDL can sneak in.

In future execute we will not close used tables after reprepare,
and that will remove the need for validation and thus eliminate any 
chance of a validation error.

> > +  if (sql_command_flags[lex->sql_command] &
> > +      CF_REEXECUTION_FRAGILE)
> > +  {
> 
> Assert that there is no recursive observers.

Done.

> > +  if (error && !thd->is_fatal_error && !thd->killed
> &&
> > +      execute_observer.m_invalidated &&
> > +      reprepare_attempt++ < MAX_REPREPARE_ATTEMPTS)
> 
> Assert that m_invalidate is true only for a ER_NEED_REPREPARE error.

Done.
> > +  copy= new Prepared_statement(thd, &thd->protocol_text);
> 
> How about placing the copy in the stack? Prepared_statement is not fat
> and the code becomes cleaner.

Done.

> > +  if (! error)
> > +  {
> > +    swap_prepared_statement(copy);
> > +    swap_parameter_array(param_array, copy->param_array, param_count);
> > +    is_reprepared= TRUE;
> 
> is_reprepared is not present when debugging is off.

Fixed.

> > +      transparent to the user. We use mysql_reset_errors() since
> > +      there were no separate query id issued for re-prepare.
> > +    */
> > +    mysql_reset_errors(thd, TRUE);
> 
> I would prefer a internal handler to silence warnings.

Unfortunately, we can not simply silence the warnings.
In case reprepare failed, all warnings should be sent to the
client intact. In case it succeeded, the client should not be able
to notice. Use of mysql_reset_errors() currently is the best of
two bad choices.

When a fix for Bug#23032 is pushed, one will be able to introduce
a sort of "savepoints" for warnings. Then, in case of reprepare
success, the server will "rollback to the savepoint", and thus
remove all new warnings. In case of reprepare error, all warnings
from it will be sent to the user, as it happens now.

> > +  Item *item_org;
> > +  Item *item_new;
> 
> item_org and item_new are unused, please remove.

Done.

> > +    /** Column counts mismatch. */
> > +    my_error(ER_PS_REBIND, MYF(0));
> 
> As we talked on IRC, client side code and test coverage is missing.

Will do.

> > +   Firstly, spaces (in mathematical sense) of version numbers
> > +   never intersect for different metadata types. Therefore,
> 
> I guess your intent here in the mathematical sense is "sets".

Fixed.
> 
> "sets of version ids for different metadata types never intersect"
> 
> > +   version id of a temporary table is never compared with
> > +   a version id of a view or a temporary table, and vice versa.
> 
> The version id of a set can only be compared with others from the same
> set, thus a temp table version id IS compared to another temp table
> version id.

> 
> "version id of a temporary table is never compared with the version id
> of a view and vice versa"

Fixed.
> > +  ulong get_metadata_version() const
> > +  {
> > +    return tmp_table == SYSTEM_TMP_TABLE || is_view ? 0 : table_map_id;
> 
> For the sake of readability:
> 
> return (tmp_table == SYSTEM_TMP_TABLE || is_view) ? 0 : table_map_id;

Fixed.
> > +  inline
> > +  bool is_metadata_version_equal(TABLE_SHARE *s) const
> 
> Function name should reflect that metadata type is also compared.

Changed to is_metadata_id_equal(). Suggestions welcome :-p
However, I think it's a bikeshed argument -- conceptually it's a
version, even though its numeric part is also named "version".

Perhaps will change again after implementing Timour's review
comments. It seems everybody has his own archetype of "version",
Marc's patch would call "version" a completely different thing.


> > +  myheader("test_wl4166");
> 
> Just in case: memset(&my_bind, 0, sizeof(my_bind));
> 

Fixed.

--