maria-developers team mailing list archive

[Sergey Vojtovich <svoj@xxxxxxxxxxx>]

Hi Sanja,

ok to push, a few minor notes JFYI inline. I found only one real bug, but
generally I agree that we should fix these fragile macros.

On Tue, Sep 01, 2015 at 11:47:09AM +0200, sanja@xxxxxxxxxxx wrote:
> revision-id: 0a1b60a8aee4bce3235fd5a1d8cb2231b51ff5cc (mariadb-10.0.21-7-g0a1b60a)
> parent(s): 4b41e3c7f33714186c97a6cc2e6d3bb93b050c61
> committer: Oleksandr Byelkin
> timestamp: 2015-09-01 11:47:06 +0200
> message:
> 
> MDEV-8450: PATCH] Wrong macro expansion in Query_cache::send_result_to_client()
> 
> Expression in macro protected by ()
> 
> diff --git a/include/maria.h b/include/maria.h
> index 16a9bea..cdaeade 100644
> --- a/include/maria.h
> +++ b/include/maria.h
> @@ -43,7 +43,7 @@ extern "C" {
>  #define MARIA_NAME_IEXT	".MAI"
>  #define MARIA_NAME_DEXT	".MAD"
>  /* Max extra space to use when sorting keys */
> -#define MARIA_MAX_TEMP_LENGTH	2*1024L*1024L*1024L
> +#define MARIA_MAX_TEMP_LENGTH	(2*1024L*1024L*1024L)
>  /* Possible values for maria_block_size (must be power of 2) */
>  #define MARIA_KEY_BLOCK_LENGTH	8192		/* default key block length */
>  #define MARIA_MIN_KEY_BLOCK_LENGTH	1024	/* Min key block length */
Was this used at all? May be remove it?

> diff --git a/sql/log_slow.h b/sql/log_slow.h
> index 2ae07da..3ae2060 100644
> --- a/sql/log_slow.h
> +++ b/sql/log_slow.h
> @@ -16,23 +16,23 @@
>  /* Defining what to log to slow log */
>  
>  #define LOG_SLOW_VERBOSITY_INIT           0
> -#define LOG_SLOW_VERBOSITY_INNODB         1 << 0
> -#define LOG_SLOW_VERBOSITY_QUERY_PLAN     1 << 1
> -#define LOG_SLOW_VERBOSITY_EXPLAIN        1 << 2
> +#define LOG_SLOW_VERBOSITY_INNODB         (1 << 0)
Was LOG_SLOW_VERBOSITY_INNODB used at all? May be remove it?

> +#define LOG_SLOW_VERBOSITY_QUERY_PLAN     (1 << 1)
> +#define LOG_SLOW_VERBOSITY_EXPLAIN        (1 << 2)
>  
>  #define QPLAN_INIT            QPLAN_QC_NO
>  
> -#define QPLAN_ADMIN           1 << 0
> -#define QPLAN_FILESORT        1 << 1
> -#define QPLAN_FILESORT_DISK   1 << 2
> -#define QPLAN_FULL_JOIN       1 << 3
> -#define QPLAN_FULL_SCAN       1 << 4
> -#define QPLAN_QC              1 << 5
> -#define QPLAN_QC_NO           1 << 6
> -#define QPLAN_TMP_DISK        1 << 7
> -#define QPLAN_TMP_TABLE       1 << 8
> -#define QPLAN_FILESORT_PRIORITY_QUEUE       1 << 9
> +#define QPLAN_ADMIN           (1 << 0)
> +#define QPLAN_FILESORT        (1 << 1)
> +#define QPLAN_FILESORT_DISK   (1 << 2)
> +#define QPLAN_FULL_JOIN       (1 << 3)
> +#define QPLAN_FULL_SCAN       (1 << 4)
> +#define QPLAN_QC              (1 << 5)
> +#define QPLAN_QC_NO           (1 << 6)
> +#define QPLAN_TMP_DISK        (1 << 7)
> +#define QPLAN_TMP_TABLE       (1 << 8)
> +#define QPLAN_FILESORT_PRIORITY_QUEUE       (1 << 9)
>  
>  /* ... */
> -#define QPLAN_MAX             ((ulong) 1) << 31 /* reserved as placeholder */
> +#define QPLAN_MAX             (((ulong) 1) << 31) /* reserved as placeholder */
>  
log_slow.h was fixed in rev.203f4d419 in my 10.0. Did you use outdated tree?

I may be wrong, but there seem to be no real bug with QPLAN_QC_NO currently.
Just because nobody sets it. So in fact it clears QPLAN_ADMIN, which FWICS is
never set at this point.

But it is a good to have it fixed nevertheless.

Btw, QPLAN_QC_NO doesn't seem to be used in fact. May be remove it?

> diff --git a/storage/archive/archive_test.c b/storage/archive/archive_test.c
> index d01c1e0..bb052b8 100644
> --- a/storage/archive/archive_test.c
> +++ b/storage/archive/archive_test.c
> @@ -33,7 +33,7 @@
>  
>  #define ARCHIVE_ROW_HEADER_SIZE 4
>  
> -#define BUFFER_LEN 1024 + ARCHIVE_ROW_HEADER_SIZE
> +#define BUFFER_LEN (1024 + ARCHIVE_ROW_HEADER_SIZE)
>  
>  char test_string[BUFFER_LEN];
Alright, there was a bug in this line:
assert(write_length != count * BUFFER_LEN);

> diff --git a/strings/ctype.c b/strings/ctype.c
> index 048fbe3..d8a1dd7 100644
> --- a/strings/ctype.c
> +++ b/strings/ctype.c
> @@ -264,7 +264,7 @@ static const struct my_cs_file_section_st
>  }
>  
>  #define MY_CS_CSDESCR_SIZE	64
> -#define MY_CS_TAILORING_SIZE	32*1024
> +#define MY_CS_TAILORING_SIZE	(32*1024)
>  #define MY_CS_UCA_VERSION_SIZE  64
>  #define MY_CS_CONTEXT_SIZE      64
Was this used at all? May be remove it?

Regards,
Sergey