Re: [Maria-developers] Coding Style Review INSERT RETURNING

Hello!

Thanks for the review. I'll start working on it and keep you updated with
my progress.

Regards,
Rucha

On Wed, Aug 7, 2019 at 7:51 PM Vicențiu Ciorbaru 
wrote:

> Hi Rucha!
>
> This is going to be a very thorough review looking at coding style. I'll
> nitpick on every line of code where I find something, just be patient and
> let's clean up everything.
>
> You might consider me picking on the coding style a bit pedantic, but it
> really is important. It is important because it helps in searching, it
> helps
> in understanding the code faster, it helps to look-up history faster.
> The good part is that once you get used to writing things properly, you
> won't
> even have to think about it actively. It will just come naturally.
>
> I suspect you do not have your editor configured to show whitespaces and
> show you tabs versus spaces. Please look into your editor's settings to
> highlight both tabs and spaces.
>
> There are scripts that remove trailing whitespaces for you, across whole
> files.
> I don't recommend them for MariaDB code as we have remnants from MySQL
> times of
> these and because we care more about preserving the immediate history for
> running `git blame`, we did not fully fix all files. For any other
> project, I
> highly recommend you use them.
>
> Some of these comments might slightly contradict what I said before. That
> is
> because I wanted us to get things working first and I didn't want to burden
> you with too many details initially. I'll look to explain myself now
> clearly
> so you understand why some things are bad ideas. I want you to take this
> review also as a general guideline for any other code you write, anywhere.
> With these hints, tricks, ideas, you'll produce much higher quality code in
> the long run. It will take you longer to write sometimes, but it really
> makes
> a difference. :)
>
> Also, you may have noticed some parts of our code do not agree with my
> general
> guidelines. You have to take into account that this code was written over
> the
> course of many years, by different people, with different levels of
> experience
> or available time to implement something. Try not to make use of it as an
> excuse for new code that you write. It's up to you to always write the best
> possible version *with the time allowed*. We have adequate time now so we
> can afford to look for better solutions.
>
> With all that said, this took me a while to write and I may still
> have missed a few things. Feel free to point out anything that looks
> strange.
> Now, let's get to the review.
>
> >
> > diff --git a/sql/sql_class.h b/sql/sql_class.h
> > index 18ccf992d1e..9e4bb1d4579 100644
> > --- a/sql/sql_class.h
> > +++ b/sql/sql_class.h
> > @@ -5475,7 +5476,13 @@ class select_dump :public select_to_file {
> >
> >  class select_insert :public select_result_interceptor {
> >   public:
> > +  select_result* sel_result;
> The * should be next to the variable name. Not everybody does this, but the
> code around it does, so let's be consistent.
> >    TABLE_LIST *table_list;
> > +  /*
> > +    Points to the table in which we're inserting records. We need to
> save the
> > +    insert table before it is masked in the parsing phase.
> > +  */
> > +  TABLE_LIST *insert_table;
> This sort of code and comment is usually a bad idea:
> 1. We are adding an extra member, because of a hack in a different part of
>    the code.
> 2. We are mentioning a specific flow for which this member is good for.
> This
>    already begins to hint at a bad design decision. Sometimes there's no
> way
>    around it, but this guarantees lack of re-usability. If, in the future
>    we fix the "masking in the parsing phase" to no longer mask the first
> table,
>    which we probably will do, because we have to to allow INSERT within
> other
>    queries, the comment will become outdated. One needs to remember this
> too.
>
>    Forgetting to update comments happens easily. Here is one example:
>    One checks the class initially, finds the appropriate member to use,
>    uses it in a different context than the original author thought of. The
>    class then is no longer changed so it does not show up during review
> time,
>    the reviewer doesn't think to check the class member's definition
> either.
>    You now have outdated comments.
>
>    To prevent this from happening, avoid as much as possible to explain
> where
>    a member is used, rather explain what type of data it is supposed to
> store.
>    The first sentence accomplishes that goal.
> >    TABLE *table;
> >    List *fields;
> >    ulonglong autoinc_value_of_last_inserted_row; // autogenerated or not
> > @@ -5484,7 +5491,7 @@ class select_insert :public
> select_result_interceptor {
> >    select_insert(THD *thd_arg, TABLE_LIST *table_list_par,
> >               TABLE *table_par, List *fields_par,
> >               List *update_fields, List *update_values,
> > -             enum_duplicates duplic, bool ignore);
> > +             enum_duplicates duplic, bool ignore,select_result*
> sel_ret_list,TABLE_LIST *save_first);
> This is an artifact of our long-standing code base. TABS mixed with
> spaces. :(
> Let's clean up all of this select_insert's constructor. Do not keep the
> tabs,
> we are modifying it anyway. Turn all these tabs to proper spaces. Make sure
> the function arguments match the beginning parenthesis like so:
>   (TYPE1 arg1, TYPE2 arg2,
>                   TYPE3 arg3, TYPE4 arg4,
>                   TYPE5 arg5)
>
> >    ~select_insert();
> >    int prepare(List &list, SELECT_LEX_UNIT *u);
> >    virtual int prepare2(JOIN *join);
> > @@ -5520,7 +5527,7 @@ class select_create: public select_insert {
> >                  List &select_fields,enum_duplicates duplic, bool
> ignore,
> >                  TABLE_LIST *select_tables_arg):
> >      select_insert(thd_arg, table_arg, NULL, &select_fields, 0, 0,
> duplic,
> > -                  ignore),
> > +                  ignore, NULL, NULL),
> Sigh... again, we have a historical background of mixing NULLs with 0s.
> Ok here I guess.
> >      create_table(table_arg),
> >      create_info(create_info_par),
> >      select_tables(select_tables_arg),
> > diff --git a/sql/sql_insert.cc b/sql/sql_insert.cc
> > index f3a548d7265..4bb19cef726 100644
> > --- a/sql/sql_insert.cc
> > +++ b/sql/sql_insert.cc
> > @@ -119,6 +119,18 @@ static bool check_view_insertability(THD *thd,
> TABLE_LIST *view);
> >    @returns false if success.
> >  */
> >
> > +/*
> > + Swaps the context before and after calling setup_fields() and
> setup_wild() in
> > + INSERT...SELECT when LEX::returning_list is not empty.
> One more space to indent the comment text, for both lines.
> > +*/
> > +template 
> > +void swap_context(T& cxt1, T& cxt2)
> > +{
> > +     T temp = cxt1;
> > +     cxt1 = cxt2;
> > +     cxt2 = temp;
> > +
> > +}
> Unneded empty line before the the closing }.
> Also, you should add 2 empty lines after the function. Delete the tabs in
> the
> function, replace with 2 spaces.
> Also, this function is not even used just to swap_contexts any more. Rename
> it to swap.
>
> Can we not use std::swap instead?
> >  static bool check_view_single_update(List &fields, List
> *values,
> >                                       TABLE_LIST *view, table_map *map,
> >                                       bool insert)
> > @@ -688,6 +700,10 @@ Field **TABLE::field_to_fill()
> >
> >  /**
> >    INSERT statement implementation
> > +
> Trailing whitespace.
> > +  SYNOPSIS
> > +  mysql_insert()
> > +  result    NULL if returning_list is empty
> I'd change this to NULL if the insert is not outputing results via
> 'RETURNING'
> clause.
> >
> >    @note Like implementations of other DDL/DML in MySQL, this function
> >    relies on the caller to close the thread tables. This is done in the
> > @@ -700,7 +716,7 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >                    List &update_fields,
> >                    List &update_values,
> >                    enum_duplicates duplic,
> > -               bool ignore)
> > +               bool ignore, select_result* result)
> Remove tabs, use spaces, align properly, like explained above.
> >  {
> >    bool retval= true;
> >    int error, res;
> > @@ -719,6 +735,8 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >    List_item *values;
> >    Name_resolution_context *context;
> >    Name_resolution_context_state ctx_state;
> > +  SELECT_LEX* select_lex = thd->lex->first_select_lex();
> > +  List& returning_list = thd->lex->returning_list;
> Generally this kind of use of references is a bad idea. Highly suggest you
> only use const references, otherwise things can turn very confusing,
> very quickly. If you read the Google C++ coding style guide, you'll see
> they
> only allow const references in any part of their code.
> >  #ifndef EMBEDDED_LIBRARY
> >    char *query= thd->query();
> >    /*
> > @@ -775,9 +793,13 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >
> >    if (mysql_prepare_insert(thd, table_list, table, fields, values,
> >                          update_fields, update_values, duplic,
> &unused_conds,
> > -                           FALSE))
> > -    goto abort;
> > -
> > +                           FALSE,result?true:false))
> Missing a few spaces. One after , One before and one after ?
> One before and one after :.
> > +       goto abort;
> Tabs instead of spaces.
> > +
> Trailing whitespaces.
> > +  /* Prepares LEX::returing_list if it is not empty */
> > +  if (result)
> > +             (void)result->prepare(returning_list, NULL);
> Tabs instead of spaces.
> > +
> Tabs, spaces, trailing whitespaces. Make sure each new line that contains
> no text be just that, a newline.
> >    /* mysql_prepare_insert sets table_list->table if it was not set */
> >    table= table_list->table;
> >
> > @@ -947,6 +969,18 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >        goto values_loop_end;
> >      }
> >    }
> > +  /*
> > +  If  statement returns result set, we need to send the result set
> metadata
> > +  to the client so that it knows that it has to expect an EOF or ERROR.
> > +  At this point we have all the required information to send the result
> set metadata.
> > +  */
> When adding comments like these, you need to indent the text 2 more spaces.
> You crossed the 80 character limit with the "metadata" word. That needs to
> be
> put on a new line.
> > +  if (result)
> > +  {
> > +       if (unlikely(result->send_result_set_metadata(returning_list,
> Tabs again.
> > +      Protocol::SEND_NUM_ROWS |
> > +      Protocol::SEND_EOF)))
> > +      goto values_loop_end;
> The goto indentation is ok, but Protocol should be aligned to the last open
> parenthesis.
> > +  }
> >
> >    THD_STAGE_INFO(thd, stage_update);
> >    do
> > @@ -1060,7 +1094,7 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >          error= 1;
> >          break;
> >        }
> > -
> > +
> You added an extra line here, but introduced a tab and trailing
> whitespaces,
> not good.
> >  #ifndef EMBEDDED_LIBRARY
> >        if (lock_type == TL_WRITE_DELAYED)
> >        {
> > @@ -1072,9 +1106,25 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >        }
> >        else
> >  #endif
> > -        error=write_record(thd, table ,&info);
> > +        error=write_record(thd, table ,&info);
> You didn't change this line logic at all. Just introduced an extra tab plus
> whitespaces. Please clean it up.
> >        if (unlikely(error))
> >          break;
> > +      /*
> > +        We send the rows after writing them to table so that the
> correct information
> > +        is sent to the client. Example INSERT...ON DUPLICAT KEY UPDATE
> and values
> > +        set to auto_increment. Write record handles auto_increment
> updating values
> > +        if there is a duplicate key. We want to send the rows to the
> client only
> > +        after these operations are carried out. Otherwise it shows 0 to
> the client
> > +        if the fields that are incremented automatically are not given
> explicitly
> > +        and the irreplaced values in case of ON DUPLICATE KEY UPDATE
> (even if
> > +        the values are replaced or incremented while writing record.
> > +        Hence it shows different result set to the client)
> > +      */
> Good comment! Trailing whitespaces however are still present and you are
> breaking the 80 character rule.
> > +      if (result && result->send_data(returning_list) < 0)
> > +      {
> > +        error = 1;
> Remove space before =.
> > +        break;
> > +      }
> >        thd->get_stmt_da()->inc_current_row_for_warning();
> >      }
> >      its.rewind();
> > @@ -1233,19 +1283,30 @@ bool mysql_insert(THD *thd,TABLE_LIST
> *table_list,
> >      retval= thd->lex->explain->send_explain(thd);
> >      goto abort;
> >    }
> > +
> Trailing whitespaces. Please fix.
> >    if ((iteration * values_list.elements) == 1 &&
> (!(thd->variables.option_bits & OPTION_WARNINGS) ||
> >                                   !thd->cuted_fields))
> > -  {
> > -    my_ok(thd, info.copied + info.deleted +
> > +  {
> > +    /*
> > +      Client expects an EOF/Ok packet if result set metadata was sent.
> If
> > +      LEX::returning_list is not empty and the statement returns result
> set
> > +      we send EOF which is the indicator of the end of the row stream.
> > +      Else we send Ok packet i.e when the statement returns only the
> status
> > +      information
> > +    */
> > +       if (result)
> > +      result->send_eof();
> > +       else
> > +      my_ok(thd, info.copied + info.deleted +
> >                 ((thd->client_capabilities & CLIENT_FOUND_ROWS) ?
> > -                info.touched : info.updated),
> > -          id);
> > +                info.touched : info.updated),id);
> >    }
> >    else
> > -  {
> > +  {
> You didn't change the logic here, just added an extra trailing whitespace.
> >      char buff[160];
> >      ha_rows updated=((thd->client_capabilities & CLIENT_FOUND_ROWS) ?
> >                       info.touched : info.updated);
> > +
> I guess I am ok with an extra line here, but it should not have an extra
> tab.
> Please, only newlines, no extra characters.
> >      if (ignore)
> >        sprintf(buff, ER_THD(thd, ER_INSERT_INFO), (ulong) info.records,
> >             (lock_type == TL_WRITE_DELAYED) ? (ulong) 0 :
> > @@ -1255,8 +1316,12 @@ bool mysql_insert(THD *thd,TABLE_LIST *table_list,
> >        sprintf(buff, ER_THD(thd, ER_INSERT_INFO), (ulong) info.records,
> >             (ulong) (info.deleted + updated),
> >                (long)
> thd->get_stmt_da()->current_statement_warn_count());
> > -    ::my_ok(thd, info.copied + info.deleted + updated, id, buff);
> > +             if (result)
> > +                     result->send_eof();
> > +             else
> Tabs, please use spaces.
> > +      ::my_ok(thd, info.copied + info.deleted + updated, id, buff);
> >    }
> > +
> Trailing whitespaces. :(
> >    thd->abort_on_warning= 0;
> >    if (thd->lex->current_select->first_cond_optimization)
> >    {
> > @@ -1470,6 +1535,7 @@ static void prepare_for_positional_update(TABLE
> *table, TABLE_LIST *tables)
> >                       be taken from table_list->table)
> >      where            Where clause (for insert ... select)
> >      select_insert    TRUE if INSERT ... SELECT statement
> > +    with_returning_list TRUE if returning_list is not empty
> Trailing whitespace. (I know the rest of the function comments also have
> trailing whitespaces. Perfect time to fix all that and remove all their
> tabs.
> >
> >    TODO (in far future)
> >      In cases of:
> > @@ -1490,7 +1556,7 @@ bool mysql_prepare_insert(THD *thd, TABLE_LIST
> *table_list,
> >                            TABLE *table, List &fields, List_item
> *values,
> >                            List &update_fields, List
> &update_values,
> >                            enum_duplicates duplic, COND **where,
> > -                          bool select_insert)
> > +                          bool select_insert,  bool with_returning_list)
> One too many spaces after the last comma.
> >  {
> >    SELECT_LEX *select_lex= thd->lex->first_select_lex();
> >    Name_resolution_context *context= &select_lex->context;
> > @@ -1556,8 +1622,21 @@ bool mysql_prepare_insert(THD *thd, TABLE_LIST
> *table_list,
> >       */
> >      table_list->next_local= 0;
> >      context->resolve_in_table_list_only(table_list);
> > +
> Trailing whitespaces.
> > +    /*
> > +      Perform checks like all given fields exists, if exists fill
> struct with
> Trailing whitespace.
> > +      current data and expand all '*' in given fields for
> LEX::returning_list.
> > +    */
> Trailing whitespaces.
> > +     if(with_returning_list)
> Trailing whitespaces. Put a space after if.
> > +     {
> > +       res= ((select_lex->with_wild && setup_wild(thd, table_list,
> Trailing whitespace.
> > +
>  thd->lex->returning_list,NULL, select_lex->with_wild,
> Trailing whitespace, tabs instead of spaces.
> > +
>  &select_lex->hidden_bit_fields)) ||
> > +                                             setup_fields(thd,
> Ref_ptr_array(),
> > +                                             thd->lex->returning_list,
> MARK_COLUMNS_READ, 0, NULL, 0));
> Tabs instead of spaces, swap them to spaces. Make sure to follow the 80
> character max line length.
> > +     }
> >
> > -    res= (setup_fields(thd, Ref_ptr_array(),
> > +    res= (res||setup_fields(thd, Ref_ptr_array(),
> Please Add a space before and after ||.
> >                         *values, MARK_COLUMNS_READ, 0, NULL, 0) ||
> >            check_insert_fields(thd, context->table_list, fields, *values,
> >                                !insert_into_view, 0, &map));
> > @@ -1724,7 +1803,7 @@ int write_record(THD *thd, TABLE *table,COPY_INFO
> *info)
> >          table->file->insert_id_for_cur_row= insert_id_for_cur_row;
> >        bool is_duplicate_key_error;
> >        if (table->file->is_fatal_error(error, HA_CHECK_ALL))
> > -     goto err;
> > +        goto err;
> A good fix, not strictly necessary, but a good fix.
> >        is_duplicate_key_error=
> >          table->file->is_fatal_error(error, HA_CHECK_ALL &
> ~HA_CHECK_DUP);
> >        if (!is_duplicate_key_error)
> > @@ -3557,29 +3636,39 @@ bool Delayed_insert::handle_inserts(void)
> >      TRUE  Error
> >  */
> >
> > -bool mysql_insert_select_prepare(THD *thd)
> > +bool mysql_insert_select_prepare(THD *thd,select_result *sel_res)
> Put a space after last comma (,).
> This function could use a bit of tweaking, but let's fix all the code style
> issues first.
> >  {
> >    LEX *lex= thd->lex;
> >    SELECT_LEX *select_lex= lex->first_select_lex();
> >    DBUG_ENTER("mysql_insert_select_prepare");
> > -
> > +  List& returning_list = thd->lex->returning_list;
> For assignment operators no space before it. Like so:
> List& returning_list= thd->lex->returning_list;
> >
> >
> >    /*
> >      SELECT_LEX do not belong to INSERT statement, so we can't add WHERE
> >      clause if table is VIEW
> >    */
> > -
> > +  /*
> > +    Passing with_returning_list (last argument) as false otherwise
> > +    setup_field() and setup_wild() will be called twice. 1) in
> mysql_prepare_insert()
> > +    and 2) time in select_insert::prepare(). We want to call it only
> once:
> > +    in select_insert::prepare()
> > +  */
> Ok, this is an interesting comment. It's much better if we can write code
> that
> prevents this sort of reasoning. Pragmatically, why do we end up calling it
> twice? Can we somehow refactor it such that this is not a problem in the
> first place?
> For example, here it kind of makes sense to just compute
> with_returning_list
> within the function, by looking at thd->lex->returning_list.
> >    if (mysql_prepare_insert(thd, lex->query_tables,
> >                             lex->query_tables->table, lex->field_list, 0,
> >                             lex->update_list, lex->value_list,
> lex->duplicates,
> > -                           &select_lex->where, TRUE))
> > +                           &select_lex->where, TRUE,false))
> Put a space after last comma. And let's use FALSE.
> >      DBUG_RETURN(TRUE);
> >
> > +  /* If LEX::returning_list is not empty, we prepare the list */
> Uhm, the comment doesn't seem to match the code. You are checking sel_res,
> yet the comment says that if LEX:returning_list is not empty, prepare the
> list.
> > +  if (sel_res)
> > +       (void)sel_res->prepare(returning_list, NULL);
> Here we have a tab instead of spaces again.
> > +
> >    DBUG_ASSERT(select_lex->leaf_tables.elements != 0);
> >    List_iterator ti(select_lex->leaf_tables);
> >    TABLE_LIST *table;
> >    uint insert_tables;
> >
> > +
> This line is not necessary.
> >    if (select_lex->first_cond_optimization)
> >    {
> >      /* Back up leaf_tables list. */
> > @@ -3607,6 +3696,7 @@ bool mysql_insert_select_prepare(THD *thd)
> >    while ((table= ti++) && insert_tables--)
> >      ti.remove();
> >
> > +
> This line is not necessary.
> >    DBUG_RETURN(FALSE);
> >  }
> >
> > @@ -3617,7 +3707,7 @@ select_insert::select_insert(THD *thd_arg,
> TABLE_LIST *table_list_par,
> >                               List *update_fields,
> >                               List *update_values,
> >                               enum_duplicates duplic,
> > -                             bool ignore_check_option_errors):
> > +                             bool
> ignore_check_option_errors,select_result *result, TABLE_LIST *save_first):
> This line is still way too long. I remember mentioning this in a previous
> review, please look more closely and wrap this properly.
> >    select_result_interceptor(thd_arg),
> >    table_list(table_list_par), table(table_par), fields(fields_par),
> >    autoinc_value_of_last_inserted_row(0),
> > @@ -3630,6 +3720,8 @@ select_insert::select_insert(THD *thd_arg,
> TABLE_LIST *table_list_par,
> >    info.update_values= update_values;
> >    info.view= (table_list_par->view ? table_list_par : 0);
> >    info.table_list= table_list_par;
> > +  sel_result= result;
> > +  insert_table=      save_first;
> Trailing whitespace, you added a tab after =. Please delete the tab.
> >  }
> >
> >
> > @@ -3637,10 +3729,11 @@ int
> >  select_insert::prepare(List &values, SELECT_LEX_UNIT *u)
> >  {
> >    LEX *lex= thd->lex;
> > -  int res;
> > +  int res=0;
> Space after =.
> >    table_map map= 0;
> >    SELECT_LEX *lex_current_select_save= lex->current_select;
> >    DBUG_ENTER("select_insert::prepare");
> > +  SELECT_LEX* select_lex = thd->lex->first_select_lex();
> Delete space before =.
> >
> >    unit= u;
> >
> > @@ -3651,7 +3744,33 @@ select_insert::prepare(List &values,
> SELECT_LEX_UNIT *u)
> >    */
> >    lex->current_select= lex->first_select_lex();
> >
> > -  res= (setup_fields(thd, Ref_ptr_array(),
> > +  /*
> > +    We want the returning_list to point to insert table. But the
> context is masked.
> > +       So we swap it with the context saved during parsing stage.
> Tab mixed with spaces. Only use spaces. Traling whitespace, here and the
> line
> before.
> > +  */
> > +  if(sel_result)
> Trailing whitespace.
> > +  {
> Trailing whitespace.
> > +    swap_context(insert_table,select_lex->table_list.first);
> > +
> swap_context(select_lex->context.saved_table_list,select_lex->context.table_list);
> > +
> swap_context(select_lex->context.saved_name_resolution_table,select_lex->context.first_name_resolution_table);
> Line length exceeds 80 characters.
> > +
> > +    /*
> > +      Perform checks for LEX::returning_list like we do for other
> variant of INSERT.
> > +    */
> > +    res=((select_lex->with_wild && setup_wild(thd, table_list,
> > +                     thd->lex->returning_list, NULL,
> select_lex->with_wild,
> > +                     &select_lex->hidden_bit_fields)) ||
> > +                     setup_fields(thd, Ref_ptr_array(),
> > +                     thd->lex->returning_list, MARK_COLUMNS_READ, 0,
> NULL, 0));
> Tabs instead of spaces and mixed spaces. Not good. Also trailing
> whitespaces.
> Also, the parameters of setup_wild should be aligned to the first
> parenthesis
> of setup_wild. setup_fields should be aligned to the first parenthesis
> here.
> I also think there's one too many parantheses here, the outer most ones are
> not necessary..
> > +
> > +             /* Swap it back to retore the previous state for the rest
> of the function */
> Tabs instead of spaces.
> > +
> > +             swap_context(insert_table,select_lex->table_list.first);
> > +
> swap_context(select_lex->context.saved_table_list,select_lex->context.table_list);
> > +
>  swap_context(select_lex->context.saved_name_resolution_table,
> select_lex->context.first_name_resolution_table);
> Tabs and spaces mixed here. Watch line length.
> > +  }
> > +
> > +  res= res || (setup_fields(thd, Ref_ptr_array(),
> >                       values, MARK_COLUMNS_READ, 0, NULL, 0) ||
> >          check_insert_fields(thd, table_list, *fields, values,
> >                              !insert_into_view, 1, &map));
> The line that starts with "values" should be aligned to the first open
> parenthesis.
> > @@ -3822,13 +3941,26 @@ select_insert::prepare(List &values,
> SELECT_LEX_UNIT *u)
> >
> >  int select_insert::prepare2(JOIN *)
> >  {
> > +
> Trailing whitespace.
> >    DBUG_ENTER("select_insert::prepare2");
> > +  List& returning_list = thd->lex->returning_list;
> Delete space before =
> > +  LEX* lex = thd->lex;
> This function does not use the lex local variable at all, only via
> thd->lex.
> This line should be removed.
> >    if (thd->lex->current_select->options & OPTION_BUFFER_RESULT &&
> >        thd->locked_tables_mode <= LTM_LOCK_TABLES &&
> >        !thd->lex->describe)
> >      table->file->ha_start_bulk_insert((ha_rows) 0);
> >    if (table->validate_default_values_of_unset_fields(thd))
> >      DBUG_RETURN(1);
> > +
> > +  /* Same as the other variants of INSERT */
> > +  if (sel_result)
> > +  {
> > +       if(unlikely(sel_result->send_result_set_metadata(returning_list,
> > +               Protocol::SEND_NUM_ROWS |
> > +               Protocol::SEND_EOF)))
> The Protocol should be aligned to the last open parenthesis.
> You are mixing tabs with spaces here. Convert to spaces.
> > +
> > +               DBUG_RETURN(1);
> Mixing tabs with spaces again, convert to spaces.
> > +  }
> >    DBUG_RETURN(0);
> >  }
> >
> > @@ -3842,6 +3974,8 @@ void select_insert::cleanup()
> >  select_insert::~select_insert()
> >  {
> >    DBUG_ENTER("~select_insert");
> > +  sel_result=NULL;
> Add a space after =.
> > +  insert_table=NULL;
> Add a space after =.
> >    if (table && table->is_created())
> >    {
> >      table->next_number_field=0;
> > @@ -3857,6 +3991,8 @@ select_insert::~select_insert()
> >  int select_insert::send_data(List &values)
> >  {
> >    DBUG_ENTER("select_insert::send_data");
> > +  LEX* lex = thd->lex;
> Delete space before =.
> >
> > +  List& returning_list = thd->lex->returning_list;
> Delete space before =.
> >    bool error=0;
> >
> >    if (unit->offset_limit_cnt)
> > @@ -3889,8 +4025,18 @@ int select_insert::send_data(List &values)
> >        DBUG_RETURN(1);
> >      }
> >    }
> > -
> > +
> Trailing whitespaces added for no good reason.
> >    error= write_record(thd, table, &info);
> > +
> Trailing whitespaces.
> > +  /*
> > +    Sending the result set to the cliet after writing record. The
> reason is same
> > +    as other variants of insert.
> > +  */
> > +  if (sel_result && sel_result->send_data(returning_list) < 0)
> > +  {
> > +    error = 1;
> Delete space before =.
> > +    DBUG_RETURN(1);
> > +  }
> >    table->vers_write= table->versioned();
> >    table->auto_increment_field_not_null= FALSE;
> >
> > @@ -4024,7 +4170,7 @@ bool select_insert::send_ok_packet() {
> >    char  message[160];                           /* status message */
> >    ulonglong row_count;                          /* rows affected */
> >    ulonglong id;                                 /* last insert-id */
> > -
> > +  LEX *lex=thd->lex;
> This function does not use this variable at all. Please delete it if it's
> not
> used. All it does is produce a compiler warning.
> >    DBUG_ENTER("select_insert::send_ok_packet");
> >
> >    if (info.ignore)
> > @@ -4045,8 +4191,15 @@ bool select_insert::send_ok_packet() {
> >      (thd->arg_of_last_insert_id_function ?
> >       thd->first_successful_insert_id_in_prev_stmt :
> >       (info.copied ? autoinc_value_of_last_inserted_row : 0));
> > -
> > -  ::my_ok(thd, row_count, id, message);
> > +
> Trailing whitespace
> > +  /*
> > +    Client expects an EOF/Ok packet If LEX::returning_list is not empty
> and
> Trailing whitespace
> > +    if result set meta was sent. See explanation for other variants of
> INSERT.
> > +  */
> > +  if (sel_result)
> > +       sel_result->send_eof();
> > +  else
> > +    ::my_ok(thd, row_count, id, message);
> >
> >    DBUG_RETURN(false);
> >  }
> > diff --git a/sql/sql_insert.h b/sql/sql_insert.h
> > index a37ed1f31e5..314817b53d3 100644
> > --- a/sql/sql_insert.h
> > +++ b/sql/sql_insert.h
> > @@ -27,11 +27,11 @@ bool mysql_prepare_insert(THD *thd, TABLE_LIST
> *table_list, TABLE *table,
> >                            List &fields, List_item *values,
> >                            List &update_fields,
> >                            List &update_values, enum_duplicates
> duplic,
> > -                          COND **where, bool select_insert);
> > +                          COND **where, bool select_insert, bool
> with_returning_list);
> This line exceeds 80 characters. Please wrap it to 80 characters by moving
> the final function parameter to a new line.
> >  bool mysql_insert(THD *thd,TABLE_LIST *table,List &fields,
> >                    List &values, List &update_fields,
> >                    List &update_values, enum_duplicates flag,
> > -                  bool ignore);
> > +                  bool ignore, select_result* result);
> >  void upgrade_lock_type_for_insert(THD *thd, thr_lock_type *lock_type,
> >                                    enum_duplicates duplic,
> >                                    bool is_multi_insert);
> > diff --git a/sql/sql_lex.h b/sql/sql_lex.h
> > index 0e1d17d13f0..00cf3025efe 100644
> > --- a/sql/sql_lex.h
> > +++ b/sql/sql_lex.h
> > @@ -3091,6 +3092,8 @@ struct LEX: public Query_tables_list
> >    SELECT_LEX *current_select;
> >    /* list of all SELECT_LEX */
> >    SELECT_LEX *all_selects_list;
> > +  /* List of fields and expression for returning part of insert*/
> Add a space before closing the comment.
> > +  List returning_list;
> >    /* current with clause in parsing if any, otherwise 0*/
> >    With_clause *curr_with_clause;
> >    /* pointer to the first with clause in the current statement */
> > diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> > index 01d0ed1c383..852078b254b 100644
> > --- a/sql/sql_parse.cc
> > +++ b/sql/sql_parse.cc
> > @@ -4489,6 +4489,7 @@ mysql_execute_command(THD *thd)
> >    case SQLCOM_INSERT:
> >    {
> >      WSREP_SYNC_WAIT(thd, WSREP_SYNC_WAIT_BEFORE_INSERT_REPLACE);
> > +     select_result * sel_result = NULL;
> Tabs instead of spaces. Delete the space after *. Delete the space before
> =.
> >      DBUG_ASSERT(first_table == all_tables && first_table != 0);
> >
> >      WSREP_SYNC_WAIT(thd, WSREP_SYNC_WAIT_BEFORE_INSERT_REPLACE);
> > @@ -4509,9 +4510,43 @@ mysql_execute_command(THD *thd)
> >        break;
> >
> >      MYSQL_INSERT_START(thd->query());
> > +
> > +     Protocol* UNINIT_VAR(save_protocol);
> > +     bool replaced_protocol = false;
> Tabs instead of spaces. Delete space before =
> > +
> > +     if (!thd->lex->returning_list.is_empty())
> > +     {
> > +    /*increment the status variable. This is useful for feedback
> plugin*/
> I would remove this comment, the function name is suggestive enough.
> > +    status_var_increment(thd->status_var.feature_insert_returning);
> > +
> > +              /*This is INSERT ... RETURNING. It will return output to
> the client */
> Space after start of comment.
> > +             if (thd->lex->analyze_stmt)
> > +             {
> > +      /*
> > +                       Actually, it is ANALYZE .. INSERT .. RETURNING.
> We need to produce
> > +                       output and then discard it.
> > +      */
> Trailign whitespace tab.
> > +                     sel_result = new (thd->mem_root)
> select_send_analyze(thd);
> Delete space before =
> > +                     replaced_protocol = true;
> Delete space before =
> > +                     save_protocol = thd->protocol;
> Delete space before =
> > +                     thd->protocol = new Protocol_discard(thd);
> Delete space before =
> > +             }
> > +             else
> > +             {
> > +                     if (!lex->result && !(sel_result = new
> (thd->mem_root) select_send(thd)))
> Delete space before =
> > +                             goto error;
> > +             }
> > +     }
> Tabs instead of spaces for this whole part.
> > +
> Trailing whitespaces.
> >      res= mysql_insert(thd, all_tables, lex->field_list,
> lex->many_values,
> >                     lex->update_list, lex->value_list,
> > -                      lex->duplicates, lex->ignore);
> > +                      lex->duplicates, lex->ignore, lex->result ?
> lex->result : sel_result);
> This line overflows the 80 character line rule.
> Wrap the last parameter to another line. IT also makes things a lot easier
> to read.
> > +     if (replaced_protocol)
> > +     {
> Tabs instead of spaces for these 2 lines.
> > +    delete thd->protocol;
> > +    thd->protocol= save_protocol;
> > +  }
> > +     delete sel_result;
> Tab instead of spaces.
> >      MYSQL_INSERT_DONE(res, (ulong) thd->get_row_count_func());
> >      /*
> >        If we have inserted into a VIEW, and the base table has
> > @@ -4547,6 +4582,8 @@ mysql_execute_command(THD *thd)
> >    {
> >      WSREP_SYNC_WAIT(thd, WSREP_SYNC_WAIT_BEFORE_INSERT_REPLACE);
> >      select_insert *sel_result;
> > +    TABLE_LIST *save_first=  NULL;
> Tab instead of space after =.
> > +    select_result* result = NULL;
> Delete space after =. Move * close to result not to select_result.
> >      bool explain= MY_TEST(lex->describe);
> >      DBUG_ASSERT(first_table == all_tables && first_table != 0);
> >      WSREP_SYNC_WAIT(thd, WSREP_SYNC_WAIT_BEFORE_UPDATE_DELETE);
> > @@ -4595,6 +4632,34 @@ mysql_execute_command(THD *thd)
> >          Only the INSERT table should be merged. Other will be handled by
> >          select.
> >        */
> > +
> > +       Protocol* UNINIT_VAR(save_protocol);
> > +       bool replaced_protocol = false;
> Tabs instead of spaces. Delete space before =.
> > +
> > +       if (!thd->lex->returning_list.is_empty())
> > +       {
> Tabs instead of spaces.
> > +      /*increment the status variable. This is useful for feedback
> plugin*/
> I would remove this comment, the function name is suggestive enough.
> > +      status_var_increment(thd->status_var.feature_insert_returning);
> > +
> > +               /* This is INSERT ... RETURNING.  It will return output
> to the client */
> > +               if (thd->lex->analyze_stmt)
> > +               {
> > +                       /*
> > +                             Actually, it is ANALYZE .. INSERT ..
> RETURNING. We need to produce
> > +                             output and then discard it.
> > +                       */
> > +                       result = new (thd->mem_root)
> select_send_analyze(thd);
> > +                       replaced_protocol = true;
> > +                       save_protocol = thd->protocol;
> > +                       thd->protocol = new Protocol_discard(thd);
> > +               }
> > +               else
> > +               {
> > +                       if (!lex->result && !(result = new
> (thd->mem_root) select_send(thd)))
> > +                               goto error;
> > +               }
> > +       }
> > +
> >        /* Skip first table, which is the table we are inserting in */
> >        TABLE_LIST *second_table= first_table->next_local;
> >        /*
> > @@ -4604,18 +4669,33 @@ mysql_execute_command(THD *thd)
> >          TODO: fix it by removing the front element (restoring of it
> should
> >          be done properly as well)
> >        */
> > +
> Trailing whitespaces for this line.
> > +      /*
> > +      Also, if items are present in returning_list, then we need those
> items
> > +      to point to INSERT table during setup_fields() and setup_wild().
> But
> > +      it gets masked before that. So we save the values in saved_first,
> > +      saved_table_list and saved_first_name_resolution_context before
> they are masked.
> > +      We will later swap the saved values with the masked values if
> returning_list
> > +      is not empty in INSERT...SELECT...RETURNING.
> This comment text needs to indented 2 more spaces. Remove trailing
> whitespaces. Also, make sure to keep within 80 characters per line.
> > +      */
> > +
> > +                     TABLE_LIST
> *save_first=select_lex->table_list.first;
> Tabs instead of spaces. Add space after =.
> >        select_lex->table_list.first= second_table;
> > +
>  select_lex->context.saved_table_list=select_lex->context.table_list;
> Tabs instead of spaces. Add space after =.
> > +                     select_lex->context.saved_name_resolution_table=
> Tabs instead of spaces.
> > +        select_lex->context.first_name_resolution_table;
> >        select_lex->context.table_list=
> >          select_lex->context.first_name_resolution_table= second_table;
> > -      res= mysql_insert_select_prepare(thd);
> > -      if (!res && (sel_result= new (thd->mem_root) select_insert(thd,
> > -
>  first_table,
> > -
>  first_table->table,
> > -
>  &lex->field_list,
> > -
>  &lex->update_list,
> > -
>  &lex->value_list,
> > -
>  lex->duplicates,
> > -
>  lex->ignore)))
> > +      res= mysql_insert_select_prepare(thd,lex->result ? lex->result :
> result);
> > +      if (!res && (sel_result= new (thd->mem_root) select_insert(thd,
> first_table,
> > +
> first_table->table,
> > +
> &lex->field_list,
> > +
> &lex->update_list,
> > +
> &lex->value_list,
> > +
> lex->duplicates,
> > +
> lex->ignore,
> > +
> lex->result ? lex->result : result,
> > +
> save_first)))
> This is ugly. I suggest you keep just new (thd->mem_root) on one line and
> wrap
> everything to a new line. Something like:
> +      if (!res &&
> +          (sel_result= new (thd->mem_root)
> +                         select_insert(thd, first_table,
> +                                       first_table->table,
> +                                       &lex->field_list,
> +                                       &lex->update_list,
> +                                       &lex->value_list,
> +                                       lex->duplicates,
> +                                       lex->ignore,
> +                                       lex->result ? lex->result : result,
> +                                       save_first)))
> >
> >        {
> >          if (lex->analyze_stmt)
> >
> ((select_result_interceptor*)sel_result)->disable_my_ok_calls();
> > @@ -4630,6 +4710,7 @@ mysql_execute_command(THD *thd)
> >            TODO: this is workaround. right way will be move invalidating
> in
> >            the unlock procedure.
> >          */
> > +
> Extra line added for no good reason.
> >          if (!res && first_table->lock_type ==
> TL_WRITE_CONCURRENT_INSERT &&
> >              thd->lock)
> >          {
> > @@ -4650,8 +4731,13 @@ mysql_execute_command(THD *thd)
> >            sel_result->abort_result_set();
> >          }
> >          delete sel_result;
> > +        delete result;
> > +      }
> > +      if (replaced_protocol)
> > +      {
> > +        delete thd->protocol;
> > +        thd->protocol= save_protocol;
> >        }
> > -
> No need to delete this empty line.
> >        if (!res && (explain || lex->analyze_stmt))
> >          res= thd->lex->explain->send_explain(thd);
> >
> > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc
> > index 8088b6923f2..2f5d0f17451 100644
> > --- a/sql/sql_prepare.cc
> > +++ b/sql/sql_prepare.cc
> > @@ -1296,7 +1296,7 @@ static bool mysql_test_insert(Prepared_statement
> *stmt,
> >
> >      if (mysql_prepare_insert(thd, table_list, table_list->table,
> >                               fields, values, update_fields,
> update_values,
> > -                             duplic, &unused_conds, FALSE))
> > +                             duplic, &unused_conds, FALSE,false))
> FALSE mixed with false. As this function call uses FALSE, let's use FALSE
> too.
> I know, the file is inconsistent, it uses FALSE and false at times. :(
> Let's do our best here and not make things stranger than they have to be.
> Space after last comma too.
> Like so:                   duplic, &unused_conds, FALSE, FALSE))
> >        goto error;
> >
> >      value_count= values->elements;
> > @@ -2154,7 +2154,7 @@ static int mysql_insert_select_prepare_tester(THD
> *thd)
> >      thd->lex->first_select_lex()->context.first_name_resolution_table=
> >      second_table;
> >
> > -  return mysql_insert_select_prepare(thd);
> > +  return mysql_insert_select_prepare(thd,NULL);
> space after , please.
> >  }
> >
> >
> > diff --git a/sql/sql_yacc.yy b/sql/sql_yacc.yy
> > index 183a2504b70..c7706ca3c1f 100644
> > --- a/sql/sql_yacc.yy
> > +++ b/sql/sql_yacc.yy
> > @@ -1911,11 +1912,14 @@ bool my_yyoverflow(short **a, YYSTYPE **b,
> size_t *yystacksize);
> >  %type  text_literal
> >
> >  %type 
> > -        expr_list opt_udf_expr_list udf_expr_list when_list
> when_list_opt_else
> > +        opt_insert_update
> Trailing whitespace.
> > +        opt_select_expressions expr_list
> Trailing whitespace.
> > +        opt_udf_expr_list udf_expr_list when_list when_list_opt_else
> >          ident_list ident_list_arg opt_expr_list
> >          decode_when_list_oracle
> >          execute_using
> >          execute_params
> > +        select_item_list
> >
> >  %type 
> >          sp_cursor_stmt_lex
> > @@ -9223,6 +9226,7 @@ query_specification_start:
> >            select_item_list
> >            {
> >              Select->parsing_place= NO_MATTER;
> > +            Select->item_list=*($5);
> Space after =.
> >            }
> >            ;
> >
> > @@ -9544,9 +9548,24 @@ opt_lock_wait_timeout_new:
> >          }
> >        ;
> >
> > +      /*
> > +        Here, we make select_item_list return List to prevent it
> from adding
> > +        everything to SELECT_LEX::item_list. If items are already there
> in the item_list
> > +        then using RETURNING with INSERT...SELECT is not possible
> because rules occuring
> > +        after insert_values add everything to SELECT_LEX::item_list.
> > +      */
> This comment must be wrapped at 80 lines.
> > +
> Delete this final line.
> >  select_item_list:
> >            select_item_list ',' select_item
> > +          {
> > +            $1->push_back($3,thd->mem_root);
> Space after ,
> > +            $$=$1;
> Space after =
> > +          }
> >          | select_item
> > +          {
> > +            if (unlikely(!($$= List::make(thd->mem_root, $1))))
> > +              MYSQL_YYABORT;
> > +          }
> >          | '*'
> >            {
> >              Item *item= new (thd->mem_root)
> > @@ -9554,24 +9573,23 @@ select_item_list:
> >                                       NULL, NULL, &star_clex_str);
> >              if (unlikely(item == NULL))
> >                MYSQL_YYABORT;
> > -            if (unlikely(add_item_to_list(thd, item)))
> > +            if (unlikely(!($$= List::make(thd->mem_root, item))))
> >                MYSQL_YYABORT;
> >              (thd->lex->current_select->with_wild)++;
> > +
> > +            (thd->lex->current_select->with_wild)++;
> This double's with_wild. This is a bug.
> >            }
> >          ;
> >
> >  select_item:
> >            remember_name select_sublist_qualified_asterisk remember_end
> >            {
> > -            if (unlikely(add_item_to_list(thd, $2)))
> > -              MYSQL_YYABORT;
> > +            $$=$2;
> >            }
> >          | remember_name expr remember_end select_alias
> >            {
> >              DBUG_ASSERT($1 < $3);
> > -
> > -            if (unlikely(add_item_to_list(thd, $2)))
> > -              MYSQL_YYABORT;
> > +            $$=$2;
> >              if ($4.str)
> >              {
> >                if (unlikely(Lex->sql_command == SQLCOM_CREATE_VIEW &&
> > @@ -13307,13 +13325,15 @@ insert:
> >              Select->set_lock_for_tables($3, true);
> >              Lex->current_select= Lex->first_select_lex();
> >            }
> > -          insert_field_spec opt_insert_update
> > -          {
> > +          insert_field_spec opt_insert_update opt_select_expressions
> > +          {
> Trailing whitespace.
> > +            if ($9)
> > +              Lex->returning_list=*($9);
> >              Lex->pop_select(); //main select
> >              if (Lex->check_main_unit_semantics())
> >                MYSQL_YYABORT;
> >            }
> > -        ;
> > +         ;
> Trailing whitespace tab. Additionally the semicolon is not
> Why change this? The semicolon is now indented wrong. Also trailing tab
> introduced after.
> >
> >  replace:
> >            REPLACE
> > @@ -13331,13 +13351,15 @@ replace:
> >              Select->set_lock_for_tables($3, true);
> >              Lex->current_select= Lex->first_select_lex();
> >            }
> > -          insert_field_spec
> > +          insert_field_spec opt_select_expressions
> >            {
> > +            if ($7)
> > +              Lex->returning_list=*($7);
> Space after =.
> >              Lex->pop_select(); //main select
> >              if (Lex->check_main_unit_semantics())
> >                MYSQL_YYABORT;
> >            }
> > -        ;
> > +          ;
> Why change this? The semicolon is now indented wrong.
> >
> >  insert_lock_option:
> >            /* empty */
> > @@ -13389,8 +13411,8 @@ insert_table:
> >          ;
> >
> >  insert_field_spec:
> > -          insert_values {}
> > -        | insert_field_list insert_values {}
> > +          insert_values
> Trailing whitespace.
> > +        | insert_field_list insert_values
> >          | SET
> >            {
> >              LEX *lex=Lex;
> > @@ -13732,6 +13754,8 @@ single_multi:
> >            {
> >              if ($3)
> >                Select->order_list= *($3);
> > +            if($5)
> > +              Select->item_list=*($5);
> Space after if. Space after =.
> >              Lex->pop_select(); //main select
> >            }
> >          | table_wild_list
> > @@ -13764,9 +13788,16 @@ single_multi:
> >            }
> >          ;
> >
> > +      /*
> Trailing whitespace.
> > +         Return NULL if the  rule is empty else return the list of
> items
> Trailing whitespace.
> > +         in the select expression
> Trailing whitespce. Put a fullstop at the end of the sentence.
> > +      */
> Comment should start at the beginning of line, not indented in this case.
> >  opt_select_expressions:
> > -          /* empty */
> > -        | RETURNING_SYM select_item_list
> > +          /* empty */ {$$=NULL;}
> Space after =. Space after {. Space after ;
> > +        | RETURNING_SYM select_item_list
> > +          {
> > +            $$=$2;
> Space after =.
> > +          }
> >          ;
> >
> >  table_wild_list:
>
>