Hi Jorgen,
When looking at your patch, I come up with a solution which would not require
removing objects from the catalogue. The trick is, in case of a view, to first
process its dependencies and only when this was successful, insert it into the
catalogue. The change is as follows:
> --- sql/backup/backup_info.cc 2008-05-28 15:25:24 +0000
> +++ sql/backup/backup_info.cc 2008-06-30 14:31:46 +0000
> @@ -995,16 +995,6 @@ Backup_info::add_db_object(Db &db, const
>
> }
>
> - Dbobj *o= Image_info::add_db_object(db, type, *name, pos);
> -
> - if (!o)
> - {
> - m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> - return NULL;
> - }
> -
> - o->m_obj_ptr= obj;
> -
> /*
> Add new object to the dependency list. If it is a view, add its
> dependencies first.
> @@ -1023,15 +1013,6 @@ Backup_info::add_db_object(Db &db, const
> return NULL;
> }
>
> - /*
> - Store a pointer to the catalogue item in the dep. list node. If this node
> - was a placeholder inserted into the list before, now it will be filled with
> - the object we are adding to the catalogue.
> - */
> -
> - DBUG_ASSERT(n);
> - n->obj= o;
> -
> /*
> If a new node was created, it must be added to the dependency list with
> add_to_dep_list(). However, if the object is a view, we must first add
> @@ -1050,6 +1031,25 @@ Backup_info::add_db_object(Db &db, const
> add_to_dep_list(type, n);
> }
>
> + Dbobj *o= Image_info::add_db_object(db, type, *name, pos);
> +
> + if (!o)
> + {
> + m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> + return NULL;
> + }
> +
> + o->m_obj_ptr= obj;
> +
> + /*
> + Store a pointer to the catalogue item in the dep. list node. If this node
> + was a placeholder inserted into the list before, now it will be filled with
> + the object we are adding to the catalogue.
> + */
> +
> + DBUG_ASSERT(n);
> + n->obj= o;
> +
> DBUG_PRINT("backup",("Added object %s of type %d from database %s (pos=%lu)",
> name->ptr(), type, db.name().ptr(), pos));
> return o;
If you don't object, let's go for this solution because it doesn't require
implementing remove methods which have its own problems.
One more request for this patch is to include a test script for it.
Best,
Rafal
Jorgen Loland wrote:
> #At file:///localhome/jl208045/mysql/mysql-6.0-backup-34902/
>
> 2630 Jorgen Loland 2008-06-11
> Bug#34902: "Backup: crash if view depends on dropped object"
>
> Pre-fix behavior: When adding a view to the backup image fails,
> the object representing the view is deleted but not removed
> from the database catalog. This view object is later tried deleted
> as part of catalog delete.
>
> Fix: If adding an object to the catalog fails, the object is
> removed from the catalog before deleting the object.
> modified:
> sql/backup/backup_aux.h
> sql/backup/backup_info.cc
> sql/backup/image_info.cc
> sql/backup/image_info.h
>
> per-file comments:
> sql/backup/backup_aux.h
> Added method Map<A,B>::remove
> sql/backup/backup_info.cc
> Added code to remove object from catalog if something fails when adding the
> object to the backup image.
> sql/backup/image_info.cc
> Added method Image_info::del_db_object, used to delete objects from a database
> catalog
> sql/backup/image_info.h
> Added definition for del_db_object and del_obj
> Added method Image_info::del_obj, used to delete an object from a given position
> in a database catalog
> === modified file 'sql/backup/backup_aux.h'
> --- a/sql/backup/backup_aux.h 2008-03-05 17:48:12 +0000
> +++ b/sql/backup/backup_aux.h 2008-06-11 11:55:29 +0000
> @@ -180,6 +180,7 @@ class Map
> ~Map();
>
> int insert(const A&, B*);
> + int remove(const A&);
> B* operator[](const A&) const;
>
> private:
> @@ -239,6 +240,23 @@ int Map<A,B>::insert(const A &a, B *b)
> return my_hash_insert(&m_hash, (uchar*) n);
> }
>
> +/**
> + Remove a mapping.
> +
> + RETURN
> + 0 if mapping was found and removed
> + 1 if mapping was not found
> + */
> +template<class A, class B>
> +inline
> +int Map<A,B>::remove(const A &a)
> +{
> + Node *n= (Node*) hash_search(&m_hash, (uchar*) &a, sizeof(A));
> + return n ? hash_delete(&m_hash, (uchar*) n) : 1;
> +}
> +
> +
> +
> /// Get pointer corresponding to the given value.
> template<class A, class B>
> inline
>
> === modified file 'sql/backup/backup_info.cc'
> --- a/sql/backup/backup_info.cc 2008-05-28 15:25:24 +0000
> +++ b/sql/backup/backup_info.cc 2008-06-11 11:55:29 +0000
> @@ -1019,6 +1019,8 @@ Backup_info::add_db_object(Db &db, const
>
> if (res == get_dep_node_res::ERROR)
> {
> + // if something fails, obj must be deleted from the catalog.
> + Image_info::del_db_object(db, pos);
> m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> return NULL;
> }
> @@ -1043,6 +1045,8 @@ Backup_info::add_db_object(Db &db, const
> if (type == BSTREAM_IT_VIEW)
> if (add_view_deps(*obj))
> {
> + // if something fails, obj must be deleted from the catalog.
> + Image_info::del_db_object(db, pos);
> m_ctx.fatal_error(error, db.name().ptr(), name->ptr());
> return NULL;
> }
>
> === modified file 'sql/backup/image_info.cc'
> --- a/sql/backup/image_info.cc 2008-04-21 10:45:39 +0000
> +++ b/sql/backup/image_info.cc 2008-06-11 11:55:29 +0000
> @@ -246,6 +246,26 @@ Image_info::Dbobj* Image_info::add_db_ob
> return o;
> }
>
> +/**
> + Delete per database object from the catalogue.
> +
> + @param[in] db database to which this object belongs - this database must
> + already be in the catalogue
> + @param[in] pos position where the object to delete is stored inside
> + database's object list
> +
> + @returns
> + TRUE if object was deleted from the catalog
> + FALSE if the object was not found in the catalog
> + */
> +bool Image_info::del_db_object(Db &db, ulong pos){
> +
> + if (db.del_obj(pos))
> + return FALSE;
> +
> + return TRUE;
> +}
> +
> /**
> Return per database object stored in catalogue.
>
>
> === modified file 'sql/backup/image_info.h'
> --- a/sql/backup/image_info.h 2008-05-17 16:08:00 +0000
> +++ b/sql/backup/image_info.h 2008-06-11 11:55:29 +0000
> @@ -118,6 +118,7 @@ public: // public interface
> Ts* add_ts(const String &db_name, uint pos);
> Dbobj* add_db_object(Db &db, const obj_type type,
> const ::String &name, ulong pos);
> + bool del_db_object(Db &db, ulong pos);
> Table* add_table(Db &db, const ::String &table_name,
> Snapshot_info &snap, ulong pos);
>
> @@ -408,6 +409,7 @@ class Image_info::Db
> ulong obj_count() const;
> obs::Obj* materialize(uint ver, const ::String &sdata);
> result_t add_obj(Dbobj&, ulong pos);
> + result_t del_obj(ulong pos);
> Dbobj* get_obj(ulong pos) const;
> result_t add_table(Table&);
> const char* describe(describe_buf&) const;
> @@ -1030,6 +1032,20 @@ result_t Image_info::Db::add_obj(Dbobj &
> return OK;
> }
>
> +/**
> + Delete object on a given prosition from a database.
> + */
> +inline
> +result_t Image_info::Db::del_obj(ulong pos)
> +{
> + if (m_objs.remove(pos))
> + return ERROR;
> +
> + m_obj_count--;
> +
> + return OK;
> +}
> +
> /// Get database object stored at given position.
> inline
> Image_info::Dbobj* Image_info::Db::get_obj(ulong pos) const
>
>
