7 Aug
2019
7 Aug
'19
4:55 p.m.
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 <vicentiu@mariadb.org>
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<Item> *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<Item> *fields_par,
> > List<Item> *update_fields, List<Item> *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:
> <function_name>(TYPE1 arg1, TYPE2 arg2,
> TYPE3 arg3, TYPE4 arg4,
> TYPE5 arg5)
>
> > ~select_insert();
> > int prepare(List<Item> &list, SELECT_LEX_UNIT *u);
> > virtual int prepare2(JOIN *join);
> > @@ -5520,7 +5527,7 @@ class select_create: public select_insert {
> > List<Item> &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 <typename T>
> > +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<Item> &fields, List<Item>
> *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<Item> &update_fields,
> > List<Item> &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<Item>& 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<Item> &fields, List_item
> *values,
> > List<Item> &update_fields, List<Item>
> &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<Item>& returning_list = thd->lex->returning_list;
> For assignment operators no space before it. Like so:
> List<Item>& 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<TABLE_LIST> 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<Item> *update_fields,
> > List<Item> *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<Item> &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<Item> &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<Item> &values,
> SELECT_LEX_UNIT *u)
> >
> > int select_insert::prepare2(JOIN *)
> > {
> > +
> Trailing whitespace.
> > DBUG_ENTER("select_insert::prepare2");
> > + List<Item>& 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<Item> &values)
> > {
> > DBUG_ENTER("select_insert::send_data");
> > + LEX* lex = thd->lex;
> Delete space before =.
> >
> > + List<Item>& 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<Item> &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<Item> &fields, List_item *values,
> > List<Item> &update_fields,
> > List<Item> &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<Item> &fields,
> > List<List_item> &values, List<Item> &update_fields,
> > List<Item> &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<Item> 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 <item_basic_constant> text_literal
> >
> > %type <item_list>
> > - 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>
> > 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<Item> 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<Item>::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<Item>::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:
>
>