Hi Chuck,
Charles Bell wrote:
> STATUS
> ------
> Patch rejected.
>
> REQUIRED
> --------
> 1. I think there may be a bug in this code. Please see BUG#47386 and
> Rafal's solution for case sensitive file systems. For example, does
> this query return two tables when t1 and T1 exist in the same database
> on Linux? Please verify and correct if necessary.
--> I did not see any issues with tables in the BUG#47386. The
query returned two tables t1 and T1 when they exist in the same database
on Linux.
>
> 2. Why are we pushing a temporary fix that we know will be redundant
> work?
--> Even if BUG#47697 is fixed, the current fix for BUG#45587
works properly. The redundancy and confusion is *for those developers*
who look at
the current fix in the future, i.e the transformed query
s_stream in obs::get_tables().
So, as you have already mentioned our decision to push
this fix comes down to the urgency of fixing this bug.
>
> REQUESTS
> --------
> 3. Please use 'BUG#NNNNN' (all caps). This is the convention we follow.
>
> 4. Please keep your comment lines to < 80 characters. This makes the
> reading of the comments much easier.
>
> 5. Please keep comments to the point and readable (and grammatically
> correct please). ;)
--> Thank you Chuck for pointing out the
inconveniences/conventions. All your requests
will be implemented.
>
> DETAILS
> -------
> > 2876 Ritheesh Vedire 2009-09-30
> > Bug#45587 Backup of tables using a non-existent storage engine
>
> [3] Please use 'BUG' (all caps).
--> I have already started using it. :)
>
> > does not give warning/error
> >
> > This happens because backup engine does not know the
> existence of
>
> [4] Please keep lines to < 80 characters per line.
>
> > all invalid tables
> > @ mysql-test/suite/backup/t/backup_no_engine.test
> > The test file was edited and RESTORE command is removed ( as
> > there is no point in
> > restoring if backup fails) . This test reports errors for unidentified
> > storage engines
>
> [5] Please use complete sentences and keep it short. I would have
> written the above like this (just a suggestion):
>
> Backup now fails for unidentified storage engines. Thus, the restore
> command removed from the test case.
>
> > @ sql/si_objects.cc
> > The query s_stream in the function obs::get_db_tables() (file:
> > si_objects.cc):
> >
> > SELECT <db_name>, table_name FROM
> > INFORMATION_SCHEMA.TABLES
> > WHERE table_schema = '<db_name>' AND table_type
> = 'BASE
> > TABLE';
>
> [4] < 80 chars, please.
>
> >
> > does not return invalid tables . See bug#47697
>
> [3] BUG#47697
>
> [5] This is too wordy. We don't need so much detail. It's ok as is
> (except for the grammar), but I would have described the problem and
> solution in 1-2 sentences if possible.
>
> >
> > So I have changed the query , to get all the tables
> > including invalid ones from the information_schema.tables.
> After
> > that
> > back up engine knows about invalid tables and throws error
> > messages.
> >
> > This fix is temporary . once bug#47697 is fixed, the
> > present
> > fix is redundant and we can revert to original s_stream query.
>
>
> [2] I do not think it is appropriate to push temporary fixes unless
> there is an urgent need or the triage team has deemed the fix needed
> for an immediate or near release. AFAICT, 'CHECKED' does not fit this
> criteria. I am therefore of the opinion that this patch not be pushed
> but rather this bug be annotated as dependent on BUG#47697 and
> reevaluated when that bug is fixed. Sorry, it's a cart before the
> horse issue. Specifically, how do we *know* the patch for BUG#47697
> *will* fix the problem and not introduce other problems? In another
> vein, what are we gaining by pushing a patch that we know will be
> fixed in a more appropriate manner (I assume) in the future?
--> If BUG#47697 is fixed, BUG#45587 is automatically fixed. If
there is no urgency for the BUG#45587 to be fixed, even I'm of the
opinion that it should
be set as dependent on BUG#47697. I don't know how we
decide the state of urgency.
On another note, I can argue upon the fact that fix for
BUG#47697 will *not* introduce any new issues concerning BUG#45587.
> And how do we communicate that information clearly so that we don't
> spend unnecessary time diagnosing the redundancy?
--> Yes, if we decide to push this fix we have to communicate
to the team that fixes BUG#47697. Does a description of 'what-to-do' on
the BUG#47697 page
will solve this issue?
> Lastly, BUG#47697 has not been evaluated by the triage team so the
> urgency of that fix is uncertain.
--> What shall I do with this bug now?
>
> [4] < 80 chars, please. Note: periods follow the last character in a
> sentence with no spaces. Similarly, there is no space then using commas.
>
> >
> > modified:
> > mysql-test/suite/backup/r/backup_no_engine.result
> > mysql-test/suite/backup/t/backup_no_engine.test
> > sql/si_objects.cc
> > === modified file 'mysql-test/suite/backup/r/backup_no_engine.result'
> > --- a/mysql-test/suite/backup/r/backup_no_engine.result 2009-06-18
> > 11:28:01 +0000
> > +++ b/mysql-test/suite/backup/r/backup_no_engine.result 2009-09-30
> > 13:40:57 +0000
> > @@ -6,19 +6,5 @@ Tables_in_db
> > t1
> > t2
> > BACKUP DATABASE db TO "db.backup";
> > -backup_id
> > -#
> > -DROP DATABASE db;
> > -RESTORE FROM "db.backup";
> > -backup_id
> > -#
> > -SHOW TABLES IN db;
> > -Tables_in_db
> > -t1
> > -SHOW CREATE TABLE db.t1;
> > -Table Create Table
> > -t1 CREATE TABLE `t1` (
> > - `a` int(11) DEFAULT NULL,
> > - `b` char(32) DEFAULT NULL
> > -) ENGINE=MyISAM DEFAULT CHARSET=latin1
> > +ERROR 42000: Unknown storage engine 'PARADOX'
>
> Hey, I've used Paradox! hehehe :D
>
> > DROP DATABASE db;
> >
> > === modified file 'mysql-test/suite/backup/t/backup_no_engine.test'
> > --- a/mysql-test/suite/backup/t/backup_no_engine.test 2009-06-18
> > 11:28:01 +0000
> > +++ b/mysql-test/suite/backup/t/backup_no_engine.test 2009-09-30
> > 13:40:57 +0000
> > @@ -20,21 +20,11 @@ copy_file $table_def $MYSQLD_DATADIR/db/
> >
> > SHOW TABLES IN db;
> >
> > -# backup test database, the table with no storage engine will be
> ignored
> > +# backup test database, an error will be displayed
> > +# as storage engine of the table is not identified
> > --replace_column 1 #
> > +--error ER_UNKNOWN_STORAGE_ENGINE
> > BACKUP DATABASE db TO "db.backup";
> >
> > -# wipe-out backed-up database
> > -DROP DATABASE db;
> > -
> > ---replace_column 1 #
> > -RESTORE FROM "db.backup";
> > -
> > -# check that the table with no engine was excluded
> > -SHOW TABLES IN db;
> > -# check that table t was correctly restored
> > -SHOW CREATE TABLE db.t1;
> > -
> > # clean up
> > DROP DATABASE db;
> > -remove_file $MYSQLD_BACKUPDIR/db.backup;
> >
> > === modified file 'sql/si_objects.cc'
> > --- a/sql/si_objects.cc 2009-09-10 13:46:13 +0000
> > +++ b/sql/si_objects.cc 2009-09-30 13:40:57 +0000
> > @@ -2645,10 +2645,12 @@ Obj_iterator *get_db_tables(THD *thd, co
> > String_stream s_stream;
> >
> > s_stream <<
> > - "SELECT '" << db_name << "', table_name "
> > + "SELECT '" << db_name << "', table_name "
> > + "FROM ( SELECT * "
> > "FROM INFORMATION_SCHEMA.TABLES "
> > "WHERE table_schema = '" << db_name << "' AND "
> > - "table_type = 'BASE TABLE'";
> > + "table_type = 'BASE TABLE'"
> > + ") AS get_table";
>
> [1] There is a possible issue here concerning systems that support
> case sensitive file systems. See BUG#47386.
>
> >
> > return create_row_set_iterator<Db_tables_iterator>(thd,
> > s_stream.lex_string());
> > }
> >
> >
>
> Chuck
>
Thanks,
--Ritheesh.
