Skip to content

Commit

Permalink
Fix bypass range query
Browse files Browse the repository at this point in the history 
Summary:
In current bypass implementation, its range query implementation is not correct. For A >= begin and A <= end , it'll only pick begin *or* end as start of the range, but it will only use the prefix as the end of the range, which means it can potentially scan a lot more rows than needed. It rely on the condition A >= begin / A <= end to filter out the unnecessary rows so the end result is correct (that's why this issue is not discovered in testing).

The correct way to implement is to set up (begin, end) range slice correctly, and do NOT rely on any conditional evaluation to filter rows. The tricky part is to determine the correct begin/end based on a few factors:
* forward / reverse column family
* ascending / descending orders
* inclusive vs non-inclusive ranges (>=, <= vs <, >)
* prefix query vs range query

For better testing, I've done the following:
* verify_bypass_query.inc that will run the query in bypass and non-bypass, verify the row reads are same or less than non-bypass case to ensure such issue won't happen again. Most bypass query validation are moved to use verify_bypass_query.inc. We can also add more validation in the future in verify_bypass_query.inc as needed. As a bonus, it'll make verifying the results are the same in bypass/non-bypass case easy as well.
* move range key validation from bypass_select_scenarios into bypass_select_range_pk and bypass_select_range_sk
* added more test cases to bypass_select_range_pk / bypass_select_range_sk, especially for PK case where some scenarios are missing
* dump query results to file and verify query results didn't change w/ and w/o bypass. I had to back port `output` mysqltest support to make this work properly (for some reason --exec $MYSQL doesn't like multiline queries".

For review, there is no need to go through results file as there are validation in-place to make sure query return same results w/ and w/o bypass.

Reviewed By: luqun

Differential Revision: D25982322

fbshipit-source-id: c857f9a382e
  • Loading branch information
@yizhang82 @facebook-github-bot
yizhang82 authored and facebook-github-bot committed Feb 14, 2021
1 parent 5eba6ba commit ef9a677
Show file tree
Hide file tree
Showing 24 changed files with 25,839 additions and 7,032 deletions.
56 changes: 53 additions & 3 deletions client/mysqltest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -813,6 +813,7 @@ enum enum_commands {
Q_DISABLE_DEPRECATE_EOF,
Q_ENABLE_CLIENT_INTERACTIVE,
Q_RESET_CONNECTION,
Q_OUTPUT, /* redirect output to a file */
Q_UNKNOWN, /* Unknown command. */
Q_COMMENT, /* Comments, ignored. */
Q_COMMENT_WITH_COMMAND,
Expand Down Expand Up @@ -930,6 +931,7 @@ const char *command_names[]=
"disable_deprecate_eof",
"enable_client_interactive",
"resetconnection",
"output",

0
};
Expand Down Expand Up @@ -974,6 +976,7 @@ struct st_command
my_bool abort_on_error, used_replace;
struct st_expected_errors expected_errors;
char require_file[FN_REFLEN];
char output_file[FN_REFLEN];
enum enum_commands type;
};

Expand Down Expand Up @@ -1066,6 +1069,9 @@ void free_replace_regex();
/* Used by sleep */
void check_eol_junk_line(const char *eol);

static void var_set(const char *var_name, const char *var_name_end,
const char *var_val, const char *var_val_end);

void free_all_replace(){
free_replace();
free_replace_regex();
Expand Down Expand Up @@ -1737,8 +1743,7 @@ void handle_command_error(struct st_command *command, uint error)
{
DBUG_PRINT("info", ("command \"%.*s\" failed with expected error: %d",
command->first_word_len, command->query, error));
revert_properties();
DBUG_VOID_RETURN;
goto end;
}
if (command->expected_errors.count > 0)
die("command \"%.*s\" failed with wrong error: %d. my_errno=%d",
Expand All @@ -1752,6 +1757,16 @@ void handle_command_error(struct st_command *command, uint error)
command->first_word_len, command->query,
command->expected_errors.err[0].code.errnum);
}
end:
// Save error code
{
const char var_name[]= "__error";
char buf[10];
int err_len= snprintf(buf, 10, "%u", error);
buf[err_len > 9 ? 9 : err_len]= '0';
var_set(var_name, var_name + 7, buf, buf + err_len);
}

