maria-developers team mailing list archive

[Alexander Barkov <bar@xxxxxxxxxxx>]

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:
SELECT FROM_UNIXTIME(UNIX_TIMESTAMP(CREATED)*100)
FROM INFORMATION_SCHEMA.triggers;

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');
...
SET TIMESTAM=DEFAULT;


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.


Greetings.