Re: [Maria-developers] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()
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@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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", ... could you please remove explicit width specification from these messages? this looks rather ugly in warnings. This applies both to MyISAM and Aria.
test.t1 check status OK UPDATE t1 SET b=repeat('a', 800) where a=10; ERROR HY000: The table 't1' is full diff --git a/mysql-test/main/grant4.result b/mysql-test/main/grant4.result index 981bbdeddf5..ba64010f1bb 100644 --- a/mysql-test/main/grant4.result +++ b/mysql-test/main/grant4.result @@ -187,8 +187,8 @@ connection con1; check table mysqltest_db1.t1; Table Op Msg_type Msg_text mysqltest_db1.t1 check warning 1 client is using or hasn't closed the table properly -mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 -mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7 +mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 +mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7
Same here.
mysqltest_db1.t1 check error Corrupt # The below statement should fail before repairing t1. # Otherwise info about such repair will be missing from its result-set. diff --git a/mysql-test/main/myisam_recover.result b/mysql-test/main/myisam_recover.result index da96682186c..d3c385bc49b 100644 --- a/mysql-test/main/myisam_recover.result +++ b/mysql-test/main/myisam_recover.result @@ -69,9 +69,9 @@ flush table t1; # check table is needed to mark the table as crashed. check table t1; Table Op Msg_type Msg_text -test.t1 check warning Size of datafile is: 42 Should be: 21 -test.t1 check error Record-count is not ok; is 6 Should be: 3 -test.t1 check warning Found 6 key parts. Should be: 3 +test.t1 check warning Size of datafile is: 42 Should be: 21 +test.t1 check error Record-count is not ok; is 6 Should be: 3 +test.t1 check warning Found 6 key parts. Should be: 3
Same. Must be in many messages in mi_check.c and ma_check.c
test.t1 check error Corrupt # diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c index a2e3f9b738d..1621db2e412 100644 --- a/strings/my_vsnprintf.c +++ b/strings/my_vsnprintf.c @@ -224,12 +224,27 @@ static char *backtick_string(CHARSET_INFO *cs, char *to, const char *end, */
static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end, - size_t width, char *par, uint print_type, - my_bool nice_cut) + longlong length_arg, size_t width, char *par, + uint print_type, my_bool nice_cut) { int well_formed_error; uint dots= 0; size_t plen, left_len= (size_t) (end - to) + 1, slen=0; + my_bool left_fill= 1; + size_t length; + + /* + The sign of the length argument specific the string should be right + or left adjusted + */ + if (length_arg < 0)
You're trying to support left alignment, but you have yourself added a test that shows that it doesn't work. So this code is dead, never used. See below.
+ { + length= -length_arg; + left_fill= 0; + } + else + length= (size_t) length_arg; + if (!par) par = (char*) "(null)";
@@ -693,7 +720,8 @@ size_t my_vsnprintf_ex(CHARSET_INFO *cs, char *to, size_t n, if (*fmt == 's' || *fmt == 'T') /* String parameter */ { reg2 char *par= va_arg(ap, char *); - to= process_str_arg(cs, to, end, width, par, print_type, (*fmt == 'T')); + to= process_str_arg(cs, to, end, (longlong) length, width, par,
see? length is unsigned type size_t, the sign before length is ignored by the parser. Whether you cast length to a signed type or not and whether you checks for length < 0 or not, left alignment still won't work. That's why you have added this test:
+ test1("Negative width is ignored for strings < x> < y>", + "Negative width is ignored for strings <%-4s> <%-5s>", "x", "y");
Regards, Sergei VP of MariaDB Server Engineering and security@mariadb.org
Hi! On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik <serg@mariadb.org> 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@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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.
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.
test.t1 check status OK UPDATE t1 SET b=repeat('a', 800) where a=10; ERROR HY000: The table 't1' is full diff --git a/mysql-test/main/grant4.result b/mysql-test/main/grant4.result index 981bbdeddf5..ba64010f1bb 100644 --- a/mysql-test/main/grant4.result +++ b/mysql-test/main/grant4.result @@ -187,8 +187,8 @@ connection con1; check table mysqltest_db1.t1; Table Op Msg_type Msg_text mysqltest_db1.t1 check warning 1 client is using or hasn't closed the table properly -mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 -mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7 +mysqltest_db1.t1 check error Size of indexfile is: 1024 Should be: 2048 +mysqltest_db1.t1 check warning Size of datafile is: 14 Should be: 7
Same here.
It is exactly how it i want it. One can get +100 of these errors and it is hopeless to read them if things are not aligned. <cut>
diff --git a/strings/my_vsnprintf.c b/strings/my_vsnprintf.c index a2e3f9b738d..1621db2e412 100644 --- a/strings/my_vsnprintf.c +++ b/strings/my_vsnprintf.c @@ -224,12 +224,27 @@ static char *backtick_string(CHARSET_INFO *cs, char *to, const char *end, */
static char *process_str_arg(CHARSET_INFO *cs, char *to, const char *end, - size_t width, char *par, uint print_type, - my_bool nice_cut) + longlong length_arg, size_t width, char *par, + uint print_type, my_bool nice_cut) { int well_formed_error; uint dots= 0; size_t plen, left_len= (size_t) (end - to) + 1, slen=0; + my_bool left_fill= 1; + size_t length; + + /* + The sign of the length argument specific the string should be right + or left adjusted + */ + if (length_arg < 0)
You're trying to support left alignment, but you have yourself added a test that shows that it doesn't work.
Yes, as I did not need left alignment and if someone wants to add it, they are welcome to do that.
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?
+ { + length= -length_arg; + left_fill= 0; + } + else + length= (size_t) length_arg; + if (!par) par = (char*) "(null)";
@@ -693,7 +720,8 @@ size_t my_vsnprintf_ex(CHARSET_INFO *cs, char *to, size_t n, if (*fmt == 's' || *fmt == 'T') /* String parameter */ { reg2 char *par= va_arg(ap, char *); - to= process_str_arg(cs, to, end, width, par, print_type, (*fmt == 'T')); + to= process_str_arg(cs, to, end, (longlong) length, width, par,
see? length is unsigned type size_t, the sign before length is ignored by the parser. Whether you cast length to a signed type or not and whether you checks for length < 0 or not, left alignment still won't work. That's why you have added this test:
+ 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. Regards, Monty
Hi, Michael! On Mar 31, Michael Widenius wrote:
Hi!
On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik <serg@mariadb.org> 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@gmail.com> committer: Michael Widenius <michael.widenius@gmail.com> 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@mariadb.org
participants (2)
-
Michael Widenius -
Sergei Golubchik