revert_properties();
DBUG_VOID_RETURN;
}
Expand Down Expand Up @@ -3908,6 +3923,14 @@ void do_exec(struct st_command *command)
die("command \"%s\" succeeded - should have failed with errno %d...",
command->first_argument, command->expected_errors.err[0].code.errnum);
}
// Save error code
{
const char var_name[]= "__error";
char buf[10];
int err_len= snprintf(buf, 10, "%u", error);
buf[err_len > 9 ? 9 : err_len]= '0';
var_set(var_name, var_name + 7, buf, buf + err_len);
}

dynstr_free(&ds_cmd);
DBUG_VOID_RETURN;
Expand Down Expand Up @@ -9055,7 +9078,7 @@ void run_query(struct st_connection *cn, struct st_command *command, int flags)
Create a temporary dynamic string to contain the output from
this query.
*/
if (command->require_file[0])
if (command->require_file[0] || command->output_file[0])
{
init_dynamic_string(&ds_result, "", 1024, 1024);
ds= &ds_result;
Expand Down Expand Up @@ -9240,6 +9263,13 @@ void run_query(struct st_connection *cn, struct st_command *command, int flags)
check_require(ds, command->require_file);
}

if (command->output_file[0])
{
/* An output file was specified for _this_ query */
str_to_file2(command->output_file, ds_result.str, ds_result.length, false);
command->output_file[0]= 0;
}

if (ds == &ds_result)
dynstr_free(&ds_result);
DBUG_VOID_RETURN;
Expand Down Expand Up @@ -9663,9 +9693,11 @@ int main(int argc, char **argv)
my_bool q_send_flag= 0, abort_flag= 0;
uint command_executed= 0, last_command_executed= 0;
char save_file[FN_REFLEN];
char output_file[FN_REFLEN];
MY_INIT(argv[0]);

save_file[0]= 0;
output_file[0]= 0;
TMPDIR[0]= 0;

init_signal_handling();
Expand Down Expand Up @@ -10084,6 +10116,11 @@ int main(int argc, char **argv)
strmake(command->require_file, save_file, sizeof(save_file) - 1);
*save_file= 0;
}
if (*output_file)
{
strmake(command->output_file, output_file, sizeof(output_file) - 1);
*output_file= 0;
}
run_query(cur_con, command, flags);
display_opt_trace(cur_con, command, flags);
command_executed++;
Expand Down Expand Up @@ -10314,6 +10351,19 @@ int main(int argc, char **argv)
break;
case Q_ENABLE_CLIENT_INTERACTIVE:
enable_client_interactive = TRUE;
break;
case Q_OUTPUT:
{
static DYNAMIC_STRING ds_to_file;
const struct command_arg output_file_args[] =
{{ "to_file", ARG_STRING, TRUE, &ds_to_file, "Output filename" }};
check_command_args(command, command->first_argument,
output_file_args, 1, ' ');
strmake(output_file, ds_to_file.str, FN_REFLEN);
dynstr_free(&ds_to_file);
break;
}

default:
processed= 0;
break;
Expand Down
48 changes: 48 additions & 0 deletions mysql-test/suite/rocksdb/include/bypass_create_table.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,62 @@ CREATE TABLE `link_table5` (
) ENGINE=ROCKSDB DEFAULT CHARSET=latin1 COLLATE=latin1_bin
ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8;

