Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
Hi!
On Thu, Apr 1, 2021 at 12:03 AM Sergei Golubchik wrote:
> > > > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > > > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> > > >part_def->part_id, part_def->is_subpart,
> > > > - part_name));
> > > > + length, part_name));
> > >
> > > These two changes contradict each other. Either part_name must be
> > > \0-terminated, and then you don't need to specify a length in printf.
> > > Or it is not \0-terminated and you must specify a length.
> >
> > part_name is not null terminated and that is why I had to add .* and length.
> > What is the contradiction ?
>
> You've cut too much from my email. "contradiction" referred to the
> change just above this one, it was:
>
> > - @param part_name Partition name to match.
> > + @param part_name Partition name to match. Must be \0 terminated!
>
> so, please, decide what it is. Either the comment is wrong or the code
> is redundant.
Neither. The comment is right. part_name must at this point in time be
null terminated, but
may not have to be that in the future.
The printf string for DBUG_PRINT is correct. It uses the length,
which avoids a strlen() inside
sprintf() and is also future proof.
The only reason that part_name needs to be null terminated is because
of this code:
if (!part_def)
{
my_error(ER_UNKNOWN_PARTITION, MYF(0), part_name, table->alias.c_ptr());
DBUG_RETURN(true);
}
Whenever we fix the error message to use %.*s, then we can lift the
restrictions that
part_name needs to be null terminated.
Regards,
Monty
> > > > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s,
> > > > uint len, CHARSET_INFO *cs)
> > > > // Shrink the buffer, but only if it is allocated on the heap.
> > > > void Binary_string::shrink(size_t arg_length)
> > > > {
> > > > -if (!is_alloced())
> > > > -return;
> > > > -if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > > + {
> > > > +char *new_ptr;
> > > > +/* my_realloc() can't fail as new buffer is less than the original
> > > > one */
> > > > +if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr,
> > > > arg_length,
> > >
> > > you don't need an if() because, as you wrote yourself "my_realloc()
> > > can't fail" here.
> >
> > Yes, this should be true, but one never knows with allocation libraries.
> > There is implementation of realloc that makes a copy and deletes the old
> > one.
> > However I am not sure what happens if there is no place to make a copy.
> > Better safe than sorry.
>
> No. I've fixed this quite a while ago. my_realloc() can *never* return
> NULL when reducing a buffer size. System realloc(), indeed, can, I've
> researched that topic when making the change. But my_realloc() has a
> protection against it and it never returns NULL in this case.
> We have few places in the code that rely on it.
We also have code that relies on that my_malloc() never fails.
Note what this commit did was just an cleanup of the original code,
not trying to do any big changes of logic, except for c_ptr().
Anyway, I can remove the if here to keep you happy.
> > > > +{
> > > > + Ptr[str_length]=0;
> > > > + return Ptr;
> > > > +}
> > > > +(void) realloc(str_length+1); /* This will add end
> > > > \0 */
> > > > return Ptr;
> > > >}
> > > > + /* Use c_ptr() instead. This will be deleted soon, kept for
> > > > compatiblity */
> > > >inline char *c_ptr_quick()
> > > >{
> > > > -if (Ptr && str_length < Alloced_length)
> > > > - Ptr[str_length]=0;
> > > > -return Ptr;
> > > > +return c_ptr_safe();
> > >
> > > 1. why not to remove it now?
> > > 2. it's strange that you write "use c_ptr() instead" but you actually
> > >use c_ptr_safe() instead.
> >
> > What I meant is that the developer should instead use c_ptr().
> > I will make the message more clear.
> > I decided to call c_ptr_safe() here as this is the most safe
> > version to use. In practice the new c_ptr() should be safe
> > for 99% of our code.
>
> Let's remove c_ptr_quick() now. Why keep it?
Not enough time to do it now. I rather work on fixing review comments.
Regards,
Monty
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
Hi, Monty!
On Mar 31, Michael Widenius wrote:
> > > > --- a/sql/sql_string.h
> > > > +++ b/sql/sql_string.h
> > > > @@ -600,25 +600,34 @@ class Binary_string: public Static_binary_string
> > > >
> > > >inline char *c_ptr()
> > > >{
> > > > -DBUG_ASSERT(!alloced || !Ptr || !Alloced_length ||
> > > > -(Alloced_length >= (str_length + 1)));
> > > > -
> > > > -if (!Ptr || Ptr[str_length]) // Should be safe
> > > > - (void) realloc(str_length);
> > > > +if (unlikely(!Ptr))
> > > > + return (char*) "";
> > > > +/*
> > > > + Here we assume that any buffer used to initalize String has
> > > > + an end \0 or have at least an accessable character at end.
> > > > + This is to handle the case of String("Hello",5) efficently.
> > > > +*/
> > > > +if (unlikely(!alloced && !Ptr[str_length]))
> > > > + return Ptr;
> > >
> > > No, this is not good. Note the difference between
> > >
> > > String a("Hello", 5)
> > >
> > > and
> > >
> > > char hello[5];
> > > String a(buf, 5);
> > >
> > > Your assumption should only apply to the first case, not to the second.
> > > In the first case alloced=Alloced_length=0, in the second case only
> > > alloced=0 and Alloced_length=5. So in the if() above you need to look
> > > at Alloced_length:
> > >
> > > if (!Alloced_length && !Ptr[str_length])
> > > return Ptr;
>
> Unfortunately this does not work good with our code.
>
> We have a lot of code that does things like this:
> String *new_str= new (thd->mem_root) String((const char*) name.str,
> name.length,
> system_charset_info)
> Where string is 0 terminated.
> With your proposed change, all of these will require a full realloc
> when using c_ptr().
Hmm, I don't understand. The difference between
String a("Hello", 5);
and
char hello[5];
String a(buf, 5);
is that in the first case the argument is const char*. And in that case
Alloced_length=0 and in that case you can expect the string to be \0
terminated.
In your example that we do "a lot", the first argument is const char*,
so String will have Alloced_length==0 and my suggestion will work
exactly as planned, it'll use a \0 terminated fast path.
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
Hi, Michael!
On Mar 31, Michael Widenius wrote:
>
> Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
> build these up I do not want to depend on that for generic usage. For
> usage to printf() than we should trust that the developer knows what
> he is doing.
>
> However Item_string() will only use the exact length and never call
> printf, so here this is safe.
Okay. So, in effect, LEX_CSTRING is *not necessarily* \0 terminated.
Fine.
> > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
>
>
> > > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> > >part_def->part_id, part_def->is_subpart,
> > > - part_name));
> > > + length, part_name));
> >
> > These two changes contradict each other. Either part_name must be
> > \0-terminated, and then you don't need to specify a length in printf.
> > Or it is not \0-terminated and you must specify a length.
>
> part_name is not null terminated and that is why I had to add .* and length.
> What is the contradiction ?
You've cut too much from my email. "contradiction" referred to the
change just above this one, it was:
> - @param part_name Partition name to match.
> + @param part_name Partition name to match. Must be \0 terminated!
so, please, decide what it is. Either the comment is wrong or the code
is redundant.
> > > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, uint
> > > len, CHARSET_INFO *cs)
> > > // Shrink the buffer, but only if it is allocated on the heap.
> > > void Binary_string::shrink(size_t arg_length)
> > > {
> > > -if (!is_alloced())
> > > -return;
> > > -if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > > + {
> > > +char *new_ptr;
> > > +/* my_realloc() can't fail as new buffer is less than the original
> > > one */
> > > +if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr,
> > > arg_length,
> >
> > you don't need an if() because, as you wrote yourself "my_realloc()
> > can't fail" here.
>
> Yes, this should be true, but one never knows with allocation libraries.
> There is implementation of realloc that makes a copy and deletes the old one.
> However I am not sure what happens if there is no place to make a copy.
> Better safe than sorry.
No. I've fixed this quite a while ago. my_realloc() can *never* return
NULL when reducing a buffer size. System realloc(), indeed, can, I've
researched that topic when making the change. But my_realloc() has a
protection against it and it never returns NULL in this case.
We have few places in the code that rely on it.
> > > +{
> > > + Ptr[str_length]=0;
> > > + return Ptr;
> > > +}
> > > +(void) realloc(str_length+1); /* This will add end \0
> > > */
> > > return Ptr;
> > >}
> > > + /* Use c_ptr() instead. This will be deleted soon, kept for
> > > compatiblity */
> > >inline char *c_ptr_quick()
> > >{
> > > -if (Ptr && str_length < Alloced_length)
> > > - Ptr[str_length]=0;
> > > -return Ptr;
> > > +return c_ptr_safe();
> >
> > 1. why not to remove it now?
> > 2. it's strange that you write "use c_ptr() instead" but you actually
> >use c_ptr_safe() instead.
>
> What I meant is that the developer should instead use c_ptr().
> I will make the message more clear.
> I decided to call c_ptr_safe() here as this is the most safe
> version to use. In practice the new c_ptr() should be safe
> for 99% of our code.
Let's remove c_ptr_quick() now. Why keep it?
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 21a84e23cf3: MDEV-16437: merge 5.7 P_S replication instrumentation and tables
Hi, Sujatha! Note, it's a review of the combined `git diff e5fc78 21a84e` On Mar 31, Sujatha wrote: > revision-id: 21a84e23cf3 (mariadb-10.5.2-307-g21a84e23cf3) > parent(s): 4e6de585ff7 > author: Sujatha > committer: Sujatha > timestamp: 2020-11-30 13:22:32 +0530 > message: > > MDEV-16437: merge 5.7 P_S replication instrumentation and tables That was pretty good, thanks! Just few small comments below, no big changes will be needed. > diff --git a/mysql-test/suite/multi_source/simple.result > b/mysql-test/suite/multi_source/simple.result > index a66d49e88cb..b57e146feb5 100644 > --- a/mysql-test/suite/multi_source/simple.result > +++ b/mysql-test/suite/multi_source/simple.result > @@ -21,7 +21,21 @@ show all slaves status; > Connection_name Slave_SQL_State Slave_IO_State Master_Host > Master_User Master_Port Connect_Retry Master_Log_File > Read_Master_Log_Pos Relay_Log_File Relay_Log_Pos Relay_Master_Log_File > Slave_IO_RunningSlave_SQL_Running Replicate_Do_DB > Replicate_Ignore_DB Replicate_Do_Table Replicate_Ignore_Table > Replicate_Wild_Do_Table Replicate_Wild_Ignore_Table Last_Errno > Last_Error Skip_CounterExec_Master_Log_Pos Relay_Log_Space > Until_Condition Until_Log_File Until_Log_Pos Master_SSL_Allowed > Master_SSL_CA_File Master_SSL_CA_Path Master_SSL_Cert > Master_SSL_Cipher Master_SSL_Key Seconds_Behind_Master > Master_SSL_Verify_Server_Cert Last_IO_Errno Last_IO_Error > Last_SQL_Errno Last_SQL_Error Replicate_Ignore_Server_Ids > Master_Server_IdMaster_SSL_Crl Master_SSL_Crlpath Using_Gtid > Gtid_IO_Pos Replicate_Do_Domain_Ids Replicate_Ignore_Domain_Ids > Parallel_Mode SQL_Delay SQL_Remaining_Delay > Slave_SQL_Running_State Slave_DDL_Groups > Slave_Non_Transactional_Groups Slave_Transactional_Groups > Retried_transactionsMax_relay_log_size Executed_log_entries > Slave_received_heartbeats Slave_heartbeat_period Gtid_Slave_Pos > slave1 Slave has read all relay log; waiting for more updates Waiting > for master to send event127.0.0.1 rootMYPORT_160 > master-bin.01 > mysqld-relay-bin-slave1.02 master-bin.01 Yes > Yes 0 0 > None0 > No 0 No 0 > 0 1 No > optimistic 0 NULLSlave has read all relay log; waiting for > more updates 0 0 0 0 1073741824 7 0 > 60.000 > slave2 Slave has read all relay log; waiting for more updates Waiting > for master to send event127.0.0.1 rootMYPORT_260 > master-bin.01 > mysqld-relay-bin-slave2.02 master-bin.01 Yes > Yes 0 0 > None0 > No 0 No 0 > 0 2 No > optimistic 0 NULLSlave has read all relay log; waiting for > more updates 0 0 0 0 1073741824 7 0 > 60.000 > +# > +# MDEV:16437: merge 5.7 P_S replication instrumentation and tables > +# > +select * from performance_schema.replication_connection_configuration; > +CONNECTION_NAME HOSTPORTUSERUSING_GTID SSL_ALLOWED > SSL_CA_FILE SSL_CA_PATH SSL_CERTIFICATE SSL_CIPHER SSL_KEY > SSL_VERIFY_SERVER_CERTIFICATE SSL_CRL_FILESSL_CRL_PATH > CONNECTION_RETRY_INTERVAL CONNECTION_RETRY_COUNT HEARTBEAT_INTERVAL > IGNORE_SERVER_IDS REPL_DO_DOMAIN_IDS REPL_IGNORE_DOMAIN_IDS > +slave2 # # rootNO NO > NO 60 86400 60.000 > > +slave1 # # rootNO NO > NO 60 86400 60.000 > Isn't the host always 127.0.0.1 ? Why to mask it? Also, may be it'd be easier to read the result, if you print it vertically? It looks that CONNECTION_RETRY_COUNT is 86400. And 86400 is clearly a timeout, not a retry count. > start all slaves; > +# > +# MDEV:16437: merge 5.7 P_S replication instrumentation and tables > +# > +select * from performance_schema.replication_applier_status_by_coordinator; > +CONNECTION_NAME THREAD_ID SERVICE_STATE LAST_SEEN_TRANSACTION > LAST_ERROR_NUMBER LAST_ERROR_MESSAGE LAST_ERROR_TIMESTAMP > LAST_TRANS_RETRY_COUNT > +slave2 # ON
Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
Hi!
Sorry, this was because gmail send it before I wanted to.
(I do not like that ctrl+return sends email as this causes problems
when one uses this a lot in slack!)
> > > --- a/sql/sql_string.h
> > > +++ b/sql/sql_string.h
> > > @@ -600,25 +600,34 @@ class Binary_string: public Static_binary_string
> > >
> > >inline char *c_ptr()
> > >{
> > > -DBUG_ASSERT(!alloced || !Ptr || !Alloced_length ||
> > > -(Alloced_length >= (str_length + 1)));
> > > -
> > > -if (!Ptr || Ptr[str_length]) // Should be safe
> > > - (void) realloc(str_length);
> > > +if (unlikely(!Ptr))
> > > + return (char*) "";
> > > +/*
> > > + Here we assume that any buffer used to initalize String has
> > > + an end \0 or have at least an accessable character at end.
> > > + This is to handle the case of String("Hello",5) efficently.
> > > +*/
> > > +if (unlikely(!alloced && !Ptr[str_length]))
> > > + return Ptr;
> >
> > No, this is not good. Note the difference between
> >
> > String a("Hello", 5)
> >
> > and
> >
> > char hello[5];
> > String a(buf, 5);
> >
> > Your assumption should only apply to the first case, not to the second.
> > In the first case alloced=Alloced_length=0, in the second case only
> > alloced=0 and Alloced_length=5. So in the if() above you need to look
> > at Alloced_length:
> >
> > if (!Alloced_length && !Ptr[str_length])
> > return Ptr;
Unfortunately this does not work good with our code.
We have a lot of code that does things like this:
String *new_str= new (thd->mem_root) String((const char*) name.str,
name.length,
system_charset_info)
Where string is 0 terminated.
With your proposed change, all of these will require a full realloc when using
c_ptr().
Regards,
Monty
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
On Wed, Mar 31, 2021 at 9:45 PM Michael Widenius
wrote:
>
> Cut
> > > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
> > >
> > > The proble was that hen using String::alloc() to allocate a string,
> >
> > "hen" ? after a minute of staring I realized that you probably meant "when".
> >
> > (also, "problem")
> >
> > > the String ensures that there is space for an extra NULL byte in the
> > > buffer and if not, reallocates the string. This is a problem with the
> > > String::set_int() that calls alloc(21), which forces
> > > an extra malloc/free to happen.
> > >
> > > - We do not anymore re-allocate String if alloc() is called with the
> > > Allocated_length. This reduces number of malloc() allocations,
> > > especially one big re-allocation in Protocol::send_result_Set_metadata()
> > > for almost every query that produced a result to the connnected client.
> > > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE
> > > This can now be done as alloc() doesn't increase buffers if new length
> > > is
> > > not bigger than old one.
> > > - c_ptr() is redesigned to be safer (but a bit longer) than before.
> > > - Remove wrong usage of c_ptr_quick()
> > > c_ptr_quick() where used in many cases to get the pointer to the used
> >
> > s/where/was/
> >
> > > buffer, even when it didn't need to be \0 terminated. In this case
> > > ptr() is a better substitute.
> > > Another problem with c_ptr_quick() is that it didn't guarantee that
> > > the string would be \0 terminated.
> > > - item_val_str(), an API function not used currently by the server,
> > > now always returns a null terminated string (before it didn't always
> > > do that).
> > > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old
> > > mixed usage of performance keys caused assert's when String buffers
> > > where shrunk.
> > > - Binary_string::shrink() is simplifed
>
> Will fix the typos. Thanks!
>
> > > diff --git a/sql/item.cc b/sql/item.cc
> > > index f6f3e2720fa..a8a43c6266a 100644
> > > --- a/sql/item.cc
> > > +++ b/sql/item.cc
> > > @@ -4703,7 +4703,7 @@ Item *Item_param::value_clone_item(THD *thd)
> > > return 0; // Should create Item_decimal. See MDEV-11361.
> > >case STRING_RESULT:
> > > return new (mem_root) Item_string(thd, name,
> > > -
> > > Lex_cstring(value.m_string.c_ptr_quick(),
> > > + Lex_cstring(value.m_string.ptr(),
> >
> > Hmm, you said that LEX_CSTRING::str should always be \0-terminated.
> > If that's the case, then c_ptr() would be correct here, not ptr()
> >
> > >
> > > value.m_string.length()),
> > >value.m_string.charset(),
> > >collation.derivation,
>
> Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
> build these
> up I do not want to depend on that for generic usage. For usage to
> printf() than we
> should trust that the developer knows what he is doing.
>
> However Item_string() will only use the exact length and never call
> printf, so here this is safe.
>
> > > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
>
>
> > > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> > >part_def->part_id, part_def->is_subpart,
> > > - part_name));
> > > + length, part_name));
> >
> > These two changes contradict each other. Either part_name must be
> > \0-terminated, and then you don't need to specify a length in printf.
> > Or it is not \0-terminated and you must specify a length.
>
> part_name is not null terminated and that is why I had to add .* and length.
> What is the contradiction ?
>
> > > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > > index f2a0f55aec8..95a57017c53 100644
> > > --- a/sql/sql_string.cc
> > > +++ b/sql/sql_string.cc
> > > @@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
> > >return FALSE;
> > > }
> > >
> > > +
> > > bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
> > > {
> > > - uint l=20*cs->mbmaxlen+1;
> > > + /*
> > > +This allocates a few bytes extra in the unlikely case that
> > > cs->mb_maxlen
> > > +> 1, but we can live with that
> >
> > dunno, it seems that utf8 is the *likely* case and it's getting more and
> > more likely with the time,
>
> Not for integers, as we often use binary strings for these.
>
> > > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, uint
> > > len, CHARSET_INFO *cs)
> > > // Shrink the buffer, but only if it is allocated on the heap.
> > > void Binary_string::shrink(size_t arg_length)
> > > {
> > > -if (!is_alloced())
> > > -return;
> > > -if (ALIGN_SIZE(arg_length + 1) <
Re: [Maria-developers] f46382ba93c: Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
Cut
> > Optimize usage of c_ptr(), c_ptr_quick() and String::alloc()
> >
> > The proble was that hen using String::alloc() to allocate a string,
>
> "hen" ? after a minute of staring I realized that you probably meant "when".
>
> (also, "problem")
>
> > the String ensures that there is space for an extra NULL byte in the
> > buffer and if not, reallocates the string. This is a problem with the
> > String::set_int() that calls alloc(21), which forces
> > an extra malloc/free to happen.
> >
> > - We do not anymore re-allocate String if alloc() is called with the
> > Allocated_length. This reduces number of malloc() allocations,
> > especially one big re-allocation in Protocol::send_result_Set_metadata()
> > for almost every query that produced a result to the connnected client.
> > - Avoid extra mallocs when using LONGLONG_BUFFER_SIZE
> > This can now be done as alloc() doesn't increase buffers if new length is
> > not bigger than old one.
> > - c_ptr() is redesigned to be safer (but a bit longer) than before.
> > - Remove wrong usage of c_ptr_quick()
> > c_ptr_quick() where used in many cases to get the pointer to the used
>
> s/where/was/
>
> > buffer, even when it didn't need to be \0 terminated. In this case
> > ptr() is a better substitute.
> > Another problem with c_ptr_quick() is that it didn't guarantee that
> > the string would be \0 terminated.
> > - item_val_str(), an API function not used currently by the server,
> > now always returns a null terminated string (before it didn't always
> > do that).
> > - Ensure that all String allocations uses STRING_PSI_MEMORY_KEY. The old
> > mixed usage of performance keys caused assert's when String buffers
> > where shrunk.
> > - Binary_string::shrink() is simplifed
Will fix the typos. Thanks!
> > diff --git a/sql/item.cc b/sql/item.cc
> > index f6f3e2720fa..a8a43c6266a 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -4703,7 +4703,7 @@ Item *Item_param::value_clone_item(THD *thd)
> > return 0; // Should create Item_decimal. See MDEV-11361.
> >case STRING_RESULT:
> > return new (mem_root) Item_string(thd, name,
> > -
> > Lex_cstring(value.m_string.c_ptr_quick(),
> > + Lex_cstring(value.m_string.ptr(),
>
> Hmm, you said that LEX_CSTRING::str should always be \0-terminated.
> If that's the case, then c_ptr() would be correct here, not ptr()
>
> >value.m_string.length()),
> >value.m_string.charset(),
> >collation.derivation,
Yes, LEX_CSTRING *should* always be \0 terminated, but as we sometimes
build these
up I do not want to depend on that for generic usage. For usage to
printf() than we
should trust that the developer knows what he is doing.
However Item_string() will only use the exact length and never call
printf, so here this is safe.
> > diff --git a/sql/partition_info.cc b/sql/partition_info.cc
> > - DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %s",
> > + DBUG_PRINT("info", ("Found partition %u is_subpart %d for name %.*s",
> >part_def->part_id, part_def->is_subpart,
> > - part_name));
> > + length, part_name));
>
> These two changes contradict each other. Either part_name must be
> \0-terminated, and then you don't need to specify a length in printf.
> Or it is not \0-terminated and you must specify a length.
part_name is not null terminated and that is why I had to add .* and length.
What is the contradiction ?
> > diff --git a/sql/sql_string.cc b/sql/sql_string.cc
> > index f2a0f55aec8..95a57017c53 100644
> > --- a/sql/sql_string.cc
> > +++ b/sql/sql_string.cc
> > @@ -118,9 +121,14 @@ bool Binary_string::realloc_raw(size_t alloc_length)
> >return FALSE;
> > }
> >
> > +
> > bool String::set_int(longlong num, bool unsigned_flag, CHARSET_INFO *cs)
> > {
> > - uint l=20*cs->mbmaxlen+1;
> > + /*
> > +This allocates a few bytes extra in the unlikely case that
> > cs->mb_maxlen
> > +> 1, but we can live with that
>
> dunno, it seems that utf8 is the *likely* case and it's getting more and
> more likely with the time,
Not for integers, as we often use binary strings for these.
> > @@ -1254,21 +1262,16 @@ bool String::append_semi_hex(const char *s, uint
> > len, CHARSET_INFO *cs)
> > // Shrink the buffer, but only if it is allocated on the heap.
> > void Binary_string::shrink(size_t arg_length)
> > {
> > -if (!is_alloced())
> > -return;
> > -if (ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > + if (is_alloced() && ALIGN_SIZE(arg_length + 1) < Alloced_length)
> > + {
> > +char *new_ptr;
> > +/* my_realloc() can't fail as new buffer is less than the original one
> > */
> > +if ((new_ptr= (char*) my_realloc(STRING_PSI_MEMORY_KEY, Ptr,
> > arg_length,
>
> you
Re: [Maria-developers] 9bf4b92cbc5: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Sergei! On Wed, Mar 31, 2021 at 5:52 PM Sergei Golubchik wrote: > > Hi, Aleksey! > > On Dec 01, Sergei Golubchik wrote: > > Hi, Aleksey! > > > > just one comment below, no comments on all your other replies. > > > > The issue is in review, shall I take another look or it's on you now? > > I think I've never got a reply to this question. > Shall I take another look at the bb-10.6-midenok-MDEV-17554 branch or > not yet? I've updated the branch on top of the latest 10.6. Please go ahead and take a look. > > > On Sep 01, Aleksey Midenkov wrote: > > > Regards, > Sergei > VP of MariaDB Server Engineering > and secur...@mariadb.org -- All the best, Aleksey Midenkov @midenok ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()
Hi, Michael!
On Mar 31, Michael Widenius wrote:
> Hi!
>
> On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik wrote:
> >
> > Hi, Michael!
> >
> > On Mar 29, Michael Widenius wrote:
> > > revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82)
> > > parent(s): a194c5cfd41
> > > author: Michael Widenius
> > > committer: Michael Widenius
> > > 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;
> > > TableOp Msg_typeMsg_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> ",
> > > +"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 secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 1bbc852ef71: Changed field_index to use field_index_t instead of uint16
Hi!
On Mon, Mar 29, 2021 at 3:55 PM Sergei Golubchik wrote:
>
> Hi, Michael!
>
> On Mar 29, Michael Widenius wrote:
> > revision-id: 1bbc852ef71 (mariadb-10.5.2-525-g1bbc852ef71)
> > parent(s): 039f4a054bb
> > author: Michael Widenius
> > committer: Michael Widenius
> > timestamp: 2021-03-24 14:31:54 +0200
> > message:
> >
> > Changed field_index to use field_index_t instead of uint16
>
> Just curious. Why?
Mainly because it makes the code easier to read:
find_field_in_table(THD *thd, TABLE *table, const char *name, size_t length,
-bool allow_rowid, uint16 *cached_field_index_ptr)
+bool allow_rowid, field_index_t *cached_field_index_ptr)
...
-static inline uint16 read_frm_fieldno(const uchar *data)
+static inline field_index_t read_frm_fieldno(const uchar *data)
With the new version, one can easier understand what the function expects
as arguments and what the function returns.
I had also seen different types used for field numbers in some
functions/classes and that also did bother me a bit so I decided to
fix this once and for all.
For example, in 10.5 we have:
item.h: uint cached_field_index;
> > diff --git a/sql/unireg.cc b/sql/unireg.cc
> > index 51dd1fb6e8c..f1a0c5c5269 100644
> > --- a/sql/unireg.cc
> > +++ b/sql/unireg.cc
> > @@ -146,8 +146,8 @@ get_fieldno_by_name(HA_CREATE_INFO *create_info,
> > List _fiel
> >{
> > if (field_name.streq(sql_field->field_name))
> > {
> > - DBUG_ASSERT(field_no <= uint16(~0U));
> > + DBUG_ASSERT(field_no < NO_CACHED_FIELD_INDEX);
>
> this look suspicios. NO_CACHED_FIELD_INDEX is ((uint)(-1))
> which is always greater than anything you can put into field_index_t.
We have code that uses NO_CACHED_FIELD_INDEX in field_no
to indicate that there was no such field.
I looked at current code and we have:
#define NO_CACHED_FIELD_INDEX ((uint16) ~0)
> You want to ensure that field_no is valid when casted into
> field_index_t. I'd suggest to redefine NO_CACHED_FIELD_INDEX as
>
> ((field_index_t)~0U)
I can redefine as that.
> > - return uint16(field_no);
> > + return (field_index_t) field_no;
>
> why not to declare field_no to be field_index_t? then you won't need a
> cast.
I have now fixed the above and and also changed type for the function
and also updated this in table.cc:
-static uint find_field(Field **fields, uchar *record, uint start, uint length);
+static field_index_t find_field(Field **fields, uchar *record, uint start,
+uint length);
I missed some of these cases as I did not get warnings from these from
the compiler...
Regards,
Monty
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 1d577194654: Revert MDEV-16592 "Change Item::with_sum_func to a virtual method"
Hi!
On Mon, Mar 29, 2021 at 4:20 PM Sergei Golubchik wrote:
>
> Hi, Michael!
Only people who do not know me calls me Michael...
Who are you?
> > Revert MDEV-16592 "Change Item::with_sum_func to a virtual method"
> >
> > diff --git a/sql/item.cc b/sql/item.cc
> > index e5be8699d80..e0ba4ff2c89 100644
> > --- a/sql/item.cc
> > +++ b/sql/item.cc
> > @@ -975,9 +975,7 @@ class Item :public Value_source,
> >void share_name_with(const Item *item)
> >{
> > name= item->name;
> > -common_flags= static_cast
> > - ((common_flags & ~IS_AUTO_GENERATED_NAME) |
> > - (item->common_flags & IS_AUTO_GENERATED_NAME));
> > +is_autogenerated_name= item->is_autogenerated_name;
>
> this, apparently, should be in the commit 039f4a054bb
> "Removed Item::common_flags and replaced it with bit fields"
Have now done this.
I assume you understand that moving a commit like this between version
takes about 10-30 min and have no affect on the end result.
I am happy to do that if you think it is important, but doing it over
and over again
is a bit waste of time that I could use for more critical items...
Regards,
Monty
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] f8901333a82: Add support for minimum field width for strings to my_vsnprintf()
Hi!
On Mon, Mar 29, 2021 at 9:39 PM Sergei Golubchik wrote:
>
> Hi, Michael!
>
> On Mar 29, Michael Widenius wrote:
> > revision-id: f8901333a82 (mariadb-10.5.2-534-gf8901333a82)
> > parent(s): a194c5cfd41
> > author: Michael Widenius
> > committer: Michael Widenius
> > 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;
> > TableOp Msg_typeMsg_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;
> > TableOp Msg_typeMsg_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.
> > 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> ",
> > +"Negative width
Re: [Maria-developers] 25fe7dbad5d: Reduce usage of strlen()
Hi!
On Wed, Mar 31, 2021 at 5:18 PM Sergei Golubchik wrote:
>
> Hi, Michael!
>
> On Mar 31, Michael Widenius wrote:
> > commit 25fe7dbad5d
> > Author: Michael Widenius
> > Date: Wed Aug 12 20:29:55 2020 +0300
> >
> >Reduce usage of strlen()
>
> I'll only mention actual mistakes, no style comments or suggestions or
> question, just want to get it over with.
>
> > diff --git a/sql/item.h b/sql/item.h
> > index 9a6d0b4abfe..d6ff10bf3ba 100644
> > --- a/sql/item.h
> > +++ b/sql/item.h
> > @@ -1093,7 +1093,7 @@ class Item :public Value_source,
> >void share_name_with(const Item *item)
> >{
> > name= item->name;
> > -copy_flags(item, item_base_t::IS_AUTOGENERATED_NAME);
> > +copy_flags(item, item_base_t::IS_EXPLICIT_NAME);
>
> belongs to a previous commit
Sorry about that, must have happened when I did a fixup trying to keep
things clean.
How can I move most easily to the right commit.
Do a rebase and do "edit" for the previous commit, do the change by
hand and continue the rebase?
Will try that.
> > diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc
> > index 262481d019b..a9e008b0b92 100644
> > --- a/sql/item_windowfunc.cc
> > +++ b/sql/item_windowfunc.cc
> > @@ -99,13 +99,15 @@ Item_window_func::fix_fields(THD *thd, Item **ref)
> >
> >if (window_spec->window_frame && is_frame_prohibited())
> >{
> > -my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0),
> > window_func()->func_name());
> > +my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0),
> > + window_func()->func_name());
>
> This should be func_name().str, otherwise it'll crash or
> print garbage on Windows. Same below, in a couple of places.
Nope, this works thanks to this:
inline const char *func_name() const
{ return (char*) func_name_cstring().str; }
So func_name() can be used (instead of the old variable func_name)
when one wants to have the a char* pointer.
This was convenient to have for future merges and make the changes in
the commit easier to read.
Regards,
Monty
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 9bf4b92cbc5: MDEV-17554 Auto-create new partition for system versioned tables with history partitioned by INTERVAL/LIMIT
Hi, Aleksey! On Dec 01, Sergei Golubchik wrote: > Hi, Aleksey! > > just one comment below, no comments on all your other replies. > > The issue is in review, shall I take another look or it's on you now? I think I've never got a reply to this question. Shall I take another look at the bb-10.6-midenok-MDEV-17554 branch or not yet? > On Sep 01, Aleksey Midenkov wrote: > Regards, Sergei VP of MariaDB Server Engineering and secur...@mariadb.org ___ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp
Re: [Maria-developers] 25fe7dbad5d: Reduce usage of strlen()
Hi, Michael!
On Mar 31, Michael Widenius wrote:
> commit 25fe7dbad5d
> Author: Michael Widenius
> Date: Wed Aug 12 20:29:55 2020 +0300
>
>Reduce usage of strlen()
I'll only mention actual mistakes, no style comments or suggestions or
question, just want to get it over with.
> diff --git a/sql/item.h b/sql/item.h
> index 9a6d0b4abfe..d6ff10bf3ba 100644
> --- a/sql/item.h
> +++ b/sql/item.h
> @@ -1093,7 +1093,7 @@ class Item :public Value_source,
>void share_name_with(const Item *item)
>{
> name= item->name;
> -copy_flags(item, item_base_t::IS_AUTOGENERATED_NAME);
> +copy_flags(item, item_base_t::IS_EXPLICIT_NAME);
belongs to a previous commit
>}
>virtual void cleanup();
>virtual void make_send_field(THD *thd, Send_field *field);
> diff --git a/sql/item_windowfunc.cc b/sql/item_windowfunc.cc
> index 262481d019b..a9e008b0b92 100644
> --- a/sql/item_windowfunc.cc
> +++ b/sql/item_windowfunc.cc
> @@ -99,13 +99,15 @@ Item_window_func::fix_fields(THD *thd, Item **ref)
>
>if (window_spec->window_frame && is_frame_prohibited())
>{
> -my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0),
> window_func()->func_name());
> +my_error(ER_NOT_ALLOWED_WINDOW_FRAME, MYF(0),
> + window_func()->func_name());
This should be func_name().str, otherwise it'll crash or
print garbage on Windows. Same below, in a couple of places.
> return true;
>}
Regards,
Sergei
VP of MariaDB Server Engineering
and secur...@mariadb.org
___
Mailing list: https://launchpad.net/~maria-developers
Post to : maria-developers@lists.launchpad.net
Unsubscribe : https://launchpad.net/~maria-developers
More help : https://help.launchpad.net/ListHelp
