Hi Sergei!
On Thu, Jun 3, 2021 at 1:58 PM Sergei Golubchik wrote:
>
> Hi, Aleksey!
>
> On Jan 26, Aleksey Midenkov wrote:
> > On Tue, Dec 22, 2020 at 7:58 PM Sergei Golubchik wrote:
> > > On Nov 03, Aleksey Midenkov wrote:
> > >
> > > And please, pretty please, don't use non-constant references as
> > > function arguments.
> >
> > Why? With pointers we can pass NULL and we don't need NULL there, do we?
>
> Because this is the policy. If you convince Monty that non-constant
> references are fine - feel free to use them.
If this is the policy, why there is
void String::swap(String )
since 2004? Are you asking me to pass the pointer and then dereference
it and pass it as a reference into String class? That would look weird
to me. I'm going to agree if you insist, but really?
>
> > > > On Mon, Nov 2, 2020 at 11:33 PM Sergei Golubchik
> > > > wrote:
> > > > >
> > > > > > diff --git a/sql/field.cc b/sql/field.cc
> > > > > > index bdaaecc20269..0fd40c979d2c 100644
> > > > > > --- a/sql/field.cc
> > > > > > +++ b/sql/field.cc
> > > > > > @@ -8310,6 +8310,7 @@ int Field_blob::store(const char *from,uint
> > > > > > length,CHARSET_INFO *cs)
> > > > > >copy_length= copier.well_formed_copy(field_charset,
> > > > > > (char*) value.ptr(),
> > > > > > new_length,
> > > > > > cs, from, length);
> > > > > > + value.length(copy_length);
> > > > >
> > > > > good! Could this be a distinct bug with its own test case?
> > > >
> > > > Do we need to spend time on that bureaucracy? There are real problems to
> > > > solve... Tickets drain time from the management people and generally
> > > > from
> > > > anyone who sees them.
> > >
> > > I didn't mean opening an MDEV. Only writing a test case and extracting
> > > it into a separate commit.
> > >
> > > If this is a genuine bug fix and you don't create a test case - how can
> > > you be sure the fix will stay? It may disappear in a merge or a rebase,
> > > manually or automatically. Or in a refactoring that "looks fine, all
> > > tests pass". If you can create a test case - please, do.
> >
> > It is tested by federated.federated_partition test case.
>
> Do you mean, that if I remove this line, federated.federated_partition
> test will fail? If yes - good, this is the test I meant, no need to
> create another one.
Yes, the test will fail.
>
> > Why do we have to extract this side-fix to a separate commit?
>
> Because later when someone looks at the code it helps to be able to
> understand what a commit is actually fixing.
As long as both fixes are important and there is no sense to apply one
without the other I see separate commits as a measure to satisfy
someone's curiosity which I do not share (and there is nothing to be
curious about that one-liner). Though I'm going to agree if you ask me
to do that.
>
> > > > > > diff --git a/sql/ha_partition.cc b/sql/ha_partition.cc
> > > > > > --- a/sql/ha_partition.cc
> > > > > > +++ b/sql/ha_partition.cc
> > > > > > @@ -5125,14 +5128,14 @@ bool
> > > > > > ha_partition::init_record_priority_queue()
> > > > > >uint alloc_len;
> > > > > >uint used_parts= bitmap_bits_set(_part_info->read_partitions);
> > > > > >/* Allocate record buffer for each used partition. */
> > > > > > - m_priority_queue_rec_len= m_rec_length + PARTITION_BYTES_IN_POS;
> > > > > > + m_priority_queue_rec_len= m_rec_length + ORDERED_REC_OFFSET;
> > > > > >if (!m_using_extended_keys)
> > > > > >m_priority_queue_rec_len += m_file[0]->ref_length;
> > > > > >alloc_len= used_parts * m_priority_queue_rec_len;
> > > > > >/* Allocate a key for temporary use when setting up the scan. */
> > > > > >alloc_len+= table_share->max_key_length;
> > > > > >
> > > > > > - if (!(m_ordered_rec_buffer= (uchar*)my_malloc(alloc_len,
> > > > > > MYF(MY_WME
> > > > > > + if (!(m_ordered_rec_buffer= (uchar*) alloc_root(_ordered_root,
> > > > > > alloc_len)))
> > > > >
> > > > > I don't see why you need a memroot here. memroot is needed when
> > > > > you allocate later at difeerent points in time some initially
> > > > > unpredictable amount of memory that has the same lifetime and
> > > > > needs to be freed all at once.
> > > >
> > One should keep in mind this is an artificial usage of the prio queue.
> > In diversified real-world usage the share of such kind SELECT is say
> > 0.1%. But even if it is 1% we divide our stats by 100. So, do you
> > really care about 0.0015%?
>
> A right tool for a job. If you need to allocate few buffers at the same
> time - use multi_alloc, if you do many allocations over time and want to
> fee them at once - use memroot.
Changed to my_multi_alloc().
>
> > > > > > @@ -6178,7 +6203,11 @@ int
> > > > > > ha_partition::handle_ordered_index_scan(uchar *buf, bool
> > > > > > reverse_order)
> > > > > >*/
> > > > > >error= file->read_range_first(m_start_key.key? _start_key:
> > > > > > NULL,
> > > > > >
