Hello! Thanks for the review First commit is reworded, and new changes are added on top. Also, I am reverting "handler: remove store/restore record from check_duplicate_long_entry_key", where I was trying to reflect handler->position fix to unique blobs. The reason is that main.long_unique_bugs test fails, and I did not find a quick solution for that :( Please, see my answers below: On Tue, 17 Dec 2019 at 21:14, Sergei Golubchik <serg@mariadb.org> wrote:
wrap long lines, pease.
okay, reworded the message
diff --git a/sql/handler.cc b/sql/handler.cc index 46a0c313c80..ed36f3c5bd3 100644 --- a/sql/handler.cc +++ b/sql/handler.cc @@ -6402,6 +6407,14 @@ int handler::ha_external_lock(THD *thd, int lock_type) mysql_audit_external_lock(thd, table_share, lock_type); }
+ if (lock_type == F_UNLCK && check_overlaps_handler) + { + check_overlaps_handler->ha_external_lock(table->in_use, F_UNLCK); + check_overlaps_handler->close(); + check_overlaps_handler= NULL; + overlaps_error_key= -1; + }
I'm still thinking about how avoid this overhead or at least to simplify the code.
One option is to use HA_EXTRA_REMEMBER_POS It doesn't nest, you're right, but it can be fixed.
Another, simpler, option is to use TABLE::update_handler. This is a second auxillary handler, a clone, exactly as yours, created for to check long uniques. So I don't see a need to create a yet another clone when you can simply use TABLE::update_handler. It's never used for scanning, only for point lookups, so there is no position that you can disrupt.
upd: you seem to have got the same idea. and you're right, it should be in the handler class, not in the TABLE as I originally wanted.
We operate with top-level handlers only, for pertitioning. In this sense TABLE is better -- but it is a bad composition. This handler is only used in other handler, and never used in TABLE.
I have moved handler cloning code into handler class, and renamed the field to `lookup_handler`, but didn't touch the memory buffer. We use different sizes there (I need additional memory for key storage). Maybe it's better to make a common buffer as well, and allocate a maximum, i.e. reclength+keylength? Please, add a comment that on INSERT handler->inited can be NONE
+ if (handler->inited != NONE)
+ {
added
+ // Save and later restore this handler's keyread
+ int old_this_keyread= this->keyread;
what's that for? you aren't using `this` anywhere below. Unless handler == this, but then this->keyread cannot be enabled.
+ DBUG_ASSERT(this->ha_end_keyread() == 0);
Eh. Never put any code with side effects into an assert. Assert are conditionally compiled.
oops, extracted
+ auto old_row_found= [is_update, old_data, record_buffer, this, handler](){
There is no reason to use a lambda here. could you rewrite that, please? In this particular case old_row_found() is only used once, so you can inline your lambda there.
agree, that's my laziness stopped me to do it on my own🙂
+ DBUG_ASSERT(handler != this && inited != NONE);
as a general rule please use
DBUG_ASSERT(x); DBUG_ASSERT(y);
and not
DBUG_ASSERT(x && y);
✔️
+ /* Copy period_end to period_start. + * the value in period_end field is not significant, but anyway let's leave + * it defined to avoid uninitialized memory access + */
please format your multi-line comments to follow the existing server code conventions
Sorry, mixing that up every time. Reformatted.
a comment please. What does the function return? -1/0/1 ?
+int key_period_compare_bases(const KEY &lhs_key, const KEY &rhs_key, + const uchar *lhs, const uchar *rhs)
Have documented all that functions
+ bool overlaps = (cmp[0][0] <= 0 && cmp[1][0] > 0)
+ || (cmp[0][0] >= 0 && cmp[0][1] < 0); + return overlaps ? 0 : cmp[0][0];
Couldn't this be simplifed? Like
if (cmp[1][0] <= 0) // r1 <= l2 return -1; if (cmp[0][1] >= 0) // l1 >= r2 return 1; return 0;
and I think it'd be clearer to remove this cmp[][] array and write the condition directly. May be with shortcuts like
const Field const * f=lhs_fields[0]; const uchar const * l1=lhs_fields[0]->ptr_in_record(lhs), ...
if (f->cmp(r1, l2) <= 0) return -1; if (f->cmp(l1, r2) >= 0) return 1; return 0;
Yes, it's now not so self-documentary (the idea was to have `overlaps` variable), but looks more clean
index 36188e58624..bb2f0fc5296 100644 --- a/sql/share/errmsg-utf8.txt +++ b/sql/share/errmsg-utf8.txt @@ -7943,3 +7943,9 @@ ER_WARN_HISTORY_ROW_START_TIME eng "Table `%s.%s` history row start '%s' is later than row end '%s'" ER_PART_STARTS_BEYOND_INTERVAL eng "%`s: STARTS is later than query time, first history
diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt partition may exceed INTERVAL value"
+ER_KEY_CONTAINS_PERIOD_FIELDS + eng "Key %`s should not contain period fields in order to make it WITHOUT OVERLAPS"
I'd say "Key %`s cannot explicitly include column %`s" or "WITHOUT OVERLAPS key %`s cannot explicitly include column %`s" but I think the latter looks weird, I like the first variant more
+ER_PERIOD_WITHOUT_OVERLAPS_PARTITIONED + eng "Period WITHOUT OVERLAPS is not implemented for partitioned tables"
I don't think we should create a separate error message for every feature that doesn't work with partitioning. There's already
ER_FOREIGN_KEY_ON_PARTITIONED eng "Foreign key clause is not yet supported in conjunction with partitioning" ER_PARTITION_NO_TEMPORARY eng "Cannot create temporary table with partitions" ER_FULLTEXT_NOT_SUPPORTED_WITH_PARTITIONING eng "FULLTEXT index is not supported for partitioned tables"
which is two error messages too many. So I'd just say
ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING end "Partitioned tables do not support %s"
where %s could be CREATE TEMPORARY TABLE, FOREIGN KEY, FULLTEXT, WITHOUT OVERLAPS, and whatever else partitioned tables don't or won't support.
Note that you cannot remove old error messages, so just rename the first one to ER_FEATURE_NOT_SUPPORTED_WITH_PARTITIONING, and keep the rest unused but still present in the errmsg-utf8.txt file.
+ER_PERIOD_WITHOUT_OVERLAPS_NON_UNIQUE + eng "Period WITHOUT OVERLAPS is only allowed for unique keys"
"Only UNIQUE or PRIMARY keys can be WITHOUT OVERLAPS"
I see. One of them was already never used, and all the rest is done.
diff --git a/sql/table.cc b/sql/table.cc
index 7ed5121a9c6..8b84fb3035d 100644 --- a/sql/table.cc +++ b/sql/table.cc @@ -1519,6 +1520,15 @@ bool read_extra2(const uchar *frm_image, size_t len, extra2_fields *fields) size_t length= extra2_read_len(&extra2, e2end); if (!length) DBUG_RETURN(true); + + auto fill_extra2= [extra2, length](LEX_CUSTRING *section){ + if (section->str) + return true; + *section= {extra2, length}; + return false; + };
don't use a lambda here either, make it a proper function
I think it is much prettier with lambda, and does not worth to garbage the unit namespace, but I can live with that in this case. Still I think with time you should reconsider your thoughts about them, especially minding the case below:
@@ -1726,11 +1725,26 @@ int TABLE_SHARE::init_from_binary_frm_image(THD *thd, bool write,
keyinfo= &first_keyinfo; thd->mem_root= &share->mem_root;
+ auto err= [thd, share, &handler_file, &se_plugin, old_root](){ + share->db_plugin= NULL; + share->error= OPEN_FRM_CORRUPTED; + share->open_errno= my_errno; + delete handler_file; + plugin_unlock(0, se_plugin); + my_hash_free(&share->name_hash); + + if (!thd->is_error()) + open_table_error(share, OPEN_FRM_CORRUPTED, share->open_errno); + + thd->mem_root= old_root; + return HA_ERR_NOT_A_TABLE; + };
Revert that too
I have a problem that all the time i'm debugging corrupted frm's, or anything else with lots of goto err's, the second thing I am doing (the first one is I'm breaking on err: label and making sure that no telepathy today i have) is rewriting goto's to function calls, to have an additional stack frame, to see where is err() called from. This is why I want this patch so much -- because I'm suffering all the time I face this kind of problem. Same to REPLACE code (see write_row), and other places with over 9K gotos. We can avoid lambda here by making it a separate function with 5 arguments and call it table_share_init_from_binary_frm_image_err, but then each time the some new code is added to that function, the additional arguments are (most likely) added, and all the calling places should be altered. Then we can make arguments as a structure, table_share_init_from_binary_frm_image_err_agrs, and all the arguments will be placed there. No callers altering, but now it means that every error handler will add two more entities for module scope, although they both are used only in one function! Then, we can make it an object of a class Table_share_init_from_binary_frm_image_err_context, then it'll be only one entity, and class definitions can be placed inside functions, but the latter could look confusing, and lambda does *literally* same! It's even binary compatible. And does not look confusing. Am good with any of these solutions, but I need one. Which one would you choose?
+ if ((period.unique_keys + 1) * frm_keyno_size
+ != extra2.without_overlaps.length) + DBUG_RETURN(err());
you can also check here that extra2.application_period.str != NULL otherwise it's OPEN_FRM_CORRUPTED too
Yes, you're right. Added that
+ /* + @return -1, lhs precedes rhs + 0, lhs overlaps rhs + 1, lhs succeeds rhs + */
ah, a comment, good :) but move it to the function definition, please
Okey. Still do not understand, why not in headers
+ static int check_period_overlaps(const KEY &lhs_key, const KEY &rhs_key, + const uchar *lhs, const uchar *rhs); int delete_row(); void vers_update_fields(); void vers_update_end(); @@ -1759,10 +1767,18 @@ class IS_table_read_plan;
/** number of bytes used by field positional indexes in frm */ constexpr uint frm_fieldno_size= 2; +/** number of bytes used by key position number in frm */ +constexpr uint frm_keyno_size= 2; static inline uint16 read_frm_fieldno(const uchar *data) { return uint2korr(data); } -static inline void store_frm_fieldno(const uchar *data, uint16 fieldno) +static inline void store_frm_fieldno(uchar *data, uint16 fieldno) +{ int2store(data, fieldno); } +static inline uint16 read_frm_keyno(const uchar *data) +{ return uint2korr(data); } +static inline void store_frm_keyno(uchar *data, uint16 fieldno) { int2store(data, fieldno); } +static inline size_t extra2_str_size(size_t len) +{ return (len > 255 ? 3 : 1) + len; }
why did you move that? it's still not used anywhere outside of unireg.cc
Yes, one frm corruption check from extra2.application_period was missing from initial periods commit. Added it back.
diff --git a/sql/unireg.cc b/sql/unireg.cc index 7130b3e5d8a..ea5d739a97e 100644 --- a/sql/unireg.cc +++ b/sql/unireg.cc @@ -390,7 +385,9 @@ LEX_CUSTRING build_frm_image(THD *thd, const LEX_CSTRING &table,
if (create_info->period_info.name) { - extra2_size+= 1 + extra2_str_size(period_info_len); + // two extra2 sections are taken after 10.5
This is a confusing comment, it suggests that both extra2 sections were added in 10.5. Remove the comment, please, it's not worth it.
okay
diff --git a/sql/unireg.h b/sql/unireg.h index 419fbc4bd80..873d6f681fc 100644 --- a/sql/unireg.h +++ b/sql/unireg.h @@ -177,7 +177,8 @@ enum extra2_frm_value_type {
EXTRA2_ENGINE_TABLEOPTS=128, EXTRA2_FIELD_FLAGS=129, - EXTRA2_FIELD_DATA_TYPE_INFO=130 + EXTRA2_FIELD_DATA_TYPE_INFO=130, + EXTRA2_PERIOD_WITHOUT_OVERLAPS=131,
Please, try to create a table that uses WITHOUT OVERLAPS and open it in 10.4. Just as a test, to make sure it works as expected.
No it doesn't open. Is it the behavior you expect?
Regards,
Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- Yours truly, Nikita Malyavin