STATUS
------
Changes requested.
REQUESTS
--------
[1] Provide missing comment.
[2] Clarify comment in switch for object counting.
[3] Please fix spacing on return call. Also contains a suggestion.
SUGGESTIONS
-----------
[4] Readability suggestion.
COMMENTARY/QUESTIONS
--------------------
[5] See below.
[6] I have a question about a data type change.
[7] I have a question about what is counted (answer required for approval).
DETAILS
-------
> 2764 Jorgen Loland 2009-02-11
> Bug#39109 - Mysql Online Backup table doesn't show correct num_object count
>
> Previously, the num_objects column in the backup_history table showed the
> number of tables in the backup image. It now shows the number of objects with names (i.e,
> tablespace, database, table, view, routine)
...
> === modified file 'mysql-test/suite/backup/t/backup_logs.test'
> --- a/mysql-test/suite/backup/t/backup_logs.test 2009-02-06 08:28:24 +0000
> +++ b/mysql-test/suite/backup/t/backup_logs.test 2009-02-11 09:45:55 +0000
> @@ -9,6 +9,7 @@
> --source include/not_embedded.inc
> --source include/have_debug.inc
> --source include/blackhole.inc
> +--source include/have_falcon.inc
[5 - commentary only] I really don't like having to add falcon to this
test but I understand why it is needed. Compiling falcon on Windows
poses some interesting problems. I may change this in BUG#42693 by
making a special (more thorough) test for counting objects.
> === modified file 'sql/backup/image_info.cc'
> --- a/sql/backup/image_info.cc 2008-12-18 21:46:36 +0000
> +++ b/sql/backup/image_info.cc 2009-02-11 09:45:55 +0000
> @@ -14,7 +14,13 @@
> namespace backup {
>
> Image_info::Image_info()
> - :data_size(0), m_table_count(0), m_dbs(16, 16), m_ts_map(16,16)
> + :data_size(0),
> + m_table_count(0),
> + m_view_count(0),
> + m_routine_count(0),
> + m_priv_count(0),
> + m_dbs(16, 16),
> + m_ts_map(16,16)
[4] Readability suggestion: please move the : to the previous line and
indent the m_* 2 spaces.
...
> === modified file 'sql/backup/image_info.h'
> --- a/sql/backup/image_info.h 2009-01-08 14:57:41 +0000
> +++ b/sql/backup/image_info.h 2009-02-11 09:45:55 +0000
> @@ -90,9 +90,13 @@ public: // public interface
> size_t data_size; ///< How much of table data is saved in the image.
> st_bstream_binlog_pos master_pos; ///< To store master position info.
>
> - ulong table_count() const;
> + uint object_count() const;
> uint db_count() const;
> uint ts_count() const;
> + uint table_count() const;
> + uint view_count() const;
> + uint routine_count() const;
> + uint priv_count() const;
> ushort snap_count() const;
>
> // Examine contents of the catalogue.
> @@ -149,10 +153,17 @@ protected:
>
> Image_info();
> uint m_table_count; ///< Number of tables in the image.
> + uint m_view_count; ///< Number of views in the image
> + uint m_routine_count; ///< Number of stored routines in the image
> + uint m_priv_count; ///< Number of privileges in the image
> +
> MEM_ROOT mem_root; ///< Memory root for storage of catalogue items.
>
> class Tables; ///< Implementation of Table_list interface.
>
> + ///
> + void count_object(const enum_bstream_item_type type);
> +
[1] Please provide the comment for this method. I see /// but nothing
else... ;)
> private:
>
> Map<uint, Db> m_dbs; ///< Pointers to Db instances.
> @@ -721,6 +732,64 @@ Image_info::Dbobj_iterator::Dbobj_iterat
>
> ********************************************************************/
>
> +/**
> + Increase counter for this type of object. This is displayed as info
> + in the backup_history table.
> +
> + @param[in] type type of the object
> +*/
> +inline
> +void Image_info::count_object(const enum_bstream_item_type type)
> +{
> +
> + switch (type) {
> +
> + case BSTREAM_IT_TABLE:
> + m_table_count++;
> + break;
> + case BSTREAM_IT_VIEW:
> + m_view_count++;
> + break;
> + case BSTREAM_IT_SPROC:
> + case BSTREAM_IT_SFUNC:
> + case BSTREAM_IT_EVENT:
> + case BSTREAM_IT_TRIGGER:
> + m_routine_count++;
> + break;
> + case BSTREAM_IT_PRIVILEGE:
> + m_priv_count++;
> + break;
> +
> + case BSTREAM_IT_DB: // counted via m_dbs.count()
> + case BSTREAM_IT_TABLESPACE: // counted via m_ts_map.count()
> + case BSTREAM_IT_CHARSET: // not counted yet
> + case BSTREAM_IT_USER: // not counted yet
> + break;
> + default: DBUG_ASSERT(FALSE); // explicitly count or ignore any new type
[2] An assertion here is more definitive than the comment suggests.
Please correct the comment to reflect what really happens when (if) the
assertion is reached.
> + }
> +}
> +
> +/**
> + Get number of named objects (ts, db, table, view, routines) in the
> + image.
> +*/
> +inline
> +uint Image_info::object_count() const
> +{
> + return db_count()
> + + ts_count()
> + + table_count()
> + + view_count()
> + + routine_count();
> +}
[3] Please align the function calls. Also, I might suggest adding
parenthesis and moving the operator to the previous line to make it more
readable. For example:
return (db_count() +
ts_count() +
...
routine_count());
> +
> +/// Returns number of routines in the image.
> +inline
> +uint Image_info::routine_count() const
> +{
> + return m_routine_count;
> +}
> +
> /// Returns number of databases in the image.
> inline
> uint Image_info::db_count() const
> @@ -737,11 +806,25 @@ uint Image_info::ts_count() const
>
> /// Returns total number of tables in the image.
> inline
> -ulong Image_info::table_count() const
> +uint Image_info::table_count() const
[6] Why did you change to uint from ulong? Will this cause issues or are
you fixing issues?
...
[7] Was an official decision made as to what will and will not be
counted? I don't recall seeing that decision and it is not documented in
the bug report.
Chuck
