Re: [Maria-developers] [Commits] 0a1b60a: MDEV-8450: PATCH] Wrong macro expansion in Query_cache::send_result_to_client()
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@mariadb.com 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
Hi, Sergey! On 04.09.15 15:17, Sergey Vojtovich wrote:
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@mariadb.com 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? see at the end.
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? see at the end.
+#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? After some thinking I decided that author of the original patch will be happier if I merge his patch (it will be in the history of updates), so I did.
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?
I prefer do not remove difines in theory it cal lead to something unpredictable and version is stable.
participants (2)
-
Oleksandr Byelkin -
Sergey Vojtovich