maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Jan!

First, we've discussed adding support for ALTER TABLE ... DISABLE KEYS
to InnoDB. Did you try that? That would've been a better approach,
generally I'd like to avoid adding plugin-specific options to
a mysqldump utility (although, of course, InnoDB is not just any plugin,
so we can always make an exception for it).

On Jul 27, Jan Lindström wrote:
> revision-id: 7fd55542893b6d9e75406e39b7be672d8a8f473c
> parent(s): a6ab8ef9d7d465a3f0bc1cff6034ca97575629dd
> committer: Jan Lindström
> branch nick: 10.0-git
> timestamp: 2015-07-27 13:02:57 +0300
> message:
> 
> MDEV-5171: Add support for --innodb-optimize-keys to mysqldump.
> 
>   Merged revision 93 from bzr+ssh://bazaar.launchpad.net/+branch/percona-server/5.6/
>   authored by Alexey Kopytov.
> 
>   However, internal parser is rewriten to support unquoted identifiers and
>   multi-column keys.
> 
>   This patch expands the applicability of InnoDB fast index creation to
>   mysqldump, ALTER TABLE and OPTIMIZE TABLE as follows:
> 
>   1. mysqldump has now a new option, --innodb-optimize-keys, which changes
>   the way InnoDB tables are dumped so that secondary and foreign keys are
>   created after loading the data thus taking advantage of fast index
>   creation.
> 
>   This part of the patch is an implementation of the feature request
>   reported as MySQL bug #49120.
> 
>   More specifically:
> 
>   - KEY, UNIQUE KEY and CONSTRAINT specifications are omitted from CREATE
>   TABLE corresponding to InnoDB tables.
> 
>   - an additional ALTER TABLE is issued after dumping the data to create
>   the previously omitted keys.
> 
>   Delaying foreign key creation does not introduce any additional risks as
>   mysqldump always prepends its output with SET FOREIGN_KEY_CHECKS=0 anyway.
> 
>   2. When ALTER TABLE requires a table copy, secondary keys are now dropped
>   and recreated later after copying the data. The following restrictions
>   apply:
> 
>   - only non-unique keys can be involved in this optimization
> 
>   - if the table contains foreign keys, or a foreign key is being added as
>   a part of the current ALTER TABLE statement, the optimization is
>   disabled for all keys.
> 
>   This part of the patch is an implementation of the feature request
>   reported as MySQL bug #57583.
> 
>   3. As OPTIMIZE TABLE is mapped to ALTER TABLE ... ENGINE=InnoDB for
>   InnoDB tables, it now also benefits from fast index creation with the
>   same restrictions as for ALTER TABLE.
> 

This is a good comment, but a bit confusing. It should explicitly say
somehow that item 1 is what this patch implements and items 2 and 3
are existing InnoDB features. Currently the comment creates an impression
that all 1-3 are implemented in the patch.

> 
> diff --git a/client/mysqldump.c b/client/mysqldump.c
> index e4683ab..b8874e7 100644
> --- a/client/mysqldump.c
> +++ b/client/mysqldump.c
> @@ -84,12 +86,30 @@
>  #define IGNORE_DATA 0x01 /* don't dump data for this table */
>  #define IGNORE_INSERT_DELAYED 0x02 /* table doesn't support INSERT DELAYED */
>  
> +typedef enum {
> +  KEY_TYPE_NONE,
> +  KEY_TYPE_PRIMARY,
> +  KEY_TYPE_UNIQUE,
> +  KEY_TYPE_NON_UNIQUE,
> +  KEY_TYPE_FOREIGN
> +} key_type_t;
> +
>  /* Chars needed to store LONGLONG, excluding trailing '\0'. */
>  #define LONGLONG_LEN 20
>  
>  /* Max length GTID position that we will output. */
>  #define MAX_GTID_LENGTH 1024
>  
> +/* Max length of identifier (columnname, keyname, tablename, etc). */
> +#define MAX_ID_LENGTH 65

Use SAFE_NAME_LEN instead, it's defined in mysql_com.h
that mysqldump.c already includes

> +
> +/* Data structure for holding position information
> +for simple keyword parser. */
> +typedef struct {
> +  char* m_start;
> +  char* m_pos;
> +} simple_parser_t;
> +
>  static void add_load_option(DYNAMIC_STRING *str, const char *option,
>                               const char *option_value);
>  static ulong find_set(TYPELIB *lib, const char *x, uint length,
> @@ -2843,6 +3296,9 @@ static uint get_table_structure(char *table, char *db, char *table_type,
>  
>        row= mysql_fetch_row(result);
>  
> +      if (opt_innodb_optimize_keys && !opt_comments && !strcmp(table_type, "InnoDB"))
> +        skip_secondary_keys(row[1], table);

Okay, so opt_innodb_optimize_keys needs !opt_comments.
Please, add a check, like

  if (opt_innodb_optimize_keys && opt_comments)
  {
    fprintf(stderr, "%s: You can't use --innodb-optimize-keys and "
           "--comments at the same time.\n", my_progname_short);
    return(EX_USAGE);
  }

a good place for a check like this is get_options() function.

> +
>        is_log_table= general_log_or_slow_log_tables(db, table);
>        if (is_log_table)
>          row[1]+= 13; /* strlen("CREATE TABLE ")= 13 */

The parser looked fine on the first glance, but I didn't review every
single line of it.

Regards,
Sergei