maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Sergey!

Yes, that's what I had in mind, thanks!
There are few rough edges, see below.
Ok to push after you fix that, there's no need for another review round.

On Dec 26, svoj@xxxxxxxxxxx wrote:
> revision-id: 72e58dc2b1a0bb859e918de0ff820d5a2a92dc32
> parent(s): db89dd3a8f7b0d868946d25ba98c6f88612d309a
> committer: Sergey Vojtovich
> branch nick: 10.1
> timestamp: 2014-12-26 17:03:45 +0400
> message:
> 
> MDEV-7273 - 10.1 fails to start up during tc_log initializations on PPC64
> 
> log-tc-size is 24K by default. Page size is 64K on PPC64. But log-tc-size
> must be at least 3 x page size. This is enforced by TC_LOG_MMAP::open()
> with a comment: to guarantee non-empty pool.
> 
> This all makes server not startable in default configuration on PPC64.
> 
> Autosize log-tc-size, so that it's min value= page size * 6, default
> value= page size * 6, block size= page size.
> 
> ---
>  mysql-test/r/mysqld--help.result                           |  1 -
>  mysql-test/suite/sys_vars/r/log_tc_size_basic.result       |  4 ++++
>  .../suite/sys_vars/r/sysvars_server_notembedded.result     | 14 ++++++++++++++
>  mysql-test/suite/sys_vars/t/log_tc_size_basic.test         |  5 +++++
>  mysql-test/t/mysqld--help.test                             |  2 +-
>  sql/log.cc                                                 |  5 ++---
>  sql/log.h                                                  |  3 +--
>  sql/mysqld.cc                                              |  7 +------
>  sql/sys_vars.cc                                            | 11 +++++++++++
>  9 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/mysql-test/suite/sys_vars/t/log_tc_size_basic.test b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test
> new file mode 100644
> index 0000000..1fce212
> --- /dev/null
> +++ b/mysql-test/suite/sys_vars/t/log_tc_size_basic.test
> @@ -0,0 +1,5 @@
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE
> +SET GLOBAL log_tc_size=1;
> +
> +--error ER_INCORRECT_GLOBAL_LOCAL_VAR,ER_UNKNOWN_SYSTEM_VARIABLE

do we ever build on platforms without mmap?
if the answer is "no" - you don't need ER_UNKNOWN_SYSTEM_VARIABLE here

> +SET SESSION log_tc_size=1;
> diff --git a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> index 006b9a9..5491169 100644
> --- a/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> +++ b/mysql-test/suite/sys_vars/r/sysvars_server_notembedded.result
> @@ -1757,6 +1757,20 @@ NUMERIC_BLOCK_SIZE	NULL
>  ENUM_VALUE_LIST	innodb,query_plan,explain
>  READ_ONLY	NO
>  COMMAND_LINE_ARGUMENT	REQUIRED
> +VARIABLE_NAME	LOG_TC_SIZE
> +SESSION_VALUE	NULL
> +GLOBAL_VALUE	24576
> +GLOBAL_VALUE_ORIGIN	AUTO
> +DEFAULT_VALUE	24576
> +VARIABLE_SCOPE	GLOBAL
> +VARIABLE_TYPE	BIGINT UNSIGNED
> +VARIABLE_COMMENT	Size of transaction coordinator log.
> +NUMERIC_MIN_VALUE	24576
> +NUMERIC_MAX_VALUE	18446744073709551615
> +NUMERIC_BLOCK_SIZE	4096
> +ENUM_VALUE_LIST	NULL
> +READ_ONLY	YES
> +COMMAND_LINE_ARGUMENT	REQUIRED

if the answer is "yes" - this test will fail there.
even if the answer is "no", the variable value is architecture
dependent.

But it's good to see all stable variable metadata (e.g. that
GLOBAL_VALUE_ORIGIN is AUTO).

>  VARIABLE_NAME	LOG_WARNINGS
>  SESSION_VALUE	1
>  GLOBAL_VALUE	1
> diff --git a/sql/log.cc b/sql/log.cc
> index 66e1426..6d6512e 100644
> --- a/sql/log.cc
> +++ b/sql/log.cc
> @@ -8793,7 +8792,7 @@ int TC_LOG_MMAP::unlog(ulong cookie, my_xid xid)
>    mysql_mutex_lock(&LOCK_pending_checkpoint);
>    if (pending_checkpoint == NULL)
>    {
> -    uint32 size= sizeof(*pending_checkpoint);
> +    uint32 size= sizeof(*pending_checkpoint) + tc_log_page_size / sizeof(my_xid);

that's not enough. There are further sizeof's, like

  if (pending_checkpoint->count == sizeof(pending_checkpoint->cookies) /
      sizeof(pending_checkpoint->cookies[0]))

and

  for (i= 0; i < sizeof(pending->cookies)/sizeof(pending->cookies[0]); ++i)

I've found these two by looking for "sizeof" in the Kristian's patch
that introduced pending_checkpoint. There can be more sizeofs added
later.

>      if (!(pending_checkpoint=
>            (pending_cookies *)my_malloc(size, MYF(MY_ZEROFILL))))
>      {
> diff --git a/sql/sys_vars.cc b/sql/sys_vars.cc
> index fb11377..bfaf7d7 100644
> --- a/sql/sys_vars.cc
> +++ b/sql/sys_vars.cc
> @@ -5202,3 +5202,14 @@ static Sys_var_mybool Sys_strict_password_validation(
>         "that cannot be validated (passwords specified as a hash)",
>         GLOBAL_VAR(strict_password_validation),
>         CMD_LINE(OPT_ARG), DEFAULT(TRUE), NO_MUTEX_GUARD, NOT_IN_BINLOG);
> +
> +#ifdef HAVE_MMAP
> +static Sys_var_ulong Sys_log_tc_size(
> +       "log_tc_size",
> +       "Size of transaction coordinator log.",
> +       READ_ONLY GLOBAL_VAR(opt_tc_log_size),
> +       CMD_LINE(REQUIRED_ARG),
> +       VALID_RANGE(my_getpagesize() * 6, ULONG_MAX),
> +       DEFAULT(my_getpagesize() * 6),
> +       BLOCK_SIZE(my_getpagesize()));

I think, min value could be my_getpagesize()*3 ?

> +#endif

Regards,
Sergei