August 2019 - developers - lists.mariadb.org

[Maria-developers] Weekly Report
by Rucha Deodhar 19 Aug '19

19 Aug '19

Report for week 12: Hi! This week I merged some test cases in respective files into one so that the tests can be under 80 lines and also improved the documentation. Documentation for INSERT...RETURNING: https://docs.google.com/document/d/1EknNlh-J72cUlCg9rvcujUeZdzu5uzm0hmHG3qg… REPLACE...RETURNING: https://docs.google.com/document/d/1UMy5fi-j6yIObYIRwTteaJKHU5F-wPoEAVgVbeu… I also fixed feature_insert_returning system variable by adding some code and improving the test file. The test is passing. insert_returning_datatypes.result and replace_returning_datatypes.result files were showing binary data. To fix it, it was suggested to create a .reject and .result files, hexdump of the files, diff and giving a printable ASCII character as the input in BIT field because BIT is printed as VARCHAR. One thing I noticed was, after fixing the files they were still showing as binary files, which was probably because of the diffs. So I removed the existing files from my repo and pushed new files then cleaned up the commits again and fixed the 80 character per line rule for new tests. My github repo is up to date with latest changes. Regards, Rucha Deodhar

1 0

Re: [Maria-developers] e2a82094332: MENT-253 Server Audit v2 does not work with PS protocol: server crashes in filter_query_type.
by Sergei Golubchik 14 Aug '19

14 Aug '19

Hi, Alexey! On Aug 14, Alexey Botchkov wrote: > revision-id: e2a82094332 (mariadb-10.4.7-123-ge2a82094332) > parent(s): 691654721b3 > author: Alexey Botchkov <holyfoot(a)mariadb.com> > committer: Alexey Botchkov <holyfoot(a)mariadb.com> > timestamp: 2019-08-07 11:49:54 +0400 > message: > > MENT-253 Server Audit v2 does not work with PS protocol: server crashes in filter_query_type. > > Set the thd->query_string where possible. > server_audit2 should not crash when gets empty event->query. > > --- > plugin/server_audit2/server_audit.c | 15 ++++++++++++--- > sql/sql_prepare.cc | 22 ++++++++++++++++++++++ > 2 files changed, 34 insertions(+), 3 deletions(-) Test case? With normal and error code paths? > diff --git a/sql/sql_prepare.cc b/sql/sql_prepare.cc > index 74723d5bd91..d2cb4e9b372 100644 > --- a/sql/sql_prepare.cc > +++ b/sql/sql_prepare.cc > @@ -2600,6 +2600,14 @@ void mysqld_stmt_prepare(THD *thd, const char *packet, uint packet_length) > > if (stmt->prepare(packet, packet_length)) > { > + /* > + Set the thd->query_string with the current query so the > + audit plugin gets the meaningful notification. say that it's an error case. Like Prepare failed. Set the thd->query_string with the current query so the audit plugin gets the meaningful notification when it gets the error notification. > + */ > + if (alloc_query(thd, stmt->query_string.str(), stmt->query_string.length())) > + { > + thd->set_query(0, 0); > + } > /* Statement map deletes statement on erase */ > thd->stmt_map.erase(stmt); > thd->clear_last_stmt(); > @@ -3184,6 +3192,13 @@ static void mysql_stmt_execute_common(THD *thd, > char llbuf[22]; > my_error(ER_UNKNOWN_STMT_HANDLER, MYF(0), static_cast<int>(sizeof(llbuf)), > llstr(stmt_id, llbuf), "mysqld_stmt_execute"); > + /* > + Set thd->query_string with the stmt_id so the > + audit plugin gets the meaningful notification. Same here. > + */ > + if (alloc_query(thd, llbuf, strlen(llbuf))) > + thd->set_query(0, 0); > + > DBUG_VOID_RETURN; > } > stmt->read_types= read_types; > @@ -3939,6 +3954,13 @@ bool Prepared_statement::prepare(const char *packet, uint packet_len) > DBUG_RETURN(TRUE); > } > > + /* > + We'd like to have thd->query to be set to the actual query > + after the function ends. > + This value will be sent to audit pulgins later. 1. typo: "pulgins" 2. Mention explicitly that "query string is allocated in the stmt arena, not in the thd arena. But it's safe here, because stmt always has a longer lifetime than thd arena." - or something along these lines. > + */ > + stmt_backup.query_string= thd->query_string; > + > old_stmt_arena= thd->stmt_arena; > thd->stmt_arena= this; > > Regards, Sergei VP of MariaDB Server Engineering and security(a)mariadb.org

1 0

[Maria-developers] Weekly Report
by Rucha Deodhar 12 Aug '19

12 Aug '19

Report for week 11: Hello! This week I worked on cleaning up the code, commits and test cases. I was a little less familiar with git rebase -i so tried a couple of things on a dummy repo first and then cleaned up the commits on my repo. I also worked on the coding style review and removed all the trailing white spaces and tabs mentioned in the review. To remove the last argument of mysql_prepare_insert() I tried a couple of things like not calling setup_fields() and setup_wild() separately for INSERT...SELECT...RETURNING and calling std::swap if select_insert is true. Like so if(!lex->returning_list.is_empty()) { if (select_insert) //std::swap() setup_fields() and setup_wild() if (selec_insert) //std::swap() } and also changing where the above functions are called. But that didn't work. So now calling it separately for INSERT..SELECT..RETURNING and other variants. I added separate function to sql_base.cc and sql_base.h which calls setup_fields() and setup_wild() so that not only the last argument can be removed but also lines of code can be reduced. All the occurrences of (wild_num && setup_wild(thd, table_list, field_list, NULL, wild_num, &select_lex->hidden_bit_fields)) || setup_fields(thd, Ref_ptr_array(), field_list, MARK_COLUMNS_READ, NULL, NULL, 0) can be replaced with setup_returning_fields(). (Since this is used in insert..returning, insert..select..returning, delete...returning and update...returning too). On some lines of the test cases, the characters were coming to more than 80 characters. So fixed that and added some more test cases (INSERT...IGNORE..RETURNING and fields with auto_increment). Finally I merged all the commits made this week into one commit. My github repo is up to date with the latest changes. Here is the link for the same: https://github.com/rucha174/server/commit/0d7eb4198cc0aadba42acb4960c175702… Regards, Rucha Deodhar

1 0

[Maria-developers] Coding Style Review INSERT RETURNING
by Vicențiu Ciorbaru 07 Aug '19

07 Aug '19

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:

2 1

[Maria-developers] Weekly Report
by Rucha Deodhar 05 Aug '19

05 Aug '19

Report for week 10: Hello! Report for Week 10: Hello! This week I worked on adding system variable feature_insert_returning which is useful for feedback plugin. This variable increments each time INSERT...RETURNING or REPLACE...RETURNING is used. Before beginning, I did some reading about feedback plugin and referred feature_subquery for adding feature_insert_returning. Initially I was facing difficulty in finding the test file that was to be updated. I was searching for test file in sys_vars test suite. Zulip conversation helped me find the right test file and also a missed step for publishing variable for SHOW STATUS. I have made necessary changes in the code and have updated the test and result file. I also found out that the current implementation was not showing the expected output when AUTO_INCREMENT is used and the fields are not given explicitly. On using RETURNING it was showing the value of all the rows in auto_increment field as 0. Fixed that by calling send_data() after write_record(). This also fixed another thing. The previous implementation, for INSERT ... ON DUPLICATE KEY UPDATE...RETURNING was not showing the updated value in the result set. Example: If the table t1 has 1 row: id1=1 and val1='A'. The below statement INSERT INTO t1(id1,val1) VALUES(1,'B') ON DUPLICATE KEY UPDATE val1='C' returning *; was showing id1=1 and val='B' for the result even if it inserts val1='C'. Now it is fixed and is returning val1='C'. I have updated the result file for insert_returning. I also fixed line endings for tests and added more comments to make our implementation more understandable. I github repo is up to date with the latest changes. Regards, Rucha Deodhar.

1 0

Re: [Maria-developers] [Commits] 1e58c579f5b: MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log makes data on master and slave diverge
by Sachin Setiya 02 Aug '19

02 Aug '19

Hi Sujatha! On Thu, Aug 1, 2019 at 4:16 PM sujatha <sujatha.sivakumar(a)mariadb.com> wrote: > > revision-id: 1e58c579f5b61644a74045a0470d2c62b343eb75 (mariadb-10.1.41-3-g1e58c579f5b) > parent(s): 0bb8f33b55cc016d0ede86b97990a3ee30dcb069 > author: Sujatha > committer: Sujatha > timestamp: 2019-08-01 16:16:05 +0530 > message: > > MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log makes data on master and slave diverge > > Problem: > ======= > Failed CREATE OR REPLACE TEMPORARY TABLE statement which dropped the table but > failed at a later stage of creation of temporary table is not written to > binarylog in row based replication. This causes the slave to diverge. > > Analysis: > ======== > CREATE OR REPLACE statements work as shown below. > > CREATE OR REPLACE TABLE table_name (a int); > is basically the same as: > > DROP TABLE IF EXISTS table_name; > CREATE TABLE table_name (a int); > > Hence every CREATE OR REPLACE TABLE command which dropped the table should be > written to binary log, even when following CREATE TABLE part fails. In order > to achieve this, during the execution of CREATE OR REPLACE command, when a > table is dropped 'thd->log_current_statement' flag is set. When table creation > results in an error within 'mysql_create_table' code, the error handling part > looks for this flag. If it is set the failed CREATE OR REPLACE statement is > written into the binary log inspite of error. This ensure that slave doesn't > diverge from the master. In case of row based replication the error handling > code returns very early, if the table is of type temporary. This is done based > on the assumption that temporary tables are not replicated in row based > replication. > > It fails to handle the cases where a temporary table was created as part of > statement based replication at an earlier stage and the binary log format was > changed to row because of an unsafe statement. In this case when a CREATE OR > REPLACE statement is executed on this temporary table it will dropped but the > query will not be written to binary log. Hence slave diverges. > > Fix: > === > In error handling code check the return status of create table operation. If > it is successful and replication mode is row based and table is of type > temporary then return. Other wise proceed further to the code which checks for > thd->log_current_statement flag and does appropriate logging. > > --- > .../suite/rpl/r/rpl_create_or_replace_fail.result | 18 +++++++ > .../suite/rpl/t/rpl_create_or_replace_fail.test | 56 ++++++++++++++++++++++ > sql/sql_table.cc | 2 +- > 3 files changed, 75 insertions(+), 1 deletion(-) > > diff --git a/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result b/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result > new file mode 100644 > index 00000000000..57178f0efbe > --- /dev/null > +++ b/mysql-test/suite/rpl/r/rpl_create_or_replace_fail.result > @@ -0,0 +1,18 @@ > +include/master-slave.inc > +[connection master] > +CREATE TEMPORARY TABLE t1 (a INT NOT NULL); > +LOAD DATA INFILE 'x' INTO TABLE x; > +ERROR 42S02: Table 'test.x' doesn't exist > +CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x); > +ERROR HY000: Cannot create temporary table with partitions > +"************** DROP TEMPORARY TABLE Should be present in Binary log **************" > +include/show_binlog_events.inc > +Log_name Pos Event_type Server_id End_log_pos Info > +master-bin.000001 # Gtid # # GTID #-#-# > +master-bin.000001 # Query # # use `test`; CREATE TEMPORARY TABLE t1 (a INT NOT NULL) > +master-bin.000001 # Gtid # # GTID #-#-# > +master-bin.000001 # Query # # use `test`; CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x) > +CREATE TABLE t1 (b INT); > +INSERT INTO t1 VALUES (NULL); > +DROP TABLE t1; > +include/rpl_end.inc > diff --git a/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test b/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test > new file mode 100644 > index 00000000000..e75f34b0b56 > --- /dev/null > +++ b/mysql-test/suite/rpl/t/rpl_create_or_replace_fail.test > @@ -0,0 +1,56 @@ > +# ==== Purpose ==== > +# > +# Test verifies that failed CREATE OR REPLACE TEMPORARY TABLE statement which > +# dropped the table but failed at a later stage of creation of temporary table > +# is written to binarylog in row based replication. > +# > +# ==== Implementation ==== > +# > +# Steps: > +# 0 - Have mixed based replication mode. > +# 1 - Create a temporary table. It will be replicated as mixed replication > +# mode is in use. > +# 2 - Execute an unsafe statement which will switch current statement > +# binlog format to 'ROW'. i.e If binlog_format=MIXED, there are open > +# temporary tables, and an unsafe statement is executed, then subsequent > +# statements are logged in row format. Why not just set binlog format to row ? > +# 3 - Execute a CREATE OR REPLACE TEMPORARY TABLE statement which tries to > +# create partitions on temporary table. Since it is not supported it will > +# fail. > +# 4 - Check the binary log output to ensure that the failed statement is > +# written to the binary log. > +# 5 - Slave should be up and running and in sync with master. > +# > +# ==== References ==== > +# > +# MDEV-18930: Failed CREATE OR REPLACE TEMPORARY not written into binary log > +# makes data on master and slave diverge > +# > + > +--source include/have_partition.inc > +--source include/have_binlog_format_mixed.inc > +--source include/master-slave.inc > + > +CREATE TEMPORARY TABLE t1 (a INT NOT NULL); > + > +# Execute an unsafe statement which switches replication mode internally from > +# "STATEMENT" to "ROW". > +--error ER_NO_SUCH_TABLE > +LOAD DATA INFILE 'x' INTO TABLE x; > + > +--error ER_PARTITION_NO_TEMPORARY > +CREATE OR REPLACE TEMPORARY TABLE t1 (x INT) PARTITION BY HASH(x); > + > +--echo "************** DROP TEMPORARY TABLE Should be present in Binary log **************" > +--source include/show_binlog_events.inc > + > +CREATE TABLE t1 (b INT); > +INSERT INTO t1 VALUES (NULL); > +--sync_slave_with_master > + > +# Cleanup > +--connection master > +DROP TABLE t1; > + > +--source include/rpl_end.inc > + > diff --git a/sql/sql_table.cc b/sql/sql_table.cc > index 656834c7852..fe923d73200 100644 > --- a/sql/sql_table.cc > +++ b/sql/sql_table.cc > @@ -5092,7 +5092,7 @@ bool mysql_create_table(THD *thd, TABLE_LIST *create_table, > > err: > /* In RBR we don't need to log CREATE TEMPORARY TABLE */ > - if (thd->is_current_stmt_binlog_format_row() && create_info->tmp_table()) > + if (!result && thd->is_current_stmt_binlog_format_row() && create_info->tmp_table()) > DBUG_RETURN(result); Can we only binary log only the drop part ? not the create part. > > if (create_info->tmp_table()) > _______________________________________________ > commits mailing list > commits(a)mariadb.org > https://lists.askmonty.org/cgi-bin/mailman/listinfo/commits -- Regards Sachin Setiya Software Engineer at MariaDB

1 0