|
Analysis of the patches above. They change code in three main areas:
= the server:
- the default nullability of TIMESTAMP columns,
- the default assumption that the first TIMESTAMP should be autoupdated on INSERT/UPDATE
- non-NULLable TIMESTAMP columns without explicit default are treated as having no default
=> This is very easy to implement ~half a day
= replication:
"This variable is used by User thread and
as well as by replication slave applier thread to apply relay_log.
Slave applier thread enables/disables this option based on
relay_log's from replication master versions. There is possibility of
slave applier thread and User thread to have different setting for
explicit_defaults_for_timestamp"
=> The change seems quite big, and I am not an expert. Kristian disliked the idea
of the patch. I don't know replication, so I cannot evaluate the quality of this new code.
= tests
Many tests have been adjusted to include the previously implicit TIMESTAMP
properties as explicit part of the create table statement. I don't understand for sure
why this was done, as the default is the old behavior. My guess is that in this way
the test cases would not have to be changed in the future, once the new behavior
becomes default.
=> Since there are many test files, and even more test results that need to be
verified, adapting/changing tests would take about 1/2 - 1 day.
|
|
In an IRC discussion Kristian was against backporting this feature for the following reasons (edited IRC discussion):
<timour> From the code and comments, this is what I understand -
<timour> during replication the slave checks this option on the master.
<timour> and adjusts the option on the slave so that it is compatible.
<knielsen> oh, that sounds broken 
<knielsen> because one can stop master, change the option, start master, and then slave can connect and read both old and new events from master (made with different value for the option)
<timour> this is the main goal of the patch -
<timour> add a new system variable that:
<timour> - makes timestamp columns NULL by default (unlike now, they are NOT NULL)
<timour> - do not autoupdate the first timestamp column (now it is assumed to be auto-updated)
<timour> - also change non-NULL-able timestamp columns have no assumed default 0
<knielsen> any particular reason for us to merge that system variable? Sounds to me it adds more complexity for little gain?
<timour> this variable deprecates old non-standard MySQL behavior with respect to timestamp fields.
<timour> so if we don't merge it, we will be non-compatible in the next version.
<knielsen> I'd say MySQL 5.7 (or whatever) will be non-compatible with older MySQL, while we will be compatible?
<knielsen> this deprecation sounds like a reasonable idea in theory, but in practice, it seems not to work so well, old applications start breaking when deprecated stuff is removed, even if it was deprecated for 10 years or whatever
<knielsen> so basically, this option, which is non-default, sounds like something I'd suggest not to be merged. But I didn't look closely, maybe I misunderstood
<timour> I personally think that making timestamp behave as other fields is a good thing.
<knielsen> yes, that would be good, but do you see a realistic way forward to change it without breaking too much existing applications?
<timour> the problem for me is that the main part of the code related to this variable is replication, and I cannot evaluate this change.
<timour> well, for now its off by default, so nothing should break
<knielsen> it's > 6k lines of code, so it's not something anyone can evaluate at a glance ..
<knielsen> yes, now it's off. But it seems to me we could never make it on by default?
<timour> the problem will come when it gets ON by default, then users can switch it OFF, this is the only way I can see.
<knielsen> deprecation usually works like this: there is some feature that does something odd, we want to remove it. So we make it give a deprecation warning for some years, then we remove it, assuming applications have been changed to use the recommended syntax/feature
<knielsen> even this is somewhat doubtfull in my mind ... apps don't change, is the experience
<knielsen> but it seems to me - this is something that changes behaviour? So there is no "bad" old syntax, rather the meaning of reasonable syntax is changed
<knielsen> so if I have an app, what should I do? How can I change my app so it works correctly with the deprecated feature, and also will work correctly with the new feature?
<knielsen> it seems I cannot, except to always specify TIMESTAMP explicitly as NOT NULL or NULL, as I need, and explicitly specifying a DEFAULT.
<knielsen> and then the option is in any case pointless, as apps anyway have to avoid any syntax affected by the option
<timour> you can change the app by not relying on the implicit default behavior - for each TIMESTAMP column, declare explicitly if its NULL or not, and if has a DEFAULT or not.
<knielsen> so the "correct" way to deprecate would seem to be to first deprecate using TIMESTAMP without explicit NULLability and default - and then make it an error - and then later re-add the ability with the new mechanism
<knielsen> which seems just too much bother
<knielsen> if we really want this, then better introduce TIMESTAMP2 or something like that with correct semantics, and retain old TIMESTAMP behaviour for compatibility
<knielsen> Just to be clear: I've no love for current TIMESTAMP complex/non-standard semantics, but I don't see adding some even more complex option semantics improves things ...
|