Can anyone help me with my pl/sql script to update/merge in the database

Question

I made a cursor in order to merge/update records in the database

I was wondering if it is correct or if anyone has any suggestions in order to improve the query.

DECLARE
CURSOR c_itemloc
IS
SELECT 
    item ,
    loc ,
    loc_type ,
    source_method ,
    primary_supp ,
    source_wh
FROM
  (SELECT dc_vert.item ,
    dc_vert.loc ,
    dc_vert.loc_type ,
    dc_vert.source_method ,
    dc_vert.primary_supp ,
    w.primary_vwh source_wh --,dc_vert.source_wh
    ,
    dc_vert.actie ,
    MAX(dc_vert.actie) over (PARTITION BY dc_vert.item, dc_vert.loc) actie_max ,
    COUNT(dc_vert.primary_supp) over (PARTITION BY dc_vert.item, dc_vert.loc) primary_count
  FROM dc_item_loc_pim_lms dc_vert ,
    item_supplier isu ,
    store sto ,
    wh w
  WHERE dc_vert.primary_supp    IS NOT NULL
  AND isu.item                   = dc_vert.item
  AND dc_vert.primary_supp       = isu.supplier
  AND W.WH                       = dc_vert.source_wh
  AND sto.store                  = dc_vert.loc
  AND ISU.SUPP_DISCONTINUE_DATE >= SYSDATE
  )
  WHERE actie       = actie_max
  AND primary_count = 1;
  l_item item_loc.item%TYPE;
  l_loc item_loc.loc%TYPE;
  loc_type item_loc.loc_type%TYPE;
  l_source_method item_loc.source_method%TYPE;
  l_primary_supp item_loc.primary_supp%TYPE;
  l_source_wh item_loc.source_wh%TYPE;

  i        NUMBER;
  l_commit VARCHAR2(1) := 'Y';
  BEGIN
  i             :=0;
  FOR r_itemloc IN c_itemloc
  LOOP
  i := i+1;
  UPDATE item_loc il
  SET 
    il.source_method        = r_itemloc.source_method ,     -- 'S'
    il.loc_type             = r_itemloc.loc_type ,      -- 'S'
    il.primary_supp         = r_itemloc.primary_supp ,
    il.source_wh            = r_itemloc.source_wh ,
    il.last_update_datetime = SYSDATE
  WHERE item                    = r_itemloc.item
  AND   loc                   = r_itemloc.loc;
  IF l_commit                   = 'Y' AND mod(i, 1000) = 0 THEN
  COMMIT ;
  END IF;
  END LOOP;
  EXCEPTION
  WHEN OTHERS THEN
     dbms_output.put_line('SOMETHING WENT WRONG');
  END;

Answer 1

Assuming that the code does functionally what you want...

1) Remove the exception handler. There is no benefit to catching an unknown exception that you cannot handle unless you are doing something like logging it and rethrowing it. Catching an exception only to call dbms_output not only hides the exception details and the stack trace that you'd need to debug the exception, it may hide the exception entirely if the caller happens not to read from the buffer dbms_output writes to.

2) Committing in a loop is generally a bad idea. Most importantly, what happens if your session dies in the middle? You'd have a partially committed update with no way to resume the operation when you restarted the code later on. You'd have to re-update all the rows that you had previously updated and whose updates you committed which could have downstream impacts. And committing in a loop slows down your code for no real reason.

3) If all you are trying to do is update a table, the most efficient approach would be to write a single UPDATE statement that updates all the rows at once rather than iterating over a cursor and doing a lot of single-row updates. There may be other reasons to favor a cursor (it may make the code easier for other developers to understand, for example) but from a performance standpoint, if you can do it in SQL, SQL is going to be the most efficient approach.

Answer 2

1) You're replacing a meaningful error with a meaningless one. Plus, if you have haven't set output on, an error will be missed entirely. The best thing to do would be to simply remove the exception block. If you can't do that, you should dump at least SQLERRM and probably DBMS_UTILITY.FORMAT_ERROR_BACKTRACE to DBMS_OUTPUT. To catch and record errors well, you'd need to pass the exception details to a separate procedure that would use an autonomous transaction to write those details to a table. Though, even in that case, you're best off re-raising the error after you record it.

2) Committing every X records is a poor practice in most cases. If these records are all being updated together, then they should be part of the same transaction.

3) You could do this in a single statement using either UPDATE or MERGE. Generally that's preferred, as it avoids additional context switches. Personally, I like MERGE in this scenario:

MERGE INTO item_loc il
USING      (SELECT item,
                   loc,
                   loc_type,
                   source_method,
                   primary_supp,
                   source_wh
            FROM   (SELECT dc_vert.item,
                           dc_vert.loc,
                           dc_vert.loc_type,
                           dc_vert.source_method,
                           dc_vert.primary_supp,
                           w.primary_vwh source_wh,
                           dc_vert.actie,
                           MAX(dc_vert.actie) OVER (PARTITION BY dc_vert.item, dc_vert.loc) actie_max,
                           COUNT(dc_vert.primary_supp) OVER (PARTITION BY dc_vert.item, dc_vert.loc) primary_count
                    FROM   dc_item_loc_pim_lms dc_vert,
                           item_supplier isu,
                           store sto,
                           wh w
                    WHERE  dc_vert.primary_supp IS NOT NULL
                       AND isu.item = dc_vert.item
                       AND dc_vert.primary_supp = isu.supplier
                       AND w.wh = dc_vert.source_wh
                       AND sto.store = dc_vert.loc
                       AND isu.supp_discontinue_date >= SYSDATE)
            WHERE  actie = actie_max AND primary_count = 1) itemloc
ON         (il.item = itemloc.item AND il.loc = itemloc.loc)
WHEN MATCHED THEN
   UPDATE SET
      il.source_method          = itemloc.source_method,
      il.loc_type               = itemloc.loc_type,
      il.primary_supp           = itemloc.primary_supp,
      il.source_wh              = itemloc.source_wh,
      il.last_update_datetime   = SYSDATE;