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?).
* create a new test for checking dependencies
> --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?
> 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?
> +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.
>
> /**
> @@ -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?
...
> +/*
> + 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
...
> + 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.
> + 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.
