5
\$\begingroup\$

Multiple upstream servers are sending notifications to a load-balanced django app. If all the upstream servers are working correctly, the app will always receive duplicates of these notifications (since all the upstream notifiers should send the same notifications; it's just for redundancy). I want to be able to filter out these duplicates. However, since the python app is load-balanced, the only place we can check for these duplicates is in the database (unless we did distributed locking, etc, which sounds like more of a pain).

To do this, I'm hashing the messages, then invoking a stored procedure in the database that checks if a message received in the last 10 seconds had the same hash (I know about hash collisions; it's a risk). I want to be 99% sure the store procedure is safe against race conditions.

Here's the SQL code that seems to work:

DROP TABLE IF EXISTS openduty_dedup;
CREATE TABLE openduty_dedup (
  request_hash CHAR(40) NOT NULL UNIQUE PRIMARY KEY,
  triggered TIMESTAMP NOT NULL);

-- main procedure for checking if a notification is a duplicate
-- given the SHA-1 hash of a notification, it will return 1 if the
-- same notification was received in the last 10 seconds (and was not
-- treated as a duplicate then), otherwise it will return 0
DROP PROCEDURE IF EXISTS openduty_check_duplicate;
CREATE PROCEDURE openduty_check_duplicate(IN new_hash CHAR(40))
  BEGIN
    DECLARE last_trigger TIMESTAMP;
    DECLARE now TIMESTAMP;
    DECLARE is_duplicate INT;
    SELECT CURRENT_TIMESTAMP INTO now;
    DO GET_LOCK('openduty_check_duplicate', 10);
      SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;
      START TRANSACTION;
        SELECT triggered FROM openduty_dedup WHERE request_hash = new_hash INTO last_trigger;
        IF last_trigger IS NULL THEN
          INSERT INTO openduty_dedup (request_hash, triggered) VALUES (new_hash, now);
          SELECT 0 INTO is_duplicate;
        ELSEIF DATE_SUB(now, INTERVAL 10 SECOND) > last_trigger THEN
          UPDATE openduty_dedup SET triggered = now WHERE request_hash = new_hash;
          SELECT 0 INTO is_duplicate;
        ELSE
          SELECT 1 INTO is_duplicate;
        END IF;
        DELETE FROM openduty_dedup WHERE DATE_SUB(now, INTERVAL 1 MINUTE) > triggered;
      COMMIT;
    DO RELEASE_LOCK('openduty_check_duplicate');
    SELECT is_duplicate;
  END;

I'm much more worried about correctness than performance since notifications will only come on the order of every few seconds. In particular, I'm worried that the commit won't happen until after the lock is released.

Will this work as intended?

\$\endgroup\$
2
\$\begingroup\$

Will this work as intended?

Not really.

You've indented the code as if you expect that the statements betweeb DO GET_LOCK... and DO RELEASE_LOCK... would only execute if the lock is obtained.

That is not the case.

You shouldn't use GET_LOCK() in a void context (with DO) unless you don't actually care whether you actually got the lock. What you have written will wait up to 10 seconds for the lock, and if it isn't available, continues anyway with the next statement. That does not sound like what you want.

A better solution for MySQL 5.5 and later looks like this:

IF GET_LOCK('my_lock',10) IS NOT TRUE THEN
  SIGNAL SQLSTATE '45000'
         SET MESSAGE_TEXT = 'timed out waiting for named semaphore lock';
END IF;

That will throw an exception and terminate the procedure after the timeout, if the lock has not been obtained. (This is a useful tactic with the event scheduler, when you need to ensure that concurrent invocations of events don't pile up.)

Note that the logical operator IS NOT TRUE, used above, matches both 0 (FALSE) and NULL, either of which can indicate that you didn't get the lock.

The same is true of RELEASE_LOCK(), which returns TRUE only if you actually held the lock you tried to release. If you try to release a lock you don't own, that does not throw an error. You have to check return values.

In particular, I'm worried that the commit won't happen until after the lock is released.

No, the lock is released after the commit, here. The get and release operations are only functions. They execute in sequence like everything else.

Also, it's usually best to leave the caller in charge of the transaction, because if this procedure is called while already in a transaction, START TRANSACTION silently commits the previous transaction and starts a new one, leaving the caller unable to roll back any work done before the procedure was called. That may not be applicable, here, but it's good practice to handle it consistently across your code base.

\$\endgroup\$
1
\$\begingroup\$

This honestly seems like a hackish and fragile solution to a poorly implemented workflow. Why should multiple servers be sending duplicate notifications in the first place? A proper notification/queueing tool might help here.

At any rate, it might be simpler to put a unique index on the hash field and just silently discard failed duplicate inserts, than insert multiple copies of each record and then lock up this table everytime you want to de-dupe.

\$\endgroup\$
  • \$\begingroup\$ The notifications aren't inserted directly into the database in any other way (sometimes they might trigger something else happening to the database, depending on state, etc). The application, not the database, needs to know if this is a duplicate or not. The upstream nodes are in different availability zones that don't know anything about each other, and there's not much we can do about that (or want to, since the whole point of this system is to notify us when something goes wrong). \$\endgroup\$ – Robert Fraser Feb 26 '17 at 19:34

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.