maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Aleksey!

On Jun 10, Aleksey Midenkov wrote:
> On Thu, Jun 3, 2021 at 1:58 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> > Hi, Aleksey!
> >
> > On Jan 26, Aleksey Midenkov wrote:
> > > On Tue, Dec 22, 2020 at 7:58 PM Sergei Golubchik <serg@xxxxxxxxxxx> 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@xxxxxxxxxxx