maria-developers team mailing list archive

[Aleksey Midenkov <midenok@xxxxxxxxx>]

Hi Sergei!

Now updated the patch to

git diff dfa2d0bc13..9b564832e3

On Wed, Jul 21, 2021 at 11:33 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
>
> Hi, Aleksey!
>
> As before, despite the email subject, it's
>
>   git diff dfa2d0bc13..e533b5fb307
>
> Don't be confused :)
>
> On Jul 21, Aleksey Midenkov wrote:
>
> > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > index c53732a2b51..70ed14448ea 100644
> > --- a/sql/ha_partition.cc
> > +++ b/sql/ha_partition.cc
> > @@ -5130,7 +5135,18 @@ bool ha_partition::init_record_priority_queue()
> >          i= bitmap_get_next_set(&m_part_info->read_partitions, i))
> >    {
> >      DBUG_PRINT("info", ("init rec-buf for part %u", i));
> > -    int2store(ptr, i);
> > +    if (table->s->blob_fields)
> > +    {
> > +      for (uint j= 0; j < table->s->blob_fields; ++j, ++objs)
> > +      {
> > +        blob_storage[j]= new (objs) Ordered_blob_storage;
> > +        if (!blob_storage[j])
> > +          DBUG_RETURN(true);
>
> is this possible?
>

If we don't use exceptions we should handle OOM this way. If we can't
use -fno-exceptions we should make a wrapper by overloading default
new() and catching std::badalloc.

> > +      }
> > +      *((Ordered_blob_storage ***) ptr)= blob_storage;
> > +      blob_storage+= table->s->blob_fields;
> > +    }
> > +    int2store(ptr + sizeof(String **), i);
> >      ptr+= m_priority_queue_rec_len;
> >    }
> >    m_start_key.key= (const uchar*)ptr;
> > @@ -6291,6 +6333,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found()
> >  }
> >
> >
> > +void ha_partition::swap_blobs(uchar * rec_buf, Ordered_blob_storage ** storage, bool restore)
> > +{
> > +  uint *ptr, *end;
> > +  uint blob_n= 0;
> > +  table->move_fields(table->field, rec_buf, table->record[0]);
> > +  for (ptr= table->s->blob_field, end= ptr + table->s->blob_fields;
> > +       ptr != end; ++ptr, ++blob_n)
> > +  {
> > +    DBUG_ASSERT(*ptr < table->s->fields);
> > +    Field_blob *blob= (Field_blob*) table->field[*ptr];
> > +    DBUG_ASSERT(blob->flags & BLOB_FLAG);
> > +    DBUG_ASSERT(blob->field_index == *ptr);
> > +    if (!bitmap_is_set(table->read_set, *ptr) || blob->is_null())
> > +      continue;
> > +
> > +    Ordered_blob_storage &s= *storage[blob_n];
> > +
> > +    if (restore)
> > +    {
> > +      if (!s.blob.is_empty())
> > +        blob->swap(s.blob, s.set_read_value);
>
> don't you need here something like
>
>       else
>         blob->reset();
>
> to make blob empty?

I believe no. We protect only blob cache (value or read_value). If the
cache was empty that doesn't mean the blob was empty. AFAIR blobs
allocated by a storage engine should work just fine. Added a comment
about that.

>
> > +    }
> > +    else
> > +    {
> > +      bool set_read_value;
> > +      String *cached= blob->cached(set_read_value);
> > +      if (cached)
> > +      {
> > +        cached->swap(s.blob);
> > +        s.set_read_value= set_read_value;
> > +      }
> > +    }
> > +  }
> > +  table->move_fields(table->field, table->record[0], rec_buf);
>
> On swap_blobs() you always move all fields from rec_buf to table->record[0]?
> Why?
> This looks very suspicious.
>

Have you seen the first move_fields() call? We work on rec_buf and if
it is the same as record[0] move_fields() does nothing.

> > +}
> > +
> > +
> >  /*
> >    Common routine to handle index_next with ordered results
> >
>
> Regards,
> Sergei
> VP of MariaDB Server Engineering
> and security@xxxxxxxxxxx



-- 
All the best,

Aleksey Midenkov
@midenok