Hi Monty, Please find my comments below. Anything not commented on seems to be ok to me.
> commit 7d9c24f6790c66d2f457837661852bf5e870b605 > Author: Michael Widenius <[email protected]> > Date: Tue Nov 14 07:47:58 2017 +0200 > > Handle failures from malloc > > Most "new" failures fixed in the following files: > - sql_select.cc > - item.cc > - opt_subselect.cc > > diff --git a/sql/item.cc b/sql/item.cc > index 2285ae28041..baae2a9b6fb 100644 > --- a/sql/item.cc > +++ b/sql/item.cc > @@ -1261,7 +1261,7 @@ Item *Item::safe_charset_converter(THD *thd, > CHARSET_INFO *tocs) > if (!needs_charset_converter(tocs)) > return this; > Item_func_conv_charset *conv= new (thd->mem_root) > Item_func_conv_charset(thd, this, tocs, 1); > - return conv->safe ? conv : NULL; > + return conv && conv->safe ? conv : NULL; > } > I am not fully convinced that returning NULL from this function is called everywhere. It is of course better to return NULL and hope for the best than to crash right away. > @@ -3296,6 +3301,8 @@ void Item_field::fix_after_pullout(st_select_lex > *new_parent, Item **ref, > } > > Name_resolution_context *ctx= new Name_resolution_context(); > + if (ctx) > + return; // Fatal error set > if (context->select_lex == new_parent) > { > /* I assume this is a typo it should be "if (!ctx)" > diff --git a/sql/sql_select.cc b/sql/sql_select.cc > index abbf2616537..7e42890143b 100644 > --- a/sql/sql_select.cc > +++ b/sql/sql_select.cc > @@ -713,7 +713,7 @@ JOIN::prepare(TABLE_LIST *tables_init, > union_part= unit_arg->is_unit_op(); > > if (select_lex->handle_derived(thd->lex, DT_PREPARE)) > - DBUG_RETURN(1); > + DBUG_RETURN(-1); > > thd->lex->current_select->context_analysis_place= NO_MATTER; > thd->lex->current_select->is_item_list_lookup= 1; I assume this is just for the sake of consistency with other DBUG_RETURN statements in this function? > @@ -14422,7 +14446,7 @@ static COND* substitute_for_best_equal_field(THD > *thd, JOIN_TAB *context_tab, > This works OK with PS/SP re-execution as changes are made to > the arguments of AND/OR items only > */ > - if (new_item != item) > + if (new_item && new_item != item) > li.replace(new_item); > } new_item was returned from a substitute_for_best_equal_field() call. That one doesn't ever return NULL. It will return without substitution if it encounters an error. So this change is not needed. > @@ -25044,8 +25103,11 @@ int JOIN::save_explain_data_intern(Explain_query > *output, > > if (message) > { > - explain= new (output->mem_root) Explain_select(output->mem_root, > - thd->lex->analyze_stmt); > + if (!(explain= new (output->mem_root) > + Explain_select(output->mem_root, > + thd->lex->analyze_stmt))) > + DBUG_RETURN(1); > + > join->select_lex->set_explain_type(true); > > explain->select_id= join->select_lex->select_number; The return value of this function is not checked. I assume the above statement is there so that we dont try to continue to execute the function if we've got an OOM error? In this case, probably JOIN::save_explain_data_intern() should also have a similar check for its tab->save_explain_data(eta, used_tables, distinct_arg, first_top_tab); call? BR Sergei -- Sergei Petrunia, Software Developer MariaDB Corporation | Skype: sergefp | Blog: http://s.petrunia.net/blog _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : [email protected] Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
