First commit is reworded, and new changes are added on top.
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
> diff --git a/sql/share/errmsg-utf8.txt b/sql/share/errmsg-utf8.txt
> 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 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!
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