Andrei Elkin wrote:
> Mats, hej.
>
> Found yet another mail from you on the 2nd patch.
>
That was mostly informative, it didn't contain anything that were a
problem with your current patch.
[snip]
> I was replaying to this idea. The main point is that idempotency is
> requested de-facto not by engine but rather by an option on the master.
> That is exactly what the ndb's injector does and there is no
> connection of idempotency to the ndb handler.
>
If it is not requested by engine, how come you are checking the
db_type() of the engine? I don't feel this is critical to the patch, so
you can leave it if you feel it is overkill to add it.
> This fact surely could be accounted.
> However, to add a flag to be set on master denoting Write_rows event
> should be applied as replace is somehow troublesome - first of all
> lots of methods would have to change signatures.
> Let us trade it off with a todo I left that encourages such
> changes in future.
>
Yes, that is acceptable.
>
>
>> * I would like to see the .._len array go away, but it's not
>> important enough to prevent pushing.
>>
>>
>
> I answered to this point in the yesterday mail. Even though there would
> not be any use case (and i showed there are), I still don't understand
> what is so harmful in providing the _len array? :)
>
There is nothing harmful, it's just about style. This is why I said that
it's not important enough to prevent pushing (if it were harmful, I
would not accept it).
[snip]
>>> diff -Nrup a/include/my_bitmap.h b/include/my_bitmap.h
>>> --- a/include/my_bitmap.h 2007-07-25 16:29:28 +03:00
>>> +++ b/include/my_bitmap.h 2007-11-26 21:27:47 +02:00
>>> @@ -159,6 +159,23 @@ static inline my_bool bitmap_cmp(const M
>>> #define bitmap_set_all(MAP) \
>>> (memset((MAP)->bitmap, 0xFF, 4*no_words_in_map((MAP))))
>>> +/**
>>> + check, set and clear a bit of interest of an integer.
>>> +
>>> + If the bit is outside of the integer's size results are -1.
>>> + If the bit is inside then
>>> + bit_is_set returns 0 or 1 reflecting the bit is set or not;
>>> + bit_do_set returns 1;
>>> + bit_do_clear returns 0;
>>>
>>>
>> Why? This means that there is inconsistent usage between them.
>>
>
> About consistency: if you set something to 1 e.g a bit
> what do you expect to be the result?
I expect the function to return a 0 value if everything worked, and
something else if there were a failure.
> And vice versa, if you set
> something to 0 e.g a bit ...
>
I expect the function to return a 0 value if everything worked, and
something else if there were a failure.
> In other words what you set that you get, and i find that consistent
> enough.
>
I disagree.
From http://forge.mysql.com/wiki/MySQL_Internals_Coding_Guidelines:
* Functions should return zero on success, and non-zero on error, so
you can do:
if (a() || b() || c())
error("something went wrong");
>> In addition, most users expect the following to behave according to
>> conventions, which are to return non-zero on failure and zero on
>> success.
>>
>
>
>> if (bit_do_set(...)) {
>> /* Error handling code */
>> }
>>
>> if (bit_do_clear(...)) {
>> /* Error handling code */
>> }
>>
>
>
> That'd be a serious overkill. It's all checkable at compilation time.
> I replaced two asserts wrongly added yesterday with a compilation
> assert.
>
Yes? What about:
extern unsigned int x;
extern int y;
bit_do_set(x,y);
>> but it would be OK to return non-zero on success and zero on
>> failure.
>>
>
> How do you find this consistent with bit_is_set()!?
>
> Imo the current rules are pretty simple and fairly consistent.
> Minus 1 is the error. Zero and one are not.
>
It is not consistent with how all other server code is written.
[snip]
>> Still a (null) in the output, which hints at potential portability issues.
>>
>
> ops, stale result file. It's impossible to have (null) by having the
> current def for slave_rows_error_report().
>
I suspected that. Minor issue.
[snip]
>> Maybe use the new and shiny bit_do_set() and bit_do_clear() macros? :)
>>
>>
>
> There must be zillions of such operations all around the code. We'd
> rather stay moderated in ambitions :)
>
OK.
>>> + if (bit_is_set(slave_exec_mode, SLAVE_EXEC_MODE_IDEMPOTENT) == 1)
>>> + {
>>>
>>>
>> From this code, it seems like you should follow the convention that
>> non-zero means some kind of error.
>>
>>
>
> explained above.
>
... and I still think it is wrong because it's not like that anywhere
else in the code.
[snip]
>> Second, db_type is going out in favor of using other means of handling
>> this (hence the name legacy_db_type for that field). By just adding a
>> handlerton flag for this property, you have handled the situation in
>> an extensible manner so that we do not have to make changes in the
>> future. This handlerton flag would be something like HTON_IDEMPOTENT
>> or HTON_ALWAYS_IDEMPOTENT, and the test would then be:
>>
>> bit_is_set(m_table->s->db_type()->flags, HTON_ALWAYS_IDEMPOTENT)
>>
>>
>>
>
> I explained that such flag does not sound as a good idea.
> The best what we could do is to reflect the fact that
> the replace semantic is requested de-facto on master.
>
> About db_type.
>
> There should be some method to check the hanlder's code. Currently,
> it's possible only via db_type. So we have to stick to it.
>
We can handle this later.
>>> + bit_do_set(slave_exec_mode_options, SLAVE_EXEC_MODE_STRICT);
>>>
>>>
>> Not checking return value from bit_do_set() here?
>>
>
> explained.
>
I don't get it. Either bit_do_set() do not return anything, and should
then be treated as a statement (i.e., contain do { ... } while (0)), or
it returns something, and then what it returns should be checked. I
can't really see a good reason to treat different in different context.
Just my few cents,
Mats Kindahl
--
Mats Kindahl
Lead Software Developer
Replication Team
MySQL AB, www.mysql.com
