Hi Sergei! On Thu, Jun 3, 2021 at 1:58 PM Sergei Golubchik <serg@mariadb.org> wrote:
Hi, Aleksey!
On Jan 26, Aleksey Midenkov wrote:
On Tue, Dec 22, 2020 at 7:58 PM Sergei Golubchik <serg@mariadb.org> 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 &s) 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 <serg@mariadb.org> 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(&m_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(&m_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? &m_start_key: NULL, end_range, eq_range, TRUE); - memcpy(rec_buf_ptr, table->record[0], m_rec_length); + if (!error) + { + memcpy(rec_buf_ptr, table->record[0], m_rec_length); + }
did you have a bug because of that? something didn't work?
No, I just saw worthless memcpy and avoided it.
You're right that memcopy is unnecessary in case of an error. But most of the time there is no error. You made an exceptional code path faster by making a common code path slower. I think it's generally better to do the opposite, making the common, no-error code path faster, fewer conditionals, in particular. At the cost of making the error path slower, if needed.
I'm usually aware of such things in my decisions. The slowdown is close to 0, it is not even 0.0001%, it is much lower. OTOH memcpy() slowdown is much higher and this is not a fully exceptional code path: HA_ERR_KEY_NOT_FOUND continues the loop.
This is a wrong code pattern, you should optimize for the normal code path, not for errors. Errors are exceptions, they're much more rare than non-errors.
Okay. Looks like no sense to argue that anymore. Reverted.
@@ -6310,6 +6349,43 @@ int ha_partition::handle_ordered_index_scan_key_not_found() + if (cached) + { + cached->swap(s.blob); + s.set_read_value= set_read_value;
is it indeed possible here for a value to be either in `value` or in `read_value` ?
When happens what?
Yes, it uses `value` for plain fields and `read_value` for virtual fields.
Oh, I see.
This read_value was introduced in ea1b25046c81db8fdf to solve the case of UPDATE, where a row is moved with store_record(table, record[1]) and blob pointers aren't updated. This is very similar to what you're fixing, perhaps read_value should go away and both bugs should have a one unified solution that allows to "park" a record somewhere out of record[0] and restore later, all with proper blob handling?
Do you think this is necessary now? I believe there are a lot of *much* more important tasks.
Uhm, yes? This would be a notable code simplification, and it won't take long, a low-hanging fruit.
I'm not sure if this is a "fruit". Can you please elaborate the change? Use so-called Ordered_blob_storage instead of read_value and do swap_blobs() on store_record()? And have 2 arrays of Ordered_blob_storage for each TABLE::record like that: diff --git a/sql/table.h b/sql/table.h index 69bd14b2834..40042dc1df0 100644 --- a/sql/table.h +++ b/sql/table.h @@ -1151,6 +1151,8 @@ struct TABLE THD *in_use; /* Which thread uses this */ uchar *record[2]; /* Pointer to records */ + Ordered_blob_storage **blob_storage[2]; /* NULL-terminated arrays of pointers + to blob-storages */ uchar *write_row_record; /* Used as optimisation in THD::write_row */ uchar *insert_values; /* used by INSERT ... UPDATE */
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
-- All the best, Aleksey Midenkov @midenok