PLSQL Oracle Cursor in procedure

Question

currently I am learning PLSQL, using Oracle. I am trying to get data which will be older than PARAM days decalred in another table. I want the procedure to take all the data, check if some records are older (recv_date) than parameter from param_value and if yes than fire my alarm procedure. I have problem with declaring a CURSOR and ln_pallets_container. I know I could somehow get into ln_pallets data only WHERE the recv_date I already filtered but neither here I have no idea how to do it correctly. Maybe I should declare cursor before procedure and not inside of it?

  procedure CHECK_STOCK_DATE(warehouse_id_in IN warehouse.warehouse_id%TYPE)
IS
ln_pallet_count NUMBER;
ln_days_till_expiration param_value.param_value%TYPE;

CURSOR ln_pallets IS
       SELECT container_id, recv_date
       FROM wms_stock ws

ln_pallets_container%ROWTYPE;


BEGIN

       OPEN ln_pallets;
            LOOP
                FETCH ln_pallets INTO ln_pallets_container;
                EXIT WHEN ln_pallets%NOTFOUND;

                SELECT param_value.param_value
                INTO ln_days_till_expiration
                FROM param_value
                WHERE param_value.parameter_id = 266;

                   IF(ln_pallets_container.recv_date >= trunc(sysdate - ln_days_till_expiration)

                      ALARM.ALARM(WAREHOUSE_ID =>MY_COMMONS.GET_WHRS_ID,
SOURCE_TEXT => ln_pallets_container.container_id,
MESSAGE_CODE => 'Cannot find this container on warehouse. Check container code.');
                  END IF;
            END LOOP;
        CLOSE ln_pallets;
END;

Answer 1

There are several things wrong with your code, which I've fixed and highlighted in the following:

PROCEDURE check_stock_date(warehouse_id_in IN warehouse.warehouse_id%TYPE) IS
  ln_pallet_count         NUMBER;
  ln_days_till_expiration param_value.param_value%TYPE;

  CURSOR ln_pallets IS
    SELECT container_id,
           recv_date
    FROM   wms_stock ws; -- added semicolon

  ln_pallets_container ln_pallets%ROWTYPE; -- amended to set the datatype of the variable to be the cursor rowtype

BEGIN

  OPEN ln_pallets;
  LOOP
    FETCH ln_pallets
      INTO ln_pallets_container;
    EXIT WHEN ln_pallets%NOTFOUND;

    SELECT param_value.param_value
    INTO   ln_days_till_expiration
    FROM   param_value
    WHERE  param_value.parameter_id = 266;

    IF /*removed bracket*/ ln_pallets_container.recv_date >= trunc(SYSDATE - ln_days_till_expiration)
    THEN --added

      alarm.alarm(warehouse_id => my_commons.get_whrs_id,
                  source_text  => ln_pallets_container.container_id,
                  message_code => 'Cannot find this container on warehouse. Check container code.');
    END IF;
  END LOOP;
  CLOSE ln_pallets;
END check_stock_date;

However, this could be done much more efficiently. Currently, you are looping through all the rows in wms_stock, plus you are explicitly opening, fetching and closing the cursor yourself.

That means for every row in wms_stock, you are finding the value of parameter_id 266 (which I assume won't change whilst you're looping through the results!), as well as doing the check to see if you can run your alarm procedure.

Instead of fetching all the rows, why not move the check into the cursor - that way, you'll only be fetching the parameter 266 value once and filtering out any rows that don't need to have the alarm procedure run.

At the same time, why not switch to using a cursor-for-loop? That way, you don't have to worry about opening/fetching from/closing the cursor, as Oracle handles all that for you.

Doing that will result in far less code, which happens to be more efficient and easier to read and maintain, like so:

PROCEDURE check_stock_date(warehouse_id_in IN warehouse.warehouse_id%TYPE) IS 
BEGIN
  FOR ln_pallets_rec IN (SELECT container_id,
                                recv_date
                         FROM   wms_stock ws
                         WHERE  recv_date >= (SELECT trunc(SYSDATE - param_value.param_value
                                              FROM   param_value
                                              WHERE  param_value.parameter_id = 266))
  LOOP
    alarm.alarm(warehouse_id => my_commons.get_whrs_id,
                source_text  => ln_pallets_rec.container_id,
                message_code => 'Cannot find this container on warehouse. Check container code.');
  END LOOP;

END check_stock_date;