[Commits] Review comments for MDEV-6112 multiple triggers per table

Vicențiu Ciorbaru vicentiu at mariadb.org
Wed Sep 28 19:30:57 EEST 2016


Hi Monty!

Thanks for looking at this review.

Vicen> 1. I feel that "created" as a column name is rather vague. I'd
> expect that
> Vicen> column to be a BOOLEAN instead of a time value. Why not name it
> Vicen> "created_at" or "date_created" or "creation_time(date)"?
>
> The reason for 'created' is that what MySQL 5.7 is using. I prefer to
> try to keep things as compatible as possible between MariaDB


Elena also pointed this out. In that case I agree that it's best not to
diverge from MySQL
too much.


Vicen> 3. Regarding Item_timestamp_literal and Item_datetime_literal and (at
> Vicen> least) it's parent Item_temporal_literal:
>
> Vicen> The parameter MYSQL_TIME is hard copied within the constructor of
> Vicen> Item_temporal_literal. To ensure a bit of extra type safety, you
> can add
> Vicen> the const parameter to (MYSQL_TIME *) argument.
>
> We concluded with Bar that we don't need Item_timestamp_literal after
> all.
>
> In MySQL 5.7, SHOW CREATE TRIGGER shows created as a TIMESTAMP, but
> when you do SHOW TRIGGERS or use information_schema, Created is a DATETIME.
> This is probably a bug and we decided to keep it always as a DATETIME
> and file a bug report regarding SHOW CREATE TRIGGER to Oracle.
>
> Vicen> I personally would make it a const reference (const MYSQL_TIME &)
> in the
> Vicen> constructor of Item_temporal_literal. This way you force the caller
> to
> Vicen> always provide a valid entry when creating the object (NULL would
> crash the
> Vicen> server anyway).
>
> I personally don't like const references, because of couple of
> reasons:
> - It's very hard to see in the function if you are passing a full
>   struct or a reference.  I prefer to know when using an object or a
>   pointer.
> - const is useful for a lot of cases, but causes a lot of problems in
>   C++ if used too much as you can't pass an object that is partly
>   const to a function that takes const.  Having to add a lot of
>   casting from const and to const just to avoid this is problematic.
>   (See LEX_CSTRING as an example when C++/C makes it harder to use
>    const instead of easier).
>
> Note that I am more against references than using const...
>

Indeed, this is a delicate matter. You raise valid points. I don't disagree
that it should be easy to tell the difference between pointers and objects.
But for the use cases that I propose references for, that doesn't matter.

I'll explain my general thought process on this. The only use case that I
approve of references is when they are const references. Non-const
references are a headache, exactly due to the point you bring up, that it's
hard to know if you're passing a full struct or a pointer, if copies happen
or not, if you're changing a local value or affecting callers, etc.
Whatever I say next assumes that we're only allowing const references, and
only in the form of input parameters to functions. I also think that
functions should rarely accept full structures (non-pointers) as
parameters. You basically make the caller create the structure once and
also _copy_ it again so you can use it yourself, only to have it be lost
when you finish your execution.

Now, why add references to the mix when we can be using pointers instead?
The problem with only using pointers is that it's hard to figure out which
parameters for a function are supposed to be input ones and which are
supposed to be output ones or even both input and output ones. Comments are
sometimes not kept up to date. The beauty of const references is that they
can only be used as input parameters, if we stick to not exposing object
innards directly (more on this later). Const pointers generally share the
same benefits, but you have an extra question to ask yourself: Do I own the
object now or not? There are quite a few cases where the answer is not
obvious. With references, you are sure that you are not the owner, that all
you have to care about is making use of it. Cleanup will be the callers job.

Regarding LEX_STRING and LEX_CSTRING, I think those two are quite
unfortunate examples for when to use const and when not to. The naked
pointers to buffers are the main cause for this problem.

Here's an example to prove my point:

/* Function found in m_string.h */
inline void lex_string_set(LEX_STRING *lex_str, const char *c_str)
{
  lex_str->str= (char *) c_str;
  lex_str->length= strlen(c_str);
}

So with this function, I could do the following:

LEX_STRING *lex_str = new LEX_STRING();
....
/* some code */
....
lex_string_set(lex_str, "some string");
....
/* lots of convoluted code */
....
if (str->length > 2)
{
    str->length = 2;
    str->str[2] = '\0'; // Crashes on a perfectly valid operation.
}

Basically, unless you know _exactly_ how this object was created you have
no confidence if the str member is actually a const string or not. You can
even do it the other way around. With a const LEX_STRING, you can still do
lex_string->str[2] = '\0'. This happens a lot in our code base and it is
sometimes hard to track down all side effects that functions have. That's
why I would like to encourage const usage as much as possible and when
possible, const-references. I know our mem_root approach sometimes makes
cleaning up not-necessary, but not all the time.

There's obviously more nuances to this, but this is my main argument for
const references. I would like to discuss this in a bit more detail during
Amsterdam.



>
> Vicen> Making it a const pointer would only involve changing
> Vicen> Item_temporal_literal's, Item_datetime_literal and
> Item_timestamp_literal's
> Vicen> constructor definitions.
>
> Vicen> Making it a const reference would involve changing the call sites
> for these
> Vicen> constructors as well. I think this is the best option type-safety
> wise, but
> Vicen> it requires potentially more changes around the codebase. We're
> touching
> Vicen> this area a bit anyway so perhaps it's worth it to do it now?
>
> In this patch I am not touching this area at all.
>

Right, should have worded this a bit differently. "If we go with changing
it to a const reference, then it might make sense to do these changes as
well now." I guess for the time being things stay the same.


>
> Vicen> 4. The new function trim_whitespace with an extra parameter creates
> some
> Vicen> very ugly constructs where you have to define extra "unused"
> variables,
> Vicen> just to call the function. Alternatives:
>
> Vicen> a) A wrapper around it. The wrapper defines the unused variable. We
> still
> Vicen> do the extra work with the unused variable however.
>
> Vicen> b) 2 separate definitions. The function is not so long that it makes
> Vicen> maintaining it a burden.
> Vicen> c) Combine the two approaches but make the function accept NULL as a
> Vicen> parameter for it?
>
> As this function is only used in 9 places, I don't think that the
> extra, often useful parameter, is such a big burden.  Having several
> versions of the same function is a bigger burden.
>
> If it would be a function that would be used in a lot of places, then
> things would be different.
>
> I prefer not to have wrappers for simple things, as then you would not
> know the cost of calling the function when you use it. Now it's quite
> clear for any user of the function that 3 elements are passed and the
> cost involved with that.
>

I agree that this use case is narrow (for now) and the cost is not that big
of a problem. Still, right now we have 6 calling sites and only 1 makes use
of that extra parameter. For one use case, we're adding extra cost in 5
other places. If you think that 2 separate definitions for this function
would be more expensive than adding the extra unused parameters, I don't
have that strong of an opinion about it, I just don't like this "unused
parameter" construct.

In the end, if we end up using this a lot and in performance sensitive
code, we could implement separate functions down the line.

Regards,
Vicențiu
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.askmonty.org/pipermail/commits/attachments/20160928/3eea0733/attachment-0001.html>


More information about the commits mailing list