Ingo,
Thanks for finding time to look at my patch and for your valuable comments. I'll
prepare an updated patch for you to have a final look. In the meantime see my
replies below.
Ingo Strüwing wrote:
> Hi Rafal,
>
> rsomla@stripped, 31.03.2008 13:28:
> ...
>> ChangeSet@stripped, 2008-03-31 13:28:32+02:00, rafal@quant.(none) +6 -0
>> BUG#34721 (Backup driver can't refuse to provide driver):
>>
>> This patch modifies backup kernel so that it falls back to built in backup
>> engines in case a native backup engine can not be created.
>>
>> This is for the special case when storage engine defines a get_backup_engine()
>
>> factory function (inside the handlerton), but that function fails to create a
>> backup engine instance. Previous code haven't dealt correctly with that case.
>> Now it puts a warning and tries the built-in engines.
>
> Formally a changeset comment should say 1. what was the problem, and 2.
> how was it fixed. The latter is well explained. So I'd appreciate if you
> prepend a sentence saying what the problem was. The bug synopsis is too
> short IMHO.
>
OK, comment changed to:
BUG#34721 (Backup driver can't refuse to provide driver):
This patch modifies backup kernel so that it falls back to built in backup
engines in case a native backup engine can not be created.
If storage engine defines a get_backup_engine() factory function (inside the
handlerton), but that function fails to create a backup engine then the current
code interrupts backup/restore operation and reports error. This is not the
expected behaviour. This patch fixes the backup engine selection logic so that
in the above case it puts warning and continues using the built-in engines.
> ...
>> sql/backup/backup_info.cc@stripped, 2008-03-31 13:28:29+02:00, rafal@quant.(none)
> +44 -9
>> Debug code injected into has_native_backup() function which sets MyISAM
> backup
>> factory function to a dummy one. This is used to test the backup engine
> selection
>> logic.
>>
>> In find_backup_engine: in case a native backup engine has been selected for a
> given
>> table, check if created Native_snapshot object is valid (in particular, has
>> successfully created the engine). Only then use it for that table, otherwise
> try other
>> backup engines including the built-in ones.
>
> Great comment. However I would prefer to have the explanation, how it
> works, in a code comment. IMHO changeset file comments do just need to
> say _what_ was changed, which helps when resolving merge conflicts.
>
I inserted this comment in the code as well. However, I'm not sure how a perfect
cset comment should look in this case so I'm leaving it as it is (assuming that
giving too much information never hurts).
>> sql/backup/be_native.h@stripped, 2008-03-31 13:28:29+02:00, rafal@quant.(none) +1
> -1
>> If the get_backup_engine() factory function defined by a storage engine
> failes
>
> Typo: failes -> fails
>
>> to create a native backup engine don't treat it as a (fatal) error. In that
> case
>> we will try to use default, built-in backup engines. Thus only warning is
> printed.
>
> Again, here I would like to see something like "Changed
> ER_BACKUP_CREATE_BE to report as a warning". And the above comment moved
> into the code.
>
I moved comment into the code and changed cset comment to:
In Native_snapshot::init() report ER_BACKUP_CREATE_BE as warning, not as error.
> ...
>> +++ b/mysql-test/t/backup_no_be-master.opt 2008-03-31 13:28:29 +02:00
>> @@ -0,0 +1 @@
>> +--debug=d,backup_test_dummy_be_factory
>
> To try to get rid of a server restart and thus this option file, can't
> we remember the old value of hton->get_backup_engine and restore it
> after doing the related checks?
>
I was thinking about that and concluded that it is not easy to implement. But
after your suggestions I will give it another try. You should see the outcome in
the next patch I'll send.
> ...
>> +++ b/sql/backup/backup_info.cc 2008-03-31 13:28:29 +02:00
> ...
>> /// Determine if a given storage engine has native backup support.
>> static
>> bool has_native_backup(storage_engine_ref se)
>
> Please fix the format of the function comment.
>
Hmm, what's wrong with the above one - it is a valid doxygen comment. Having a
full-fledged documentation for this function would be an overkill. The one line
description I gave should be enough to understand the purpose of the function.
>> {
>> handlerton *hton= se_hton(se);
>>
>> + /*
>> + The code below is used to test the backup engine selection logic.
> ...
>> + dummy backup engine factory which never creates any engines.
>> + */
>
> Please remove the trailing space from the */ line and indent it
> correctly (one space less).
>
Sorry Ingo, but I am alergic to such review comments. Can you please point me at
a coding guideline which *requires* me to remove the spece before '*/'? The only
relevant rule I can see is:
"When writing multi-line comments please put the '/*' and '*/' on their own
lines, put the '*/' below the '/*', put a line break and a two-space indent
after the '/*', do not use additional asterisks on the left of the comment"
And I clearly put '*/' on its own line and below '/*' as required, and satisfied
all the other requirements.
I'll be grateful if in the future we skip wasting our time on fixing code
spacing issues...
>> + DBUG_EXECUTE_IF("backup_test_dummy_be_factory",
>> + if (hton == myisam_hton) hton->get_backup_engine=
> dummy_backup_engine_factory;);
>
> If you find (or create) some temporary storage in 'se', you could save
> the current value of hton->get_backup_engine so that you can restore it
> later. Then we could avoid a server restart.
>
>> +
>> return hton && hton->get_backup_engine;
>> }
>>
>> @@ -83,16 +117,17 @@ Backup_info::find_backup_engine(const ba
>> {
>> Native_snapshot *nsnap= new Native_snapshot(m_ctx, se);
>> DBUG_ASSERT(nsnap);
>
> Not part of your patch, but as it pops up in the changeset: This is ok
> in a prototype. But before push to main the assert must be changed to
> error handling.
>
Good point. I removed the assertion and added checks for nsnap != NULL. If nsnap
== NULL (we couldn't create Native_snapshot object) the code will proceed to
trying the built-in backup engines. If built-in engines don't work then an error
will be reported.
> ...
>> + if (nsnap->is_valid())
>> + {
>> + snapshots.push_front(nsnap);
>> + native_snapshots.insert(se, nsnap);
>> +
>> + if (nsnap->accept(tbl, se))
>> + snap= nsnap;
>> + }
>
> Shouldn't you delete nsnap if accept() returns FALSE? You do it only if
> is_valid() returns FALSE.
>
No, the valid native snapshot object is kept to be potentially used for other
tables. It is deleted when Backup_info object is destroyed.
>> + else
>> + delete nsnap;
>> }
>
> Here you could restore the myisam get_backup_engine pointer if you saved
> it above.
>
Right, it might work. I'll just move the error inserted code to the same function.
> ...
>
> Otherwise approved if you agree with my above change requests and
> considered my suggestions.
>
I'll send you a new patch so that you can verify your approval. I'll also
uncheck the review box on the bug page so we can better trace the status.
Rafal
