Rafal,
I am disappointed that you regard the kernel with such high emotional
protectiveness. I understand most of your points you outlined but I
think to work effectively we must devoid ourselves from certain
prejudices regarding some types of changes to the code.
I am ok with you suggesting changes to how parameters are passed and
such -- that's not what I am complaining about. What bothers me most
is that it seems any time I make a change to any part of the kernel
it's never good enough to pass your reviews and it takes a minimum of
four reviews to satisfy you. We all must simply learn to relax a
little! :)
Since you feel so strongly about changing the parameter names, types,
etc., please feel free to make those changes -- just have Svoj or
someone review them first.
As for the warn/error on backup I thought we agreed that backup should
error if backupdir is invalid at backup time. Ask Peter for his
opinion on this and go with his recommendation.
If you are not comfortable making the changes you outlined below WRT
my explanations (see below), please forget about putting this code in
this merge to main and we can have a Skype session on Tuesday to
resolve any remaining issues.
Chuck
Quoting Rafal Somla <rsomla@stripped>:
> Chuck,
>
> I still don't like the changes in the backup kernel. The serious
> concern is that you report an error always, if backupdir is invalid,
> but I think error should be reported only if backupdir used. If user
> says something like
>
> BACKUP DATABASE foo TO '/an/absolute/path';
>
> I think it should succeed even if backupdir is currently invalid,
> because the command does not refer to backupdir.
That is not how I remember it but we can run it by Peter or Robin and
see what they recommend. Go with their consensus.
> The easiest solution IMO would be to just warn about invalid backupdir.
> If it is really used, user will get error when the stream is opened.
> I'm aware that it is not perfect because error message could be
> confusing, but I think we can deal with that separately, as I think
> this particular place needs to be refactored anyway (i.e., reporting
> stream related errors).
>
> Apart from that there are few changes which I don't think are needed
> and thus are a bit annoying for an author of the code. These are things
> like changing types of function parameters, renaming the parameters and
> such. I'd like to see the reasons behind introducing such changes...
IMHO (and this is not an accusation, merely commentary): we as
developers in a team should never regard any portion of the code
higher than any other or have moving standards. Ownwership and
authorship have no place in a modern agile development (like)
environment. Arguing over parameter names and "why did you add a
parameter to my function" are controversial and adversarial at best.
Let us refrain from this mentality.
> See below for more details.
>> char*); + Backup_info* prepare_for_backup(String *location, +
>> LEX_STRING orig_loc, +
>> const char*, bool);
>> + Restore_info* prepare_for_restore(String *location, +
>> LEX_STRING orig_loc,
>> + const char*);
>
> Why parameter which holds value of backupdir is called location, not
> backupdir? Then location could be preserved as a parameter holding,
> well... the location.
Yeah, ok. I was trying to minimize changes (as per direction) but it
can be changed -- I will not debate names of parameters -- within
reason. ;)
>> + /*
>> + Check backupdir for validity (needed since we cannot trust
>> + that the path has become invalid since set).
>> + */
>> + if (backupdir->length() && my_access(backupdir->c_ptr(),
> (F_OK|W_OK)))
>> + {
>> + context.fatal_error(ER_BACKUP_BACKUPDIR, backupdir->c_ptr());
>> + DBUG_RETURN(send_error(context, ER_BACKUP_BACKUPDIR,
>> backupdir->c_ptr()));
>> + }
>> +
>
> I think it is too strict to always report error here. I would report
> warning instead and the error will happen when openning stream, but not
> if user specifies absolute path.
Please check with Peter or Robin. Go with their suggestion/recommendation.
>> +Backup_restore_ctx::prepare_for_backup(String *backupdir, +
>> LEX_STRING orig_loc, +
>> const char *query,
>> bool with_compression)
>
> Why renaming location -> orig_loc?
Because it now holds the lex->backup_dir which is the original value
specified by the user from the command line. If you can think of a
better name then use it.
>> + if (prepare(orig_loc))
>> return NULL;
>> - backup::String path(location);
>
> This variable was here for transforming the string from the
> representation used in the parser (LEX_STRING) to the representation
> used in the backup kernel code (String).
Which I now write directly to m_path so I felt it was no longer need
(and it isn't). If you feel strongly you need to reconvert it then put
it back in.
>> +Backup_restore_ctx::prepare_for_restore(String *backupdir,
>> + LEX_STRING orig_loc, +
>> const char *query)
>
> Why renaming location -> orig_loc?
See above for similar change.
>> /*
>> Open input stream.
>> */
>> - backup::String path(location);
>
> This variable was here for transforming the string from the
> representation used in parser (LEX_STRING) to the representation used
> in the backup kernel code (String).
See above for similar change.
>> -Stream::Stream(Logger &log, const ::String &name, int flags)
>> - :m_path(name), m_flags(flags), m_block_size(0), m_log(log)
>> +Stream::Stream(Logger &log, ::String *backupdir, +
>> LEX_STRING orig_loc, int flags)
>> + :m_flags(flags), m_block_size(0), m_log(log)
>
> Why "const ::String&" is replaced with "::String*" ?
> Why renaming name -> orig_loc?
> Why changing its type to LEX_STRING?
>
> I'd prefer to consistently use String for passing string values in the
> backup code. There was one exception - the parameter of
> execute_backup_command() was of type LEX_STRING. But this is an
> exception because the function is called from inside the parser which
> uses LEX_STRING internaly. Inside backup (kernel) code I wanted to use
> String class for strings.
Because it does not compile on Windows -- it complains bitterly when
transforming const to non-const and vice-versa. I also feel strongly
we should pass pointer to strings and not values. In this case, why
string by value? Is there something I am not understanding WRT pass by
value? Isn't passing the pointer faster/easier?
I see the "I" pronoun. To me, that means only Rafal can approve such
changes to the kernel. Please try to think as a team. There is seldom
one way or even an only way to code. We need to be flexible (perhaps
not in this case -- I haven't heard your reasoning) with how things
are done. If they are correct in the sense there is no one correct way
we shouldn't hold up reviews like we do.
>> + {
>> + path_len=backupdir->length() + orig_loc.length + 1;
>> + m_path.alloc(path_len);
>> + fn_format(m_path.c_ptr(), orig_loc.str, backupdir->c_ptr(), "",
>> + MY_UNPACK_FILENAME);
>> + }
>> + else
>> + {
>> + path_len= orig_loc.length + 1;
>> + m_path.alloc(path_len);
>
> Why do you allocate memory here?
Where does the memory come from? Nothing is allocated yet. If there is
a better way, please change it.
>
>> + m_path.set_ascii(orig_loc.str, orig_loc.length);
>> + }
>> + m_path.length(path_len);
>> +
>
> I think m_path.set_ascii(...) sets the length of the string, so this
> m_patch.length(...) statement is needed only inside if(...) branch,
> where fn_format(...) is used.
Ok, but when I omitted it the server crashed when m_path was accessed.
That's because I observered length == 0. Length must be set here.
>> + String backupdir;
>> +
>> + backupdir.alloc(sys_var_backupdir.value_length);
>> + backupdir.set_ascii(sys_var_backupdir.value, +
>> sys_var_backupdir.value_length);
>> + backupdir.length(sys_var_backupdir.value_length);
>> +
>
> Do you really need to copy the value of the variable? Why not just pass
> the pointer to the value?
Because the value can change from the time backup is started and the
time backup needs to access it. By capturing it here we can be sure
this session of backup/restore has a consistent value when accessed.
>
>> + /*
>> Note: execute_backup_command() sends a correct response to the client
>> (either ok, result set or error message).
>> - */ - if (execute_backup_command(thd,lex))
>> + */ + if (execute_backup_command(thd, lex, &backupdir))
>
> Since parser uses LEX_STRING for holdings strings, it would be more
> appropriate to pass backupdir as LEX_STRING. Even if we need to copy
> the value of backupdir, this can be done inside execute_backup_command.
If you insist...
Chuck
