Hi Chuck,
Thanks for your review! See my replies below.
Rafal
Chuck Bell wrote:
> Rafal,
>
> I have some questions and suggestions:
>
>> diff -Nrup a/mysql-test/t/backup_objects.test
>> b/mysql-test/t/backup_objects.test
>> --- a/mysql-test/t/backup_objects.test 2008-02-13
>> 12:40:24 +01:00
>> +++ b/mysql-test/t/backup_objects.test 2008-05-07
>> 15:07:25 +02:00
>> @@ -1,12 +1,36 @@
>> #
>> -# This test testing backup and restore for database objects.
>> +# This test is testing backup and restore of database objects.
>> #
>> +# The following objects are tested:
>> +# - tables
>> +# - views
>> +# - stored procedures and functions
>> +# - triggers
>> +# - events
>> +#
>> +# It is also tested that backup and restore works correctly
>> when there are
>> +# dependencies between objects such as:
>> +# - one table depends on another table via foreign key constraint,
>> +# - a view depends on a table or another view,
>> +# - stored routine or event refers to another object.
>>
>> --source include/have_innodb.inc
>> --source include/not_embedded.inc
>>
>> -connect (con1,localhost,root,,);
>> -connection con1;
>> +# Note: BACKUP crashes server when InnoDB engine is used and
>> views are
>> +# included in the image (see BUG#34758). Until this issue is
>> resolved we
>> +# can't use InnoDB engine here and thus define table
>> dependencies using
>> +# foreign key constraints.
>> +#
>> +# If tables use MyISAM storage engine the test works but all foreign
>> +# key constraints are silently ignored. When BUG#34758 is
>> fixed, the line
>> +# below should be changed to make InnoDB the default storage
>> engine so that
>> +# handling of dependencies introduced by foreign keys can
>> also be tested.
>> +#
>> +# Note that there is a separate test backup_fkey testing foreign key
>> +# constraints.
>> +
>> +SET storage_engine=MyISAM;
>
> I understand the need to remove the InnoDB from the test, but I am surprised
> at the radical changes to the test. I would request that the test not be
> stripped so drastically. Case in point, I would ask that the parts that are
> removed are put back in put disabled (like commented out) and marked with
> the bug report. I think it is a big risk to take functionality out of a test
> with expectations that it would be covered by another test or added later.
> Better to leave it in and disable it as a constant reminder.
>
> If you insist the radical changes are necessary, perhaps it would help to
> explain some more as to why it was changed. But let's not remove anything
> from the test even if we have to disable it until a bug report is fixed.
>
> Now that I've thought more about it, my suggestion is as follows:
>
> * return the backup objects test to its original form
> * comment out the InnoDB and change to MyISAM with notes to reenable it when
> the bug is fixed (noted in the bug report perhaps?).
This is already done - see above the long comment explaining why MyISAM is used
now and that it should be replaced by InnoDB when bugs are fixed.
> * create a new test for checking dependencies
>
It would be easier to create a separate test but I decided otherwise based on
the following reasoning.
The goal of backup_objects test is to check that our system correctly backups
and restores all supported kinds of objects. Checking that dependencies between
objects are correctly taken into account is logically part of this task. A
separate test for testing dependencies would be in fact very similar to
backup_objects test. Thus, instead of multiplying the number of tests it makes
sense to add object dependency checking to backup_objects test.
Doing this that way required changing the test but I disagree that changes are
radical. It is still the same test which creates coulple of objects of different
kinds, backups them and then restores. What is changed is the following:
- Two databases 'db1' and 'db2' used instead of single 'bup_objects'. This is
needed to create inter-database dependencies.
- Functions and procedures are created first in case view or table definitions
refer to them (currently only views).
- View definitions are more complex, so that we have views which depend on other
views.
- Different data is loaded into tables to ensure that views are not empty.
- The "Exercise the objects" section has been removed because first, it has no
influence on BACKUP/RESTORE operation and second, it is hard to define objects
with complex dependencies and usable at the same time.
- To shorten the test, SHOW CREATE statements are executed only after restore.
In case of wrong restore these will be compared to the ones stored in the result
file.
- Instead of selecting from I_S tables, sql statements such as SHOW EVENTS and
SHOW PROCEDURE STATUS are used to see if events or procedures were restored.
I don't think these changes are drastic - I claim that after the updates the
test is a superset of the previous test. Perhaps your impression was caused by
the fact that object definitions are moved around (stored routines are defined
at the beginning) which looks in the patch like a lot of lines have been
deleted. But note that equivalent lines are inserted later!
>> --remove_file $MYSQLTEST_VARDIR/master-data/bup_objects.bak
>
> Remove file? I don't see this file in my repository. Is this needed or is
> this a mistake?
>
This line comes from the original test. One line above there is
--error 0,1
which means that --remove_file is here just in case the bup_objects.bak is left
over from some other test (safety).
>> diff -Nrup a/sql/backup/backup_info.cc b/sql/backup/backup_info.cc
>> --- a/sql/backup/backup_info.cc 2008-04-24 11:18:47 +02:00
>> +++ b/sql/backup/backup_info.cc 2008-05-07 15:07:25 +02:00
>> @@ -223,6 +223,68 @@ uchar* Backup_info::Ts_hash_node::get_ke
>> return (uchar*)(n->name->ptr());
>> }
>>
>> +/**
>> + Represents a node in the dependency list.
>> +
>> + Such node can be an empty placeholder or store a pointer
>> to a catalogue item
>> + in @c obj member. Dependency list nodes are kept in a hash
>> and thus
>> + @c Dep_node contains all required infrastructure: the @c
>> key member to store
>> + a key string plus @c get_key() and @c free() functions
>> used in @c HASH
>> + operations.
>> + */
>> +struct Backup_info::Dep_node: public Sql_alloc
>> +{
>> + Dep_node *next;
>> + Dbobj *obj;
>> + String key;
>> +
>> + Dep_node(const ::String &db_name, const ::String &name);
>> + Dep_node(const Dep_node&);
>> +
>> + static uchar* get_key(const uchar *record, size_t
>> *key_length, my_bool);
>> + static void free(void *record);
>> +};
>> +
>> +/**
>> + Create an empty dependency list node for a given
>> per-database object.
>> +
>> + The object is identified by its name and the name of the
>> database to which
>> + it belongs.
>> + */
>
> Questions: What about global objects? Let's suppose we discover some global
> or at least non-database level dependencies. Having a db_name as part of the
> key is not going to work. Would you add a new subclass to handle these or
> should this class extension be designed to rely on a non-semantic key?
>
As you know we don't backup global objects right now, with exception of table
spaces and databases. The design described in WL contains a full list of objects
that we handle and explains that for most of them dependencies are handled by
restoring them in a correct order. E.g., first global objects are restored and
only then per-database objects.
Because of that, the task reduces to handling per-database object dependencies
only. Here also objects are restored in correct order and the only non-trivial
issue to resolve is handling of view-view dependencies.
The order in which global object metadata is stored in the image, and thus the
order in which these objects are restored, is determined by
Backup_info::Global_iterator. This iterator first lists tablespaces and then
databases, as specified in the worklog design.
>> +inline
>> +Backup_info::Dep_node::Dep_node(const ::String &db_name,
>> const ::String &name)
>> + :next(NULL), obj(NULL)
>> +{
>> + key.length(0);
>> + key.append(db_name);
>> + key.append(".");
>> + key.append(name);
>> +}
>> +
>> +inline
>> +Backup_info::Dep_node::Dep_node(const Dep_node &n)
>> + :Sql_alloc(n), next(n.next), obj(n.obj)
>> +{
>> + key.copy(n.key);
>> +}
>> +
>> +inline
>> +void Backup_info::Dep_node::free(void *record)
>> +{
>> + ((Dep_node*)record)->~Dep_node();
>> +}
>> +
>> +inline
>> +uchar*
>> +Backup_info::Dep_node::get_key(const uchar *record,
>> + size_t *key_length,
>> + my_bool) // not_used
>> __attribute__((unused)))
>> +{
>> + Dep_node *n= (Dep_node*)record;
>> + if (key_length)
>> + *key_length= n->key.length();
>> + return (uchar*)n->key.ptr();
>> +}
>>
>
> What about case sensitivity issues? Will the code handle the case where on
> Windows 'testdb1' and 'TESTDB1' are the same? Likewise, what about Linux
> where they are different? This code needs to handle case sensitivity issues.
>
Good point, not easy to spot from non-Win perspective! I think this issue should
be taken into account in the object services code. Recall that if, for example,
we want to look at server's databases, then we first create an iterator with
obs::get_databases(). Then we get obs::Obj instances from this iterator and can
read database name using obs::Obj::get_name(). It is up to object services
implementation to decide what exactly get_name() returns, but it should be a
"canonical" name of the object by which it can be identified inside the server.
On systems where several forms of the same name can exists, the implementation
should pick a canonical form (for example all uppercase).
Going back to the code, Dep_node structures are used in dep_hash to store
entries for objects returned by object services API functions. When object obj
is stored there, obj->get_db_name() and obj->get_name() are used to initialize
the hash entry and thus form the key.
Thus everything should be fine under assumption that if obj1 and obj2 represent
the same object then obj1->get_db_name() and obj1->get_name() are the same as
obj2->get_db_name() and obj2->get_name(). I think it is a valid assumption to
make. If this assumption is violated, as when obj1->get_name() returns "testdb"
and obj2->get_name() returns "TESTDB", then it should be reported as a bug in
si_objects, but I don't think it is the case.
>>
>> /**
>> @@ -233,7 +295,9 @@ uchar* Backup_info::Ts_hash_node::get_ke
>> @c find_backup_engine().
>> */
>> Backup_info::Backup_info(Backup_restore_ctx &ctx)
>> - :m_ctx(ctx), m_state(Backup_info::ERROR), native_snapshots(8)
>> + :m_ctx(ctx), m_state(Backup_info::ERROR), native_snapshots(8),
>> + m_dep_list(NULL), m_dep_end(NULL),
>> + m_sr_end(NULL), m_v_end(NULL), m_tr_end(NULL)
>> {
>> using namespace backup;
>>
>> @@ -243,6 +307,8 @@ Backup_info::Backup_info(Backup_restore_
>>
>> hash_init(&ts_hash, &::my_charset_bin, 16, 0, 0,
>> Ts_hash_node::get_key, Ts_hash_node::free, MYF(0));
>> + hash_init(&dep_hash, &::my_charset_bin, 16, 0, 0,
>> + Dep_node::get_key, Dep_node::free, MYF(0));
>>
>> /*
>> Create default and CS snapshot objects and add them to
>> the snapshots list.
>> @@ -263,7 +329,7 @@ Backup_info::Backup_info(Backup_restore_
>> return;
>>
>> snapshots.push_back(snap);
>> -
>> +
>> m_state= CREATED;
>> }
>>
>> @@ -282,6 +348,7 @@ Backup_info::~Backup_info()
>> delete snap;
>>
>> hash_free(&ts_hash);
>> + hash_free(&dep_hash);
>> }
>>
>> /**
>> @@ -770,6 +837,112 @@ backup::Image_info::Table* Backup_info::
>> }
>>
>> /**
>> + For all views on which the given one depends directly or
>> indirectly, add
>> + placeholders to the dependency list ensuring correct order
>> among them.
>> +
>> + The main view being processed in this function should be
>> added to the
>> + dependency list after all the placeholders created here.
>> This way if one of
>> + the views on which the given one depends is added to the
>> list later, it will
>> + be placed at the correct position indicated by the placeholder.
>> +
>> + @param[in] obj server object for the view to be processed
>> +
>> + A recursive algorithm is used to insert placeholders in
>> correct order.
>> + That is, for each base view @c bv of the given one, @c
>> add_view_deps(bv)
>> + is called to insert all dependencies of @c bv before @c bv
>> itself is appended
>> + to the list.
>> +
>> + Function @c get_dep_node() used to create a node to be
>> inserted into the
>> + dependency list detects if the node was already created
>> earlier. This ensures
>> + correct behaviour of the algorithm even if the same view
>> is visited several
>> + times during the depth-first walk of the dependency graph
>> performed by the
>> + recursive algorithm. If it is detected that a node for a
>> given view already
>> + exists then this view is not processed for the second time.
>> +
>> + This also ensures termination of the algorithm even when there are
>> + circular dependencies. Suppose that view @c v has itself
>> as an (indirect)
>> + dependency. When processing @c v, a node will be created
>> for it first
>> + and then its dependencies will be processed. When
>> add_view_deps() comes across
>> + @c v for the second time it will see that a corresponding
>> mode already
>> + exists and thus will break the recursion.
>> +
>> + @return Non-zero value if an error happened.
>> + */
>
> The above is very good documentation. I am impressed! :) Can we put some of
> this in the worklog so that it gets into the docs internal or otherwise?
>
Thanks! :) I have added more details to de description of the algorithm in the
worklog. Now it reads:
Before inserting a view into the catalogue, slots will be created for all the
views on which it depends using the following recursive procedure.
add_view_deps(v)=
for every base view v' of v do
add_view_deps(v')
append slot for v to the dependency list.
This ensures a correct order between slots for all the views on which a given
one depends.
Actually the algorithm is a bit more complicated to ensure correct termination
even when there are cyclic dependencies. To that end, set A of processed views
is maintained. If a view is in this set then it is not processed again and
recursion terminates:
add_view_deps(v)=
if A contains v then return else add v to A.
for every base view v' of v do
add_view_deps(v')
append slot for v to the dependency list.
Thus even if view v has itself as an (indirect) dependency the algorithm will
terminate. When processing v for the first time it will be added to A and then,
when add_view_deps() comes across v for the second time it will return
immediately.
>
> ...
>
>> +/*
>> + Find or create a dependency list node for an object with a
>> given name.
>> +
>> + @param[in] db_name name of the database to which this
>> object belongs
>> + @param[in] name name of the object
>> + @param[out] node pointer to the created or located node
>> +
>> + All nodes created using that function are keept inside @c
>> dep_node HASH
>
> What function? What does 'that' refer to?
> /s/keept/kept
>
>
The function being documented here. I think I should use 'this' instead.
> ...
>
>> + Set end to point at m_sr_end, m_et_end or m_v_end
>> depending on the
>
> What does _et_ _sr_ and _v_ mean? Can you please spend a few characters and
> use something more descriptive like m_view_end? It would make the code
> easier to maintain and 'cost' nothing.
>
Ok, changed to m_srout_end, m_view_end and m_trigger_end.
>> + type of the item.
>> +
>> + If the corresponding section is empty, the *end pointer
>> is set up to point
>> + at the node after which this section should start, or to
>> NULL if section
>> + should be at the beginning of the dependency list.
>> +
>> + After inserting the new node it becomes the last node in
>> the section so
>> + the pointer is updated to point at it.
>> + */
>> +
>
> ...
>
>> + enum { TABLESPACES, DATABASES, DONE } mode;
>
> What about users and configuration items? While they are unlikely to have
> dependencies, what if we discover a global item/object that does have
> dependencies? Would it be easy to add them later? If not, please place some
> comments to indicate how we could add additional global objects.
>
1. We don't backup any global objects except databases and tablespaces.
2. Possible dependencies have been analysed during the design phase and are
taken into account in the design which is approved and implemented here. Do you
think that design is not correct?
Rafal
