maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Michael!

On Mar 31, Michael Widenius wrote:
> Hi!
> 
> On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik <serg@xxxxxxxxxxx> wrote:
> >
> > Hi, Michael!
> >
> > On Mar 29, Michael Widenius wrote:
> > > revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82)
> > > parent(s): a194c5cfd41
> > > author: Michael Widenius <michael.widenius@xxxxxxxxx>
> > > committer: Michael Widenius <michael.widenius@xxxxxxxxx>
> > > timestamp: 2021-03-24 15:53:35 +0200
> > > message:
> > >
> > > Add support for minimum field width for strings to my_vsnprintf()
> > >
> > > diff --git a/mysql-test/main/almost_full.result b/mysql-test/main/almost_full.result
> > > index b2d7092aa51..4b5c8417412 100644
> > > --- a/mysql-test/main/almost_full.result
> > > +++ b/mysql-test/main/almost_full.result
> > > @@ -7,24 +7,24 @@ INSERT INTO t1 SET b=repeat('a',600);
> > >  ERROR HY000: The table 't1' is full
> > >  CHECK TABLE t1 EXTENDED;
> > >  Table        Op      Msg_type        Msg_text
> > > -test.t1      check   warning Datafile is almost full, 65448 of 65534 used
> > > +test.t1      check   warning Datafile is almost full,      65448 of      65534 used
> >
> > This happens because mi_check.c has
> >
> >    mi_check_print_warning(param, "Datafile is almost full, %10s of %10s used",
> >       ...
> 
> Yes, and that is exactly how I want it.  It is same output we have for
> aria_check and
> myisamchk and there it makes even more sense.

I know. It myisamchk and aria_check is makes not "more", it simply makes
sense. But in CHECK TABLE it does not make "less" sense, it makes no
sense and it ugly as hell. This is something you've introduced with your
patch, the width specification existed before, but it didn't do
anything. Your change caused this to appear and I ask to fix it.

I agree that it makes sense in command line tools, it's better to keep
it for command line tools, but not for the SQL interface. This can be
easily done with "%*d"

> > could you please remove explicit width specification from these
> > messages? this looks rather ugly in warnings. This applies both to
> > MyISAM and Aria.
> 
> No, see above.
> 
> This is however not related to the patch at all.  We did not support
> minimum width before and this is something that is sometimes needed.

Yes, it's caused by this patch, so very much related.

> > > +  if (length_arg < 0)
> >
> > So this code is dead, never used. See below.
> 
> It is used if someone (wrongly?) uses a negative argument.
> If the code would not be there, we would probably get
> a crash. Would you prefer that instead of ignoring it like now?

No, length_arg is never negative here, you can just add an assert. See
how it's called, the caller ensures that length_arg is always >= 0, the
sign is ignored.

> > > +  test1("Negative width is ignored for strings <   x> <    y>",
> > > +        "Negative width is ignored for strings <%-4s> <%-5s>", "x", "y");
> 
> I added the test to ensure that it this issue is documented and to
> ensure we do not get a crash when using negative numbers.
> I never claimed that left alignment should work with the patch.
> I can add to the commit message that this patch only supports right
> alignment of the string for now.

I know you never claimed that it works and that you added a test for it.
And it's very good that you have added a test!

But adding a check for something that can never happen (length_arg < 0)
looks rather unnecessary and confusing to me.

Regards,
Sergei
VP of MariaDB Server Engineering
and security@xxxxxxxxxxx