Hi, Aleksey! On Jun 10, Aleksey Midenkov wrote:
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?
because, of course, we've never had a consistent written policy that everyone would've followed. And because Monty tends to revert new commits, not 17yr old ones.
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?
Oh, right. Your swap() is just calling String::swap(), so it copies its prototype. Makes sense. Okay, let's keep it like String::swap() Still, your String * cached(bool &set_read_value) isn't "inheriting" the prototype from anything, better use a pointer there.
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.
Great, so no new test is needed.
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.
Generally, yes, it's *hugely* helpful to look at the line and see why it was added. For example, this commit: https://github.com/MariaDB/server/commit/aba91154264 Here it's very clear what's happening, one doesn't need even to read the commit comment or the test. As a counter example, MySQL commit (they have a policy for doing that, as far as I understand, our tree doesn't have such monsters): https://github.com/mysql/mysql-server/commit/67c3c70e489 447 files changed, 28239 insertions(+), 21952 deletions(-) many are tests, but in sql alone: 126 files changed, 8975 insertions(+), 6318 deletions(-) But in this particular case of your commit. If both changes, indeed, make no sense without each other, then they could be in the same commit.
> @@ -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:
May be it isn't a as low-hanging as it looked, indeed. Let's get your fix first, then I'll check again. Just don't like to different approaches to essentially the same problem Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org