From 5dbd7d095a266e85f8ae6b16b1e9c4baee9b08d8 Mon Sep 17 00:00:00 2001 From: Sergei Petrunia Date: Wed, 3 Jun 2015 10:48:35 -0700 Subject: [PATCH] Correctly handle indexes on column prefixes Summary: The code used table->field[n] when packing/unpacking index values. This happens to work most of the time, however the right way to do it is to use table->key_info[i].key_part[j], which has a field object that describes the field use in the index. Differential Revision: https://reviews.facebook.net/D39471 --- storage/rocksdb/ha_rocksdb.cc | 4 ++-- storage/rocksdb/rdb_datadic.cc | 29 +++++++++++++++++++++-------- storage/rocksdb/rdb_datadic.h | 32 ++++++++++++++++++++++++++++---- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/storage/rocksdb/ha_rocksdb.cc b/storage/rocksdb/ha_rocksdb.cc index 66ed4a9647f3..4673e4c60895 100644 --- a/storage/rocksdb/ha_rocksdb.cc +++ b/storage/rocksdb/ha_rocksdb.cc @@ -2735,7 +2735,7 @@ bool ha_rocksdb::check_keyread_allowed(uint inx, uint part, bool all_parts) cons KEY *key_info= &table_share->key_info[inx]; Field_pack_info dummy1; - res= dummy1.setup(key_info->key_part[part].field); + res= dummy1.setup(key_info->key_part[part].field, inx, part); if (res && all_parts) { @@ -2745,7 +2745,7 @@ bool ha_rocksdb::check_keyread_allowed(uint inx, uint part, bool all_parts) cons if ((field= key_info->key_part[i].field)) { Field_pack_info dummy; - if (!dummy.setup(field)) + if (!dummy.setup(field, inx, i)) { /* Cannot do index-only reads for this column */ res= false; diff --git a/storage/rocksdb/rdb_datadic.cc b/storage/rocksdb/rdb_datadic.cc index aef15e8fdb6e..bfa2798aa342 100644 --- a/storage/rocksdb/rdb_datadic.cc +++ b/storage/rocksdb/rdb_datadic.cc @@ -204,8 +204,11 @@ void RDBSE_KEYDEF::setup(TABLE *tbl) int max_part_len= 0; bool simulating_extkey= false; uint dst_i= 0; + + uint keyno_to_set= keyno; + uint keypart_to_set= 0; /* this loop also loops over the 'extended key' tail */ - for (uint src_i= 0; src_i < m_key_parts; src_i++) + for (uint src_i= 0; src_i < m_key_parts; src_i++, keypart_to_set++) { Field *field= key_part->field; @@ -232,7 +235,7 @@ void RDBSE_KEYDEF::setup(TABLE *tbl) if (field->real_maybe_null()) max_len +=1; // NULL-byte - pack_info[dst_i].setup(field); + pack_info[dst_i].setup(field, keyno_to_set, keypart_to_set); pack_info[dst_i].unpack_data_offset= unpack_len; if (pk_info) @@ -258,6 +261,8 @@ void RDBSE_KEYDEF::setup(TABLE *tbl) if (secondary_key && src_i+1 == key_info->actual_key_parts) { simulating_extkey= true; + keyno_to_set= tbl->s->primary_key; + keypart_to_set= (uint)-1; key_part= pk_info->key_part; } @@ -343,8 +348,9 @@ uint RDBSE_KEYDEF::get_primary_key_tuple(TABLE *table, if (have_value) { - Field *field= table->field[pack_info[i].fieldnr]; - if (pack_info[i].skip_func(&pack_info[i], field, &reader)) + Field_pack_info *fpi= &pack_info[i]; + Field *field= fpi->get_field_in_table(table); + if (fpi->skip_func(fpi, field, &reader)) return INVALID_LEN; } @@ -460,7 +466,7 @@ uint RDBSE_KEYDEF::pack_record(TABLE *tbl, for (uint i=0; i < n_key_parts; i++) { - Field *field= tbl->field[pack_info[i].fieldnr]; + Field *field= pack_info[i].get_field_in_table(tbl); // Old Field methods expected the record pointer to be at tbl->record[0]. // The quick and easy way to fix this was to pass along the offset @@ -629,7 +635,7 @@ int RDBSE_KEYDEF::unpack_record(TABLE *table, uchar *buf, for (uint i= 0; i < m_key_parts ; i++) { Field_pack_info *fpi= &pack_info[i]; - Field *field= table->field[pack_info[i].fieldnr]; + Field *field= fpi->get_field_in_table(table); if (fpi->unpack_func) { @@ -1016,12 +1022,14 @@ int skip_variable_length(Field_pack_info *fpi, Field *field, Stream_reader *read FALSE - Otherwise */ -bool Field_pack_info::setup(Field *field) +bool Field_pack_info::setup(Field *field, uint keynr_arg, uint key_part_arg) { int res= false; enum_field_types type= field->real_type(); - fieldnr= field->field_index; + keynr= keynr_arg; + key_part= key_part_arg; + maybe_null= field->real_maybe_null(); make_unpack_info_func= NULL; unpack_func= NULL; @@ -1107,6 +1115,11 @@ bool Field_pack_info::setup(Field *field) } +Field *Field_pack_info::get_field_in_table(TABLE *tbl) +{ + return tbl->key_info[keynr].key_part[key_part].field; +} + #if 0 void _rdbse_store_blob_length(uchar *pos,uint pack_length,uint length) { diff --git a/storage/rocksdb/rdb_datadic.h b/storage/rocksdb/rdb_datadic.h index 6e4c06933ad3..ad15bf504aa8 100644 --- a/storage/rocksdb/rdb_datadic.h +++ b/storage/rocksdb/rdb_datadic.h @@ -440,10 +440,34 @@ class Field_pack_info */ index_field_skip_t skip_func; - /* Number of field in the table */ - int fieldnr; - - bool setup(Field *field); +private: + /* + Location of the field in the table (key number and key part number). + + Note that this describes not the field, but rather a position of field in + the index. Consider an example: + + col1 VARCHAR (100), + INDEX idx1 (col1)), + INDEX idx2 (col1(10)), + + Here, idx2 has a special Field object that is set to describe a 10-char + prefix of col1. + + We must also store the keynr. It is needed for implicit "extended keys". + Every key in MyRocks needs to include PK columns. Generally, SQL layer + includes PK columns as part of its "Extended Keys" feature, but sometimes + it does not (known examples are unique secondary indexes and partitioned + tables). + In that case, MyRocks's index descriptor has invisible suffix of PK + columns (and the point is that these columns are parts of PK, not parts + of the current index). + */ + uint keynr; + uint key_part; +public: + bool setup(Field *field, uint keynr_arg, uint key_part_arg); + Field *get_field_in_table(TABLE *tbl); };