insert into link_table5 values (0, 1, 2, 2, 0, 1, 'data12', 1, 1);
insert into link_table5 values (0, 1, 3, 2, 0, 2, 'data13', 1, 1);
insert into link_table5 values (0, 1, 4, 2, 0, 2, 'data14', 1, 1);
insert into link_table5 values (0, 1, 5, 2, 0, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 6, 2, 0, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 7, 2, 0, 1, 'data15', 1, 1);
insert into link_table5 values (1, 1, 2, 2, 0, 1, 'data12', 1, 1);
insert into link_table5 values (1, 1, 3, 2, 0, 2, 'data13', 1, 1);
insert into link_table5 values (1, 1, 4, 2, 0, 2, 'data14', 1, 1);
insert into link_table5 values (1, 1, 5, 2, 0, 1, 'data15', 1, 1);
insert into link_table5 values (2, 1, 1, 2, 0, 1, 'data21', 1, 1);
insert into link_table5 values (2, 1, 2, 2, 0, 1, 'data22', 1, 1);
insert into link_table5 values (2, 1, 3, 2, 0, 1, 'data32', 1, 1);
insert into link_table5 values (0, 1, 2, 2, 1, 1, 'data12', 1, 1);
insert into link_table5 values (0, 1, 3, 2, 1, 2, 'data13', 1, 1);
insert into link_table5 values (0, 1, 4, 2, 1, 2, 'data14', 1, 1);
insert into link_table5 values (0, 1, 5, 2, 1, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 6, 2, 1, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 7, 2, 1, 1, 'data15', 1, 1);
insert into link_table5 values (1, 1, 2, 2, 1, 1, 'data12', 1, 1);
insert into link_table5 values (1, 1, 3, 2, 1, 2, 'data13', 1, 1);
insert into link_table5 values (1, 1, 4, 2, 1, 2, 'data14', 1, 1);
insert into link_table5 values (1, 1, 5, 2, 1, 1, 'data15', 1, 1);
insert into link_table5 values (2, 1, 1, 2, 1, 1, 'data21', 1, 1);
insert into link_table5 values (2, 1, 2, 2, 1, 1, 'data22', 1, 1);
insert into link_table5 values (2, 1, 3, 2, 1, 1, 'data32', 1, 1);
insert into link_table5 values (0, 1, 2, 2, 2, 1, 'data12', 1, 1);
insert into link_table5 values (0, 1, 3, 2, 2, 2, 'data13', 1, 1);
insert into link_table5 values (0, 1, 4, 2, 2, 2, 'data14', 1, 1);
insert into link_table5 values (0, 1, 5, 2, 2, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 6, 2, 2, 1, 'data15', 1, 1);
insert into link_table5 values (0, 1, 7, 2, 2, 1, 'data15', 1, 1);
insert into link_table5 values (1, 1, 2, 2, 2, 1, 'data12', 1, 1);
insert into link_table5 values (1, 1, 3, 2, 2, 2, 'data13', 1, 1);
insert into link_table5 values (1, 1, 4, 2, 2, 2, 'data14', 1, 1);
insert into link_table5 values (1, 1, 5, 2, 2, 1, 'data15', 1, 1);
insert into link_table5 values (2, 1, 1, 2, 2, 1, 'data21', 1, 1);
insert into link_table5 values (2, 1, 2, 2, 2, 1, 'data22', 1, 1);
insert into link_table5 values (2, 1, 3, 2, 2, 1, 'data32', 1, 1);


CREATE TABLE `link_table5_rev` (
`id1` bigint(20) unsigned NOT NULL DEFAULT '0',
`id1_type` int(10) unsigned NOT NULL DEFAULT '0',
`id2` bigint(20) unsigned NOT NULL DEFAULT '0',
`id2_type` int(10) unsigned NOT NULL DEFAULT '0',
`link_type` bigint(20) unsigned NOT NULL DEFAULT '0',
`visibility` tinyint(3) NOT NULL DEFAULT '0',
`data` varchar(255) COLLATE latin1_bin NOT NULL DEFAULT '',
`time` int(10) unsigned NOT NULL DEFAULT '0',
`version` bigint(20) unsigned NOT NULL DEFAULT '0',
PRIMARY KEY (`link_type`,`id1`,`id2`) COMMENT 'rev:cf_link'
) ENGINE=ROCKSDB DEFAULT CHARSET=latin1 COLLATE=latin1_bin
ROW_FORMAT=COMPRESSED KEY_BLOCK_SIZE=8;

insert into link_table5_rev SELECT * FROM link_table5;

