Mats, hej.
Hope you are getting better!
A newer patch has been committed.
> Andrei Elkin wrote:
>> Mats, hej.
>>
>> Thanks for a good review!
>>
>> I've committed a fresh patch accounting your notes.
>>
>
> OK, I'll have a look at it.
>
>> Here are my answers.
> [snip]
>>> Andrei Elkin wrote:
>>>
> [snip]
>
>>>> +#define is_bit_set(i, b) (uint) ((sizeof((i))*8 > b)? \
>>>> + ((uchar*) (&(i)))[(b) / 8] \
>>>> + & (1 << ((b) & 7))
> \
>>>> + : 0)
>>>>
>
> [snip]
>
>>> Although I think that macros or functions for this purpose is a good
>>> thing, these macros makes a large number of assumptions that is
>>> incorrect and cause confusing error or incorrect behavior.
>>>
>>>
>>
>>
>>> 1. It assumes that all bytes are 8 bits large, which is not the case.
>>>
>>
>> Generally in the server's code we are better to have the only
>> assumption. And this one as you must know is all bytes are 8 bits
>> large in my_bitmap.h,cc etc.
>>
Well, I was deluded with the magic 8...
>
> Referring to another part of the code does not change the fact that
> it's not correct, but this is not the important part. I would accept
> that bytes are treated as 8-bit entities when it does not affect the
> correctness. Comparing with my_bitmap, there is a difference. In that
> case, the assumption of 8-bit bytes just lead to wasting memory,
> but in this case, it will cause the computations to not work.
Finally I understood your point.
bitmap library exploits the magic 8, which makes the library
endian-sensetive.
Whereas in my macros there is a special out-of-range
test (sizeof((i))*8 > b) where 8 serves for different purpose.
So, I accept your suggestion and will modify the out-of-range's test.
> Still, I
> would not consider this as an important problem, if it were not for
> the issues below.
>
> [snip]
>>
>>> 4. It is endian-dependent. For example is_bit_set(0x1, 0) will be
>>> 'false' on a big-endian machine, but not on a little-endian
>>> machine.
>>>
>>
>> A good catch.
>> You must have seen that macros i used as the template are vulnarable as well
>>
>> #define _bitmap_set_bit(MAP, BIT) (((uchar*)(MAP)->bitmap)[(BIT) / 8] \
>> |= (1 << ((BIT) & 7)))
>>
>> And then they're same wat buggy, are not they?
>>
>
> Actually not, they are storing and retrieving data in the same
> "faulty" way, so they compensate each others (now, I wouldn't write
> the code this way, but that is a different issue). However, in this
> case, you are writing an "int" and retrieving it as "char", which is
> the real problem.
>
However, the macros themselves neither store nor retrieve.
[you wrote]
4. It is endian-dependent. For example is_bit_set(0x1, 0) will be
'false' on a big-endian machine, but not on a little-endian machine.
Hence, your p.4 applies for this existing macro
#define _bitmap_is_set(MAP, BIT) (uint) (((uchar*)(MAP)->bitmap)[(BIT) / 8] \
& (1 << ((BIT) & 7)))
as well: *MAP->bitmap = 0x1, BIT = 0.
The only way for the macro to be endian-agnostic is not use merely
(1 << BIT) shift and never locate the byte with []!
That's what you offered in your first review mail.
>>> 5. It does not work if something is passed as 'i' that has a size
>>> larger than 'int' (e.g., unsigned long long).
>>>
>>
>> What do you mean? There is the check
>>
>> sizeof((i))*8 > b
>>
>> so that out-of-range bit forces "not set" return - which is correct.
>> Although it'd better to return -1 to designate the error in
>> accessing the bit.
>> That's what I am adding to your versions below.
>>
>
> Nope, that is not what I was referring to... here is a small program
> with a simplified version of the macro. Note that for the values
> below, sizeof(V) * 8 <= (N), so the "out of range" result would not be
> returned.
>
> #define bit_is_set(V,N) ((V) & (1 << (N)) ? 1 : 0)
>
> int main() {
> printf("bit_is_set(%llx,%d): %d\n",
> (1ULL << 34), 34, bit_is_set(1ULL << 34, 34));
> return EXIT_SUCCESS;
> }
>
> The output from this program is:
>
> bit_is_set(400000000,34): 0
Sorry, I could not combine your p.5 with this example.
So we have N=34, V=400 000 000 and sizeof(V)=4, that's why
"sizeof(V) * 8 <= (N)", right?
But I was expecting the reverse sizeof(V) * 8 > N as
there should be some hint towards "unsigned long long" ...
>
>
> [snip]
>>> 6. I would prefer to have a naming of the macros with a common
>>> prefix, e.g., "bit_" since this is how all other naming is done
>>> (e.g., "my_", "mysql_", "COM_", "SQLCOM_")
>>>
>>
>> Okay. Let it be bit_is_set, bit_do_set, bit_do_clear.
>>
>>
>>
>>> 7. IMHO, do_set_bit() and do_clear_bit() are statements and should
>>> not have a value, but this is not important.
>>>
>>
>> I think if a routine can fail it should return something. And
>> accessing a out-of-range bit is such a case.
>>
>
> Then the return value should also be consistently checked.
Ok, I will add asserts.
>
> [snip]
>
>>> Also, what do you mean with "conspire" (apart from that
>>> it's a typo). "Conspire" basically means to plan something evil or
>>> illegal... I suggest to use another word for whatever you want to
>>> describe.
>>>
>>
>> I agree, with the meaning. As the word "failure" does not carry any
>> moral sense let's tolarate a metaphor which is placed here to make our
>> work less routine more intriguing.
>> Sometimes it's an art to make slave inconsistent :)!
>>
>
> True. :)
>
> [snip]
>
>>>> @@ -7170,43 +7201,48 @@ Write_rows_log_event::do_before_row_oper
>>>> {
>>>> int error= 0;
>>>> - /*
>>>> - We are using REPLACE semantics and not INSERT IGNORE semantics
>>>> - when writing rows, that is: new rows replace old rows. We need to
>>>> - inform the storage engine that it should use this behaviour.
>>>> - */
>>>> -
>>>> - /* Tell the storage engine that we are using REPLACE semantics. */
>>>> - thd->lex->duplicates= DUP_REPLACE;
>>>> + if (is_bit_set(slave_exec_mode, SLAVE_MODE_IDEMPOTENT) != 0 ||
>>>> + m_table->s->db_type()->db_type == DB_TYPE_NDBCLUSTER)
>>>>
>>> Here you're hard-coding for the NBD engine, which is not flexible at
>>> all. I would suggest adding a property to the handler (e.g., a table
>>> flag or a virtual function) and check that instead. That would mean
>>> that other engines that need the same feature can just set that flag
>>> or override that function.
>>>
>>
>> There is a reason in what you are saying. Still, i think we should no rash
>> with generalizing basing on only once existing case, which is it's
>> only NDB that replicates Update on table with primary key as
>> Write_rows event. The feature you are asking would be really welcome
>> as soon as the
>> NDB's method will be acquired by some-engine else and we will have
>> material for thinking more carefully.
>> At this point of time I am not sure if instead we should introduce a flag in
>> Write_rows event to force its applying in replace i.e idempotent
>> fashion.
>> So let's keep it simple as it's done and I will add your todo comments.
>>
>
> Acceptable, even though I think it would be better to add the flag.
ok
>
> [snip]
>
>>>> @@ -88,6 +88,16 @@ TYPELIB delay_key_write_typelib=
>>>> delay_key_write_type_names, NULL
>>>> };
>>>> +const char *slave_exec_mode_names[]=
>>>> +{ "STRICT", "IDEMPOTENT", NullS };
>>>> +static const unsigned int slave_exec_mode_names_len[]=
>>>> +{ strlen("STRICT"), strlen("IDEMPOTENT"), 0 };
>>>>
>>> Please don't use function calls (that is, strlen()) when doing static
>>> initialization. It's unnecessary and it can also cause portability
>>> problems. It is also contrary to how initialization is done
>>> elsewhere. Without having delved deeper into how the lengths are
>>> computed, there's plenty of code to compute the length of these fields
>>> when they are being used, so I think that you don't have to add the
>>> lengths explicitly.
>>>
>>> Could you check to see if this definition works?
>>>
>>> TYPELIB slave_exec_mode_typelib=
>>> {
>>> array_elements(slave_exec_mode_names)-1, "", slave_exec_mode_names,
> NULL
>>> };
>>>
>>>
>>>
>>> If it does, then you can remove the explicit length array. In either
>>> case, don't use strlen() inside a static initializer.
>>>
>>>
>>
>> I don't see reason to check:
>>
>> typedef struct st_typelib { /* Different types saved here */
>> unsigned int count; /* How many types */
>> const char *name; /* Name of typelib */
>> const char **type_names;
>> unsigned int *type_lengths;
>> } TYPELIB;
>>
>> We need type_lengths to reference to lengths.
>>
>
> ... but it is not used elsewhere.
Of course it is!
TYPELIB sql_mode_typelib= { array_elements(sql_mode_names)-1,"",
sql_mode_names,
(unsigned int *)sql_mode_names_len };
where
static const unsigned int sql_mode_names_len[]=
{
/*REAL_AS_FLOAT*/ 13,
...
Imo sql_mode is the closest relative to the slave_exec_mode so that
one of the approches could be to extent sql_mode with the new options.
>
>> I see that all usages of declaring TYPELIB deals with
>> explicit numbers, which is awkward - i believe you agree.
>>
>
> I don't see that. Here are a few excerpts from the server code:
>
> client/mysqladmin.c:
>
> static TYPELIB command_typelib=
> { array_elements(command_names)-1,"commands", command_names, NULL};
>
> client/mysqldump.c:
>
> TYPELIB compatible_mode_typelib=
> {array_elements(compatible_mode_names)-1, "", compatible_mode_names,
> NULL};
>
> client/mysqltest.c:
>
> TYPELIB command_typelib=
> {array_elements(command_names),"", command_names, 0};
>
> sql-common/client.c:
>
> static TYPELIB option_types={array_elements(default_options)-1,
> "options",default_options, NULL};
> TYPELIB sql_protocol_typelib =
> {array_elements(sql_protocol_names_lib)-1,"",
> sql_protocol_names_lib, NULL};
>
> sql/events.cc:
>
> const TYPELIB Events::opt_typelib= {
> array_elements(opt_event_scheduler_state_names)-1, "",
> opt_event_scheduler_state_names, NULL
> };
>
> sql/handler.cc:
>
> TYPELIB tx_isolation_typelib= {
> array_elements(tx_isolation_names)-1,"", tx_isolation_names, NULL
> };
>
>
>
>> To address your concern i shall invoke sizeof() instead :)
>>
>
> Why not stick to the established convention instead?
Well, there is no such convention, or at least there are two of them
:) One needs lengths the other not.
cheers,
Andrei
