Trigger optimisation

Question

How can I optimise this trigger? It works perfectly, but I think it can be improved:

CREATE TRIGGER `update_request_missions_after_insert` BEFORE INSERT ON `request_missions`
 FOR EACH ROW
IF ((SELECT id FROM request_missions ORDER BY id DESC LIMIT 1)+1) >= 1  AND 
   ((SELECT id FROM request_missions ORDER BY id DESC LIMIT 1)+1) <= 9 THEN
        SET New.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%y'), '-',
            CONCAT('00000',(SELECT id FROM request_missions ORDER BY id DESC LIMIT 1)+1));
    END IF

The results will be in the rm_number column, like:

for id >=1 and id <= 9

Welcome to StackOverflow! Be aware that questions about code improvement -- when the code is working -- are more suitable to be asked on [CodeReview](https://codereview.stackexchange.com/). — trincot, Jan 20 '19 at 09:37
I don't believe this does work - id = 1 will acquire null since the before trigger cannot possibly know that id 1 will be allocated for the first insert because it is allocated after the before trigger. Also anything with an id > 8 will acquire null. Anything between 2 and 8 (inclusive) will be allocated a number which matches the id(ie 19-000002 to 19-000008). The only safe way to do what you appear to want, imo, is do this with a separate update statement after the inserts possibly as part of a transaction. — P.Salmon, Jan 20 '19 at 12:53
Thank you P.Salmon , Ok, I have updated the code above , my concern is how to replace the expression (SELECT id FROM request_missions ORDER BY ID DESC LIMIT 1) +1) by another shorter like a variable for exemple. — Nouar, Jan 20 '19 at 19:16

Bill Karwin · Answer 1 · 2019-01-21T16:08:46.310

Here's a suggestion for using a local variable instead of repeating your subquery three times. You said that was your concern.

CREATE TRIGGER `update_request_missions_after_insert` BEFORE INSERT ON `request_missions`
FOR EACH ROW
BEGIN
  DECLARE next_id INT;
  SELECT COALESCE(MAX(id),0) + 1 INTO next_id FROM request_missions;
  IF next_id BETWEEN 1 AND 9 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%y'), '-', '00000', next_id);
  END IF;
END

Warning: this suffers from a race condition. For example, if two sessions are inserting to the table at the same time, they might both read the same next_id.

Also as @P.Salmon commented above, the results are undefined if next_id is > 9. I'm not sure what you're trying to do.

You might want to allow longer numbers, and pad them to a fixed length with zeroes:

LPAD(next_id, 6, '0')

See the manual on the LPAD() function.

I used COALESCE(MAX(id),0) to make sure at least there's one non-NULL value returned, in case the table has zero rows.

This is also concerning:

DATE_FORMAT(CURDATE(),'%y')

A two-digit year will roll over to 00 in 81 years. This is exactly how the Y2K problem was created! I know you won't be working on the code by then, so you might not care. But you should always code as if the person who ends up maintaining your code is a violent psychopath who knows where you live.

Re your new code posted in another answer:

This is a better solution for handling the next_id numbers up to 999,999.

CREATE TRIGGER `update_request_missions_after_insert` BEFORE INSERT ON `request_missions`
FOR EACH ROW
BEGIN
  DECLARE next_id INT;
  SELECT COALESCE(MAX(id),0) + 1 INTO next_id FROM request_missions;
  IF next_id < 1000000 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%y'), '-', LPAD(next_id, 6, '0'));
  ELSE
    SIGNAL SQLSTATE '45000' SET MESSAGE_TEXT = 'RM Number has reached capacity'
  END IF;
END

The race condition issue is different, and there's no solution while you are using MAX(id) to find the last id. The problem is that you might have more than one client inserting to the table concurrently. Each client's session will read the same value for MAX(id) and use it.

You should read about Race Conditions.

What you really need is some way of using an AUTO_INCREMENT value that generates new unique values in a thread-safe manner, and copy the value into your rm_number.

Unfortunately, there's no way to do this in a trigger. During a BEFORE INSERT trigger, the new AUTO_INCREMENT value is not generated yet. During an AFTER INSERT trigger, you can't modify NEW.rm_number because the insert is already done. I wrote about this in the past:

Thank's M. Bill Karwin the trigger Work perfectly I Just a added `SQLSTATE` to ` `SIGNAL '45000' SET MESSAGE_TEXT = 'RM Number has reached capacity' ` I think it is not wrong !! — Nouar, Jan 21 '19 at 16:03
Yes, sorry, I forgot `SQLSTATE`. I've corrected the example in my answer. — Bill Karwin, Jan 21 '19 at 16:09
However, I still get the impression you don't understand the race condition issue. That is NOT solved by my example. — Bill Karwin, Jan 21 '19 at 16:09
I think i have understand the Race Condition, but you have said there is no solution for this issue with triggers. berhaps updating the value of rm_ number after inserting data is the solution ? what you think ? — Nouar, Jan 21 '19 at 16:20
I'm not sure what you mean. I provided two links at the end of my answer. — Bill Karwin, Jan 21 '19 at 20:22
Ok that's right Thank you very mutch M. Bill Karxin for all helps — Nouar, Jan 21 '19 at 20:26
M. Bill , Finaly i decided to resolve my problem out side the database using PHP Laravel framwork. and i solved it — Nouar, Jan 23 '19 at 09:43

Nouar · Accepted Answer · 2019-01-21T11:28:53.833

Thanks M. Bill Karwin the code is now more clean For the problème of year I changed it to '%Y' . The code is now :

CREATE TRIGGER `update_request_missions_after_insert` BEFORE INSERT ON `request_missions`
FOR EACH ROW
BEGIN
  DECLARE next_id INT;
  SELECT COALESCE(MAX(id),0) + 1 INTO next_id FROM request_missions;
  IF next_id BETWEEN 1 AND 9 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', '00000', next_id);
  ELSEIF next_id BETWEEN 10 AND 99 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', '0000', next_id);
  ELSEIF next_id BETWEEN 100 AND 999 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', '000', next_id);
  ELSEIF next_id BETWEEN 1000 AND 9999 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', '00', next_id);
  ELSEIF next_id BETWEEN 10000 AND 99999 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', '0', next_id);
  ELSEIF next_id BETWEEN 100000 AND 999999 THEN
    SET NEW.rm_number = CONCAT(DATE_FORMAT(CURDATE(),'%Y'), '-', next_id);
  END IF;
END

Warning: this suffers from a race condition. For example, if two sessions are inserting to the table at the same time, they might both read the same next_id.

Is there a solution to this issue, knowing that the purpose of this trigger is to add a number in the format 'YYYY-ID' for each line added. YYYY: is the current Year ID : is the id of the current row formated as shown in the trigger

One solution to race condition is to lock the table before you INSERT, so there cannot be two concurrent sessions trying to insert. — Bill Karwin, Apr 01 '20 at 13:54
Another solution is to use another table or something outside the database to produce incrementing numbers. — Bill Karwin, Apr 01 '20 at 13:54

Trigger optimisation

2 Answers2