maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Monty!

On Jun 13, Michael Widenius wrote:
> > Hi, Michael!
> 
> Please use Monty. No reason to be formal!

That comes from the sender's name :)
I replace it manually, but sometimes I forget to.

> > > +# create test user
> > > +create user test identified by '123456';
> > > +grant all privileges on test.t1 to 'test'@'%'identified by '123456' with grant option;
> >
> > why with grant option?
> > better remove it if it's not essential for drop to work.
> > if it is essential, why is it?
> 
> Read the comment in the beginning of the test.
> This is to test that one doesn't need a separate privilge to be able to
> drop an inconsistent table.

My question was, why did you need "with grant option" and not just

 grant all privileges on test.t1 to 'test'@'%'identified by '123456';

I was quite eager to know how comes that "with grant option" is
essential to this test.

> > > +--echo #Test4: drop table can drop consistent table as well
> > > +create table t1(a int) engine=innodb;
> > > +drop table t1;
> >
> > aha, so it works! :)
> 
> Orginal code was drop table followed by drop table force.

Sure, if you want to keep a test to ensure that drop table works, it's
up to you, keep it :)
If it were my commit, I'd removed it.

> > > +# second drop with if exists will success
> > > +drop table if exists t1;
> >
> > you've just tested it in Test7, haven't you?
> 
> Read again comment at the beginning of the test file.

Yes, but you don't need to keep duplicate tests, do you?

> > > +# create Aria table t2 and rm .MAI and .MAD
> > > +create table t2(a int) engine=aria;
> > > +flush tables;
> > > +--remove_file $DATADIR/test/t2.MAD
> > > +--remove_file $DATADIR/test/t2.MAI
> > > +--list_files  $DATADIR/test/
> > > +--replace_result \\ /
> > > +drop table t2;
> 
> > may be csv and archive tests too?
> 
> CSV has only one table file, so not that interesting as it's covered by
> other tests.

  /* The file extension */
  #define CSV_EXT ".CSV"               // The data file
  #define CSN_EXT ".CSN"               // Files used during repair and update
  #define CSM_EXT ".CSM"               // Meta file

> Archive has discovery and thus will automatically remove the .frm or .ARZ
> in case of missing files.
> 
> > > diff --git a/sql/handler.cc b/sql/handler.cc
> > > index 1af0157783e..5dd02d057c7 100644
> > > --- a/sql/handler.cc
> > > +++ b/sql/handler.cc
> > > @@ -2674,26 +2712,34 @@ const char *get_canonical_filename(handler *file, const char *path,
> > >  }
> > >
> > >
> > > -/** delete a table in the engine
> > > +/**
> > > +   Delete a table in the engine
> > >
> > > +   @return 0   Table was deleted
> > > +   @return -1  Table didn't exists, no error given
> > > +   @return #   Error from table handler
> > > +
> > >    @note
> > >    ENOENT and HA_ERR_NO_SUCH_TABLE are not considered errors.
> > > -  The .frm file will be deleted only if we return 0.
> > > +  The .frm file should be deleted by the caller only if we return <= 0.
> > >  */
> > > +
> > >  int ha_delete_table(THD *thd, handlerton *table_type, const char *path,
> > > -                    const LEX_CSTRING *db, const LEX_CSTRING *alias, bool generate_warning)
> > > +                    const LEX_CSTRING *db, const LEX_CSTRING *alias,
> > > +                    bool generate_warning)
> > >  {
> > >    handler *file;
> > >    char tmp_path[FN_REFLEN];
> > >    int error;
> > >    TABLE dummy_table;
> > >    TABLE_SHARE dummy_share;
> > > +  bool is_error= thd->is_error();
> > >    DBUG_ENTER("ha_delete_table");
> > >
> > >    /* table_type is NULL in ALTER TABLE when renaming only .frm files */
> > >    if (table_type == NULL || table_type == view_pseudo_hton ||
> > >        ! (file=get_new_handler((TABLE_SHARE*)0, thd->mem_root, table_type)))
> > > -    DBUG_RETURN(0);
> > > +    DBUG_RETURN(-1);
> >
> > "Table didn't exists, no error given"?
> 
> Yes (for this function)
> 
> > if get_new_handler() fails it's OOM.
> 
> No problem with OOM as it sets thd->error() and the user will get that error
> message.

yes, but OOM it's not -1, it's ENOMEM, a hard error.

> > >  int handler::delete_table(const char *name)
> > >  {
> > > -  int saved_error= 0;
> > > -  int error= 0;
> > > -  int enoent_or_zero;
> > > +  int saved_error= ENOENT;
> > > +  bool abort_if_first_file_error= 1;
> > > +  bool some_file_deleted= 0;
> > > +  DBUG_ENTER("handler::delete_table");
> > >
> > > +  // For discovery tables, it's ok if first file doesn't exists
> >
> > why would that be ok?
> 
> For tables with discovery, there may not be any files on disk. They may
> be generated by the discovery process. That first file in a list is
> missing doesn't mean that there isn't other files.

Not really. First extension really means "file exists" even with
discovery.

> > >    for (const char **ext= bas_ext(); *ext ; ext++)
> > >    {
> > > +    int error;
> > > -    if (mysql_file_delete_with_symlink(key_file_misc, name, *ext, 0))
> > > +    if ((error= mysql_file_delete_with_symlink(key_file_misc, name, *ext,
> > > +                                              MYF(0))))
> >
> > why do you need this `error` variable, if you never use it?
> 
> Mostly for debugging. The compiler will delete it, so no big deal.
> I originally thought my_handler_delete_with_symlink() should return the
> errno, but as it can generate several errno that didn't work.

a compiler (depending on the version of flags) will also issue a warning
that a variable is set, but not used. May be move it out of if()?

  int error= mysql_file_delete_with_symlink(...);
  if (error)

that'll be ok for any compiler.

> <cut>
> 
> > >  /**
> > > +   Return true if the error from drop table means that the
> > > +   table didn't exists
> > > +*/
> > > +
> > > +bool non_existing_table_error(int error)
> > > +{
> > > +  return (error == ENOENT || error == HA_ERR_NO_SUCH_TABLE ||
> > > +          error == HA_ERR_UNSUPPORTED ||
> > > +          error == ER_NO_SUCH_TABLE ||
> > > +          error == ER_NO_SUCH_TABLE_IN_ENGINE ||
> > > +          error == ER_WRONG_OBJECT);
> > > +}
> >
> > this should, I believe, be inline in handler.h
> 
> After having to change this multiple times and have to recompile,
> I added this to the .c file.
> As this is only used for DDL's (which are slow), it's better to NOT have
> it in the head file as it's generates less code to have it here.

:)

It doesn't generate less code, it's only used in one other file.
After having to to change this multiple times ans being done with it you
can still move it into handler.h.

> <cut>
> 
> > > +static my_bool delete_table_force(THD *thd, plugin_ref plugin, void *arg)
> > > +{
> > > +  handlerton *hton = plugin_hton(plugin);
> > > +  st_force_drop_table_params *param = (st_force_drop_table_params *)arg;
> > > +
> > > +  /*
> > > +    We have to ignore HEAP tables as these may not have been created yet
> > > +    We also remove engines that is using discovery (as these will recrate
> > > +    any missing .frm if needed) and tables marked with
> > > +    HTON_AUTOMATIC_DELETE_TABLE as for these we can't check if the table
> > > +    ever existed.
> > > +  */
> > > +  if (!hton->discover_table && hton->db_type != DB_TYPE_HEAP &&
> > > +      !(hton->flags & HTON_AUTOMATIC_DELETE_TABLE))
> > > +  {
> > > +    int error;
> > > +    error= ha_delete_table(thd, hton, param->path, param->db,
> > > +                           param->alias, param->generate_warning);
> >
> > This is doing a a lot of extra work. dummy_table/dummy_share, if()s.
> > Allocating and constructing new handlers.
> 
> This is for extreme cases when there is no reason to optimize for
> performance but less code is better.
> 
> > I suggest the following:
> >
> >   1. Introduce new handlerton method hton->drop_table().
> >      By default it'll be
> >
> >         hton->get_new_handler()
> >         get_canonical_filename()
> >         file->ha_delete_table()
> >
> >      like now, so all engines will still work. But later we can start
> >      moving drop table functionality out of the handler. In this commit
> >      only engines like blackhole will have a dummy drop_table() method.
> >
> 
> We already have two handler delete_table methods. Don't want to add yet
> another one.
> 
> Note that a several handlers needs special handling. (Just check who have
> HTON_AUTOMATIC_DELETE_TABLE), not just blackhole,

as a rule, Monty, you can assume that if I've reviewed a patch I've read
every single line and checked every single comment. And after that I
might ask questions or comment on something.

> According to your suggestion, we would have to create a new
> drop_table() for all engines and have flags when to generate errors
> and which errors.  And we would have to duplicate some of that code in
> ha_delete_table().
> 
> Definetly much more code than the current one and much more changes in
> many engines.

Not exactly.

See my commit c1c2f6bb70f in bb-10.5-serg. It removes more code than it
adds.

The next commit adds drop_table implementations in some simple cases.
But in total they still remove more code then they add.
Long term there will be no handler::delete_table() at all, so that code
duplication will go away completely.

> >   2. Here you can do, basically
> >
> >        hton->drop_table(hton, param->path)
> >
> >      without checking for discovery, without a new flag
> >      HTON_AUTOMATIC_DELETE_TABLE. I'm not sure why you check for
> >      DB_TYPE_HEAP, could you please explain that?
> 
> See code comment!  HEAP TABLES are created on open, so when we call
> delete_table_force() the table is not yet created. We can't detect
> that a not yet created table exists, can we ?

How is it different from, say, sequence engine? Sequence tables are also
created on open.

> > > +int ha_delete_table_force(THD *thd, const char *path, const LEX_CSTRING *db,
> > > +                          const LEX_CSTRING *alias, bool generate_warning)
> > > +{
> > > +  st_force_drop_table_params param;
> > > +  Table_exists_error_handler no_such_table_handler;
> > > +  DBUG_ENTER("ha_delete_table_force");
> > > +
> > > +  param.path=             path;
> > > +  param.db=               db;
> > > +  param.alias=            alias;
> > > +  param.generate_warning= generate_warning;
> >
> > this is never used, you always invoke ha_delete_table_force() with
> > generate_warning=0.
> 
> Yes, I know. This was in the original code and it was not used there either.
> However, I can imagine there are scenarios where this would be useful in
> the future, so I kept it. If you feel strongly about it I can delete the
> generate_warning options.

Yes, please, do. We have enough dead code as it is, no need to add more
of it.

> > >        if (!dont_log_query && table_creation_was_logged)
> > >        {
> > >          /*
> > > +          DROP TEMPORARY succeded. For the moment when we only come
> > > +          here on success (error == 0)
> >
> > DBUG_ASSERT(error == 0) please
> 
> Not needed as the code makes it clear if one looks up a couple of lines.
> The comment is more clear than an assert.

No, definitely not. Comments become obsolete and then it's worse than no
comment at all. assert is basically a comment that ensures that it's
always true. Please, add an assert.

> > > @@ -2402,48 +2411,33 @@ int mysql_rm_table_no_locks(THD *thd, TABLE_LIST *tables, bool if_exists,
> > >        path_length= build_table_filename(path, sizeof(path) - 1, db.str,
> > >                                          alias.str, reg_ext, 0);
> > >      }
> > > +
> > >      DEBUG_SYNC(thd, "rm_table_no_locks_before_delete_table");
> > >      error= 0;
> > > -    if (drop_temporary ||
> > > -        (ha_table_exists(thd, &db, &alias, &table_type, &is_sequence) == 0 &&
> > > +    if (drop_temporary)
> > > +    {
> > > +      /* "DROP TEMPORARY" but a temporary table was not found */
> > > +      error= ENOENT;
> > > +    }
> > > +    else if (((frm_exists= ha_table_exists(thd, &db, &alias, &table_type,
> > > +                                           &is_sequence)) == 0 &&
> >
> > this is an overkill. ha_table_exists() will do a full table discovery,
> > you don't need it here, because you'll try to delete anyway.
> 
> This was in old code, nothing changed. I assume that we also need this for
> discovery as ha_delete_table() needs to know in which engine the
> table exists.

I know it's the old code. In the old code it was needed. In your new
DROP logic it is not needed, that's why I think it's better not to have it.

> > > +        if (Table_triggers_list::drop_all_triggers(thd, &db,
> > > +                                                   &table->table_name,
> > > +                                                   MYF(MY_WME |
> > > +                                                       MY_IGNORE_ENOENT)))
> > > +          ferror= -1;
> >
> > Why do you need two copies of code to drop triggers?
> > Why cannot you do it once, on the first drop_all_triggers() place?
> 
> Because the first drop_all_triggers is only done if the .frm exists and
> the table was dropped. This is part of the "drop force", but only in the
> cases where it makes sence to do DROP FORCE:

I know. But you can also do the first (and only) drop_all_triggers() if
.frm doesn't exist. I cannot imagine when it'd be wrong.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx