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

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

Hi Monty!

I've looked at your patch for MDEV-6112 (out of curiosity mostly). Although
I did not spend too much time to understand all the details, it was
generally easy to follow. I have a few stylistic comments mostly. I haven't
pasted the whole commit as it's rather long and there's no need for it I

1. I feel that "created" as a column name is rather vague. I'd expect that
column to be a BOOLEAN instead of a time value. Why not name it
"created_at" or "date_created" or "creation_time(date)"?

2. In  sql/sql_trigger.cc:361
 +  @return # Something went wrong. Pointer to the trigger that mailfuncted

3. Regarding Item_timestamp_literal and Item_datetime_literal and (at
least) it's parent Item_temporal_literal:
The parameter MYSQL_TIME is hard copied within the constructor of
Item_temporal_literal. To ensure a bit of extra type safety, you can add
the const parameter to (MYSQL_TIME *) argument.

I personally would make it a const reference (const MYSQL_TIME &) in the
constructor of Item_temporal_literal. This way you force the caller to
always provide a valid entry when creating the object (NULL would crash the
server anyway).

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

4. The new function trim_whitespace with an extra parameter creates some
very ugly constructs where you have to define extra "unused" variables,
just to call the function. Alternatives:
a) A wrapper around it. The wrapper defines the unused variable. We still
do the extra work with the unused variable however.
b) 2 separate definitions. The function is not so long that it makes
maintaining it a burden.
c) Combine the two approaches but make the function accept NULL as a
parameter for it?

Let me know what your thoughts are on these.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.askmonty.org/pipermail/commits/attachments/20160928/916c8166/attachment.html>

More information about the commits mailing list