MySQL stored procedure, handling multiple cursors and query results

Question

How can I use two cursors in the same routine? If I remove the second cursor declaration and fetch loop everthing works fine. The routine is used for adding a friend in my webapp. It takes the id of the current user and the email of the friend we want to add as a friend, then it checks if the email has a corresponding user id and if no friend relation exists it will create one. Any other routine solution than this one would be great as well.

DROP PROCEDURE IF EXISTS addNewFriend;
DELIMITER //
CREATE PROCEDURE addNewFriend(IN inUserId INT UNSIGNED, IN inFriendEmail VARCHAR(80))
BEGIN
    DECLARE tempFriendId INT UNSIGNED DEFAULT 0;
    DECLARE tempId INT UNSIGNED DEFAULT 0;
    DECLARE done INT DEFAULT 0;

    DECLARE cur CURSOR FOR
        SELECT id FROM users WHERE email = inFriendEmail;
    DECLARE CONTINUE HANDLER FOR NOT FOUND SET done = 1;

    OPEN cur;
    REPEAT
        FETCH cur INTO tempFriendId;
    UNTIL done  = 1 END REPEAT;
    CLOSE cur;

    DECLARE cur CURSOR FOR 
        SELECT user_id FROM users_friends WHERE user_id = tempFriendId OR friend_id = tempFriendId;
    DECLARE CONTINUE HANDLER FOR NOT FOUND SET done = 1;

    OPEN cur;
    REPEAT
        FETCH cur INTO tempId;
    UNTIL done  = 1 END REPEAT;
    CLOSE cur;

    IF tempFriendId != 0 AND tempId != 0 THEN
        INSERT INTO users_friends (user_id, friend_id) VALUES(inUserId, tempFriendId);
    END IF;
    SELECT tempFriendId as friendId;
END //
DELIMITER ;

Answer 1

Wow, i don't know what to say, please go and read about and learn sql a little, no offense but this is amongst the worst SQL i've ever seem.

SQL is a set based language, cursors, in general, are bad, there are situations when they are usefull, but they are fairly rare. Your use of cursors here is totally inappropriate.

Your logic in the second cursor is also flawed since it will select any record which inludes the friend, not just the required friendship.

If you wanted to fix it you could try giving the second cursor a differant name, but preferably start over.

Set a compound PK or unique constraint on users_friends, then you don't have to worry about checking for a relationship, then try something like this.

INSERT INTO users_friends 
SELECT 
    @inUserId, 
    users.user_id
FROM 
    users
WHERE
    email = @inFriendEmail

Answer 2

Rather than using cursors to check for the existence of records, you can use the EXISTS clause in the WHERE clause:

INSERT INTO users_friends 
  (user_id, friend_id) 
VALUES
  (inUserId, tempFriendId)
WHERE EXISTS(SELECT NULL 
               FROM users 
              WHERE email = inFriendEmail)
  AND NOT EXISTS(SELECT NULL 
                   FROM users_friends 
                  WHERE user_id = tempFriendId 
                    AND friend_id = tempFriendId);

I made an alteration after reading Paul's comments about the second query, and reversed the logic so the insert won't add duplicates. Ideally this should be handled as a primary key being a compound key (including two or more columns), which would stop the need for checking in code.

Answer 3

I have finally written a different function that does the same thing:

DROP PROCEDURE IF EXISTS addNewFriend;
DELIMITER //
CREATE PROCEDURE addNewFriend(IN inUserId INT UNSIGNED, IN inFriendEmail VARCHAR(80))
BEGIN
 SET @tempFriendId = (SELECT id FROM users WHERE email = inFriendEmail);
 SET @tempUsersFriendsUserId = (SELECT user_id FROM users_friends WHERE user_id = inUserId AND friend_id = @tempFriendId);
 IF @tempFriendId IS NOT NULL AND @tempUsersFriendsUserId IS NULL THEN
  INSERT INTO users_friends (user_id, friend_id) VALUES(inUserId, @tempFriendId);
 END IF;
 SELECT @tempFriendId as friendId;
END //
DELIMITER ;

I hope this is a better solution, it works fine anyway. Thanks for telling me not to use cursors when not necessary.

Answer 4

I know you found a better solution, but I believe the answer to your original question is that you need to SET Done=0; between the two cursors, otherwise the second cursor will only fetch one record before exiting the loop due to Done=1 from the previous handler.