@@ -1287,7 +1294,8 @@ static void rename_in_stat_tables(THD *thd, DDL_LOG_ENTRY *ddl_log_entry,
|
*/
|
|
static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
|
- DDL_LOG_ENTRY *ddl_log_entry)
|
+ DDL_LOG_ENTRY *ddl_log_entry,
|
+ bool report_error)
|
|
Please add a function comment about the meaning of the above parameters
|
and also explain when report_error is set.
|
|
> @@ -1320,7 +1328,8 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
|
> DBUG_RETURN(FALSE);
|
>
|
> handler_name= ddl_log_entry->handler_name;
|
> - thd->push_internal_handler(&no_such_table_handler);
|
> + if (!report_error)
|
> + thd->push_internal_handler(&no_such_table_handler);
|
|
Add a comment before the 'if' (or function comment), something like:
|
|
/*
|
If report_error is set we want the user to see all errors as we expect no
|
errors. If report_error is false, we are in recovery and we should only
|
print unexpected errors, not errors for trying to delete files that may
|
not exists.
|
*/
|
|
> @@ -1459,11 +1472,11 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
|
> /* fall through */
|
> case DDL_RENAME_PHASE_TABLE:
|
> /* Restore frm and table to original names */
|
> - execute_rename_table(ddl_log_entry, file,
|
> - &ddl_log_entry->db, &ddl_log_entry->name,
|
> - &ddl_log_entry->from_db, &ddl_log_entry->from_name,
|
> - 0,
|
> - from_path, to_path);
|
> + flags= report_error ? FN_FROM_IS_TMP : 0;
|
|
The above needs a comment. It should explain why 'flag' is depending
|
on report_error and not ddl_log_entry->flags!
|
|
I don't understand why the execution should be different runtime
|
compared to during startup recovery.
|
|
> if (increment_phase(entry_pos))
|
> break;
|
> /* Fall through */
|
> @@ -1765,31 +1785,6 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
|
> }
|
> strxnmov(to_path, sizeof(to_path)-1, path.str, reg_ext, NullS);
|
> mysql_file_delete(key_file_frm, to_path, MYF(MY_WME|MY_IGNORE_ENOENT));
|
|
We should probably set error if the above fails as we may not have a
|
error handler to catch this anymore.
|
|
> @@ -2296,11 +2291,27 @@ static int ddl_log_execute_action(THD *thd, MEM_ROOT *mem_root,
|
> }
|
>
|
> end:
|
> + if (report_error)
|
> + {
|
> + if (error && file)
|
> + {
|
> + TABLE_SHARE share;
|
Better to bzero the above, just in case.
|
> + share.db= ddl_log_entry->db;
|
> + share.table_name= ddl_log_entry->name;
|
> + share.normalized_path= ddl_log_entry->tmp_name;
|
> + /* TODO: make TABLE_SHARE-independent handler::print_error()? */
|
|
Cannot be done as the lower level may not know the user visible table name.
|
However, we could create a helper function to do the error printing when
|
we only have a file pointer (if we have multiple cases when this is needed).
|
|
> + file->change_table_ptr(NULL, &share);
|
> + file->print_error(error, MYF(0));
|
|
Shouldn't some errors already have been reported?
|
At least errors from mysql_file_delete() are already reported and should not
|
be reported again.
|
For example, in your patch you set error to -1, which should not be sent to
|
print_error.
|
Better to have two variables, 'handler_error' and 'error' and you only do the
|
above if 'handler_error' is set.
|
|
> @@ -2395,11 +2406,13 @@ void ddl_log_release_memory_entry(DDL_LOG_MEMORY_ENTRY *log_entry)
|
> @retval FALSE Success
|
> */
|
>
|
> -static bool ddl_log_execute_entry_no_lock(THD *thd, uint first_entry)
|
> +static bool ddl_log_execute_entry_no_lock(THD *thd, uint first_entry,
|
> + bool report_error)
|
|
This function has @param, please add 'report_error' there!
|
|
> @@ -2415,24 +2428,29 @@ static bool ddl_log_execute_entry_no_lock(THD *thd, uint first_entry)
|
> DBUG_ASSERT(ddl_log_entry.entry_type == DDL_LOG_ENTRY_CODE ||
|
> ddl_log_entry.entry_type == DDL_LOG_IGNORE_ENTRY_CODE);
|
>
|
> - if (ddl_log_execute_action(thd, &mem_root, &ddl_log_entry))
|
> + if (ddl_log_execute_action(thd, &mem_root, &ddl_log_entry, report_error))
|
> {
|
> uint action_type= ddl_log_entry.action_type;
|
> if (action_type >= DDL_LOG_LAST_ACTION)
|
> action_type= 0;
|
>
|
> - /* Write to error log and continue with next log entry */
|
> - sql_print_error("DDL_LOG: Got error %d when trying to execute action "
|
> - "for entry %u of type '%s'",
|
> - (int) my_errno, read_entry,
|
> - ddl_log_action_name[action_type]);
|
> + if (!report_error)
|
> + {
|
> + /* Write to error log and continue with next log entry */
|
> + sql_print_error("DDL_LOG: Got error %d when trying to execute action "
|
> + "for entry %u of type '%s'",
|
> + (int) my_errno, read_entry,
|
> + ddl_log_action_name[action_type]);
|
> + }
|
> + result= true;
|
> break;
|
> }
|
|
The above code is confusing. A reader will see it as
|
'If we should not report error, then we report an error'
|
|
Maybe better to change the name to 'interactive_mode' or 'in_recovery'
|
|
I checked the code and we only set report_error to true in the case of:
|
/* NOTE: holds "drop old table; rename tmp table" */
|
result= ddl_log_revert(thd, ddl_log_state_rm, true);
|
|
I wonder, shouldn't we have ddl_log_revert() always set report_error to true
|
as for this call we don't expect any errors?
|
|
> @@ -2930,7 +2948,7 @@ void ddl_log_complete(DDL_LOG_STATE *state)
|
> This is called for failed rename table, create trigger or drop trigger.
|
> */
|
>
|
> -bool ddl_log_revert(THD *thd, DDL_LOG_STATE *state)
|
> +bool ddl_log_revert(THD *thd, DDL_LOG_STATE *state, bool report_error)
|
> {
|
|
See comment above. What would happen if we would remove 'report_error' parameter
|
from this and do 'report_error= true' in this function?
|
|
As there should never be any errors when doing a ddl_log_revert() this should
|
be ok.
|
|
We still have to take care of following cases in ddl_log_revert():
|
|
- The original query failed and we to keep the errors state on exit.
|
(thd->is_error() was set)
|
- The original query succeded but revert failed. In this case we have to
|
convert the error to a warning as the original DDL succeded and we may
|
have already sent an 'ok' to the end user.
|
This can be done with some code like:
|
|
save=thd->is_error();
|
... execute_code
|
if (thd->is_error() && !save)
|
convert_error_to_warning(thd));
|
|
>
|
> /*
|
> Setting ddl_log_entry.phase to this has the same effect as setting
|
> @@ -248,6 +248,7 @@ typedef struct st_ddl_log_state
|
> */
|
> DDL_LOG_MEMORY_ENTRY *main_entry;
|
> uint16 flags; /* Cache for flags */
|
> + uint master_chain_pos;
|
> bool is_active() { return list != 0; }
|
> } DDL_LOG_STATE;
|
>
|
> @@ -273,7 +274,8 @@ bool ddl_log_write_execute_entry(uint first_entry,
|
> bool ddl_log_disable_execute_entry(DDL_LOG_MEMORY_ENTRY **active_entry);
|
>
|
> void ddl_log_complete(DDL_LOG_STATE *ddl_log_state);
|
> -bool ddl_log_revert(THD *thd, DDL_LOG_STATE *ddl_log_state);
|
> +bool ddl_log_revert(THD *thd, DDL_LOG_STATE *ddl_log_state,
|
> + bool report_error= false);
|
|
|
Please don't use optional arguments! It can easily be confusing when
|
looking at different pieces of code and not knowing about the extra argument
|
(could be another overloaded function). Better to change all callers!
|
|
todo.txt