← Back to team overview

maria-developers team mailing list archive

Review for MDEV-6112 multiple triggers per table


Hello Monty.

I checked your patch for MDEV-6112 multiple triggers per table.

Here are some quick notes. I'll send a detailed review separately.

1. It would be nice to push refactoring changes in separate patch:

  a. do sql_mode_t refactoring
  b. "trg_idx -> Trigger *" refactoring
  c. opt_debugging related things

2. Why not to reuse the FOLLOWING_SYM and PRECEDING_SYM keywords
   instead of adding new ones FOLLOWS_SYM, PRECEDES_SYM ?
   (this is a non-standard syntax anyway)

3. I'd prefer PRECEDES/FOLLOWS to go before FOR EACH ROW.
"FOR EACH ROW stmt" is a kind of single clause responsible for the action.
It's very confusing to have PRECEDES/FOLLOWS characteristics in between
"FOR EACH ROW" and stmt.

4. You have a lot of changes related to trim_whitespace() where you declare
an unused "prefix_removed" variable. It would be nice to avoid declaring it.

Possible ways:

a. Perhaps the new function version can be defined like this:

size_t trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str);

and make it return "prefix length removed", instead of adding a new parameter.

b. Another option is just to overload it:

extern void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str,
                            uint *prefix_removed);
inline void trim_whitespace(CHARSET_INFO *cs, LEX_STRING *str)
  uint not_used;
  trim_whitespace(cs, str, &not_used);

5. The "CREATED" field is not populated well. I'm getting strange values
like 1970-04-24 09:35:00.30.

Btw, his query returns a correct value:

You must have forgotten to multiply or delete to 100 somewhere.

6. In replication tests it's better to make sure that CREATED is safely
replicated (instead of changing it to #).

By the way, in other tests it's also a good idea not to use # for CREATED.
(see the previous problem)

You can use this:
SET TIMESTAMP=UNIX_TIMESTAMP('2001-01-01 10:20:30');

7. I suggest to move this code into a method, say find_trigger():

+  if (table->triggers)
+  {
+    Trigger *t;
+    for (t= table->triggers->get_trigger(TRG_EVENT_INSERT,
+                                         TRG_ACTION_BEFORE);
+         t;
+         t= t->next)
+      if (t->is_fields_updated_in_trigger(&full_part_field_set))
+        DBUG_RETURN(false);
+  }

It's repeated at least two times.

8. The following structure fields are defined two times:

  enum trigger_order_type ordering_clause;
  LEX_STRING anchor_trigger_name;

The first time for st_trg_chistics.
The second time for %union in sql_yacc.yy
Note, there will be the third time in sql_yacc_ora.yy.
Please define a structure in sql_lex.h instead:

struct st_trg_execution_order_chistics
    FOLLOWS or PRECEDES as specified in the CREATE TRIGGER statement.
  enum trigger_order_type ordering_clause;

    Trigger name referenced in the FOLLOWS/PRECEDES clause of the
    CREATE TRIGGER statement.
  LEX_STRING anchor_trigger_name;

and reuse it in here:

struct st_trg_chirstics: public st_trg_execution_order_chistics

and in here:

%union {
  st_trg_execition_order_chistics trg_execution_order_chirstics;

Please also rename trg_characteristics to trg_execution_order_chistics,
because "trg_characteristics" is more suitable to something having type
st_trg_chirstics rather than st_trg_execution_order_chistics.


Follow ups