maria-developers team mailing list archive

[Nikita Malyavin <nikitamalyavin@xxxxxxxxx>]

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@xxxxxxxxxxx> 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

> 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!

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@xxxxxxxxxxx
>


-- 
Yours truly,
Nikita Malyavin