CREATE TABLE `link_table3` (
`id1` bigint(20) unsigned NOT NULL DEFAULT '0',
Expand Down
101 changes: 101 additions & 0 deletions mysql-test/suite/rocksdb/include/verify_bypass_query.inc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
# Run bypass query and print rows being read
# This is needed to make sure bypass query read exactly the right amount of
# rows and avoid performance regression
#
# Parameters
# $bypass_query
# Bypass query to run

--disable_query_log
SELECT @@rocksdb_select_bypass_policy INTO @__rocksdb_select_bypass_policy;
SET GLOBAL rocksdb_select_bypass_policy=2;

SELECT variable_value INTO @bypass_rows_read_0 FROM
information_schema.global_status WHERE
variable_name="rocksdb_rows_read";
SELECT variable_value INTO @bypass_rows_sent_0 FROM
information_schema.global_status WHERE
variable_name="rows_sent";
SELECT variable_value INTO @bypass_executed_0 FROM
information_schema.global_status WHERE
variable_name="rocksdb_select_bypass_executed";

--echo ========== Verifying Bypass Query ==========
--echo WITH BYPASS:

--enable_query_log
--output $MYSQL_TMP_DIR/bypass_query_rows
--eval $bypass_query
--disable_query_log
--cat_file $MYSQL_TMP_DIR/bypass_query_rows

SELECT variable_value INTO @bypass_rows_read_1 FROM
information_schema.global_status WHERE
variable_name="rocksdb_rows_read";
SELECT variable_value INTO @bypass_rows_sent_1 FROM
information_schema.global_status WHERE
variable_name="rows_sent";
SELECT variable_value INTO @bypass_executed_1 FROM
information_schema.global_status WHERE
variable_name="rocksdb_select_bypass_executed";

--let rows_read_bypass=`SELECT @bypass_rows_read_1 - @bypass_rows_read_0`
SELECT @bypass_rows_read_1 - @bypass_rows_read_0 AS ROWS_READ;

--let $assert_cond= @bypass_executed_1 - @bypass_executed_0 = 1
--let $assert_text = Verify executed in bypass
--source include/assert.inc

SELECT variable_value INTO @bypass_rows_read_2 FROM
information_schema.global_status WHERE
variable_name="rocksdb_rows_read";
SELECT variable_value INTO @bypass_rows_sent_2 FROM
information_schema.global_status WHERE
variable_name="rows_sent";
SELECT variable_value INTO @bypass_executed_2 FROM
information_schema.global_status WHERE
variable_name="rocksdb_select_bypass_executed";

--echo WITHOUT BYPASS:
SET GLOBAL rocksdb_select_bypass_policy=0;

--enable_query_log
--output $MYSQL_TMP_DIR/regular_query_rows
--eval $bypass_query
--disable_query_log
--cat_file $MYSQL_TMP_DIR/regular_query_rows

--diff_files $MYSQL_TMP_DIR/bypass_query_rows $MYSQL_TMP_DIR/regular_query_rows
--remove_file $MYSQL_TMP_DIR/bypass_query_rows
--remove_file $MYSQL_TMP_DIR/regular_query_rows

SELECT variable_value INTO @bypass_rows_read_3 FROM
information_schema.global_status WHERE
variable_name="rocksdb_rows_read";
SELECT variable_value INTO @bypass_rows_sent_3 FROM
information_schema.global_status WHERE
variable_name="rows_sent";
SELECT variable_value INTO @bypass_executed_3 FROM
information_schema.global_status WHERE
variable_name="rocksdb_select_bypass_executed";

--let $assert_cond= @bypass_executed_3 - @bypass_executed_2 = 0
--let $assert_text = Verify not executed in bypass
--source include/assert.inc

--let rows_read_no_bypass=`SELECT @bypass_rows_read_3 - @bypass_rows_read_2`
# This doesn't seem to be stable outside bypass
# SELECT @bypass_rows_read_3 - @bypass_rows_read_2 AS ROWS_READ;

SET GLOBAL rocksdb_select_bypass_policy=@__rocksdb_select_bypass_policy;

let $assert_cond= @bypass_rows_sent_3 - @bypass_rows_sent_2 =
@bypass_rows_sent_1 - @bypass_rows_sent_0;
--let $assert_text = Verify bypass and regular query return same number of rows
--source include/assert.inc

--let $assert_cond= $ROWS_READ_BYPASS <= $ROWS_READ_NO_BYPASS;
--let $assert_text = Verify bypass reads no more than regular query
--source include/assert.inc

--enable_query_log
Loading

0 comments on commit ef9a677

Please sign in to comment.