maria-developers team mailing list archive
-
maria-developers team
-
Mailing list archive
-
Message #11924
Coding Style Review INSERT RETURNING
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:
Follow ups
