Using function inside a cursor (by using a variable)

Question

I would like to confirm the correct use of the following:

1) Use a global variable to get return values from a function only once (since my function will be returning some Sequence values)

2) Use that variable inside a cursor multiple times

3) All of these will be inside a procedure

Below shows a sample.

CREATE OR REPLACE Procedure insert_myTable is
  --declare variables for insert
  v_firstNO VARCHAR2(10);
  v_secondNO VARCHAR2(6);

  --declare variable to store the sequence number
  var_ASeqno varchar2(6);

  -- Validation
  v_check VARCHAR2 (10 Byte);
  v_table_name varchar2(50):='myTable';

  cursor c1 is
    select distinct firstNO,
                    secondNO
      from (SELECT hdr.someNum firstNO,
                   -- using variable to assign the sequence no
                   var_ASeqno secondNO
              FROM someOtherTable hdr
              WHERE -- some condition
            union
            SELECT hdr.someNum firstNO,
                   -- using variable to assign the sequence no
                   var_ASeqno  secondNO
              FROM someOtherTable hdr
              WHERE -- some other conditions
            union
            SELECT hdr.someNum firstNO,
                   -- using variable to assign the sequence no
                   var_ASeqno secondNO
              FROM someOtherTable hdr
              WHERE -- some other other conditions


begin
  if c1%isopen then
    close c1;
  end if;

  v_check:=null;

  FOR i IN c1 LOOP
    --assign variables for insert
    v_firstNO := i.firstNO ;
    v_secondNO  := i.secondNO ;

    begin
      -- calling the Function aSeqNoFunc and assign the
      --Sequence Number into the variable var_ASeqno
      var_ASeqno := aSeqNoFunc();

      select firstNO
        into v_check
        from myTable a
        where firstNO = i.firstNO
              and secondNO =i.secondNO;
    exception
      when no_data_found then
        --insert into target table
        INSERT INTO myTable (firstNO, secondNO)  
          values (v_firstNO, v_secondNO);
    end ;
  end loop;
end;

As can be seen, the function 'aSeqNoFunc' is called before the Insert near the end. The values are assigned to the variable 'var_ApmSeqno' which in turn is used three times inside the cursor.

Thank you.

Answer 1

A few suggestions:

1. You have an END; statement after the declaration of cursor c1 which doesn't match up with anything. You should remove this from your code.

2. There's no need to check to see if the cursor is open when you enter the procedure. It won't be. Even better, don't use an explicit cursor declaration - use a cursor FOR-loop.

3. Use UNION ALL instead of UNION unless you know what the difference between the two is. (And go read up on that. 99.9% of the time you want UNION ALL...).

4. However, as it appears that all the rows are being selected from the same table you may be able to eliminate the UNION's altogether, as shown below.

5. There's no benefit to assigning NULL to a variable at the beginning of a function. Variables are initialized to NULL if there's no other explicit initialization value given.

6. IMO there's no benefit to having a function which returns the next value from a sequence. It just makes understanding the code more difficult. Get rid of FUNCTION aSeqNoFunc and just invoke SOME_SEQUENCE.NEXTVAL where appropriate - so in the above I suggest you use var_ASeqno := SOME_SEQUENCE.NEXTVAL.

7. You need to assign the value to var_ASeqno before cursor c1 is opened. As written abovevar_ASeqno will be null at the time the cursor is opened, so the cursor will probably not return what you expect. But more to the point I don't see that there's any reason to have the cursor return the value of var_ASeqno. Just use the value of var_ASeqno in your INSERT statements or wherever else they're needed.

8. Use a MERGE statement to insert data if it doesn't already exist. This avoids the awkward "SELECT...catch the NO_DATA_FOUND exception...INSERT in the exception handler" logic.

9. And as @boneist points out in her comment, by the time we've gone this far there's really no point to the cursor. You might as well just use the MERGE statement to perform the INSERTs without using a cursor.

So I'd try rewriting this procedure as:

CREATE OR REPLACE Procedure insert_myTable is
begin
  MERGE INTO MYTABLE m
    USING (SELECT FIRSTNO,
                  SOME_SEQUENCE.NEXTVAL AS SECONDNO
             FROM (SELECT DISTINCT hdr.someNum AS FIRSTNO
                     FROM someOtherTable hdr
                     WHERE (/* some condition */)
                        OR (/* some other conditions */)
                        OR (/* some other other conditions */))) d
      ON d.FIRSTNO = m.FIRSTNO AND
         d.SECONDNO = m.SECONDNO
    WHEN NOT MATCHED THEN INSERT (FIRSTNO, SECONDNO)
      VALUES (d.FIRSTNO, d.SECONDNO);
end INSERT_MYTABLE;

Answer 2

Taking into account the fact that you want all the rows being inserted to have the same sequence number assigned, I think you could probably rewrite your procedure to something like:

create or replace procedure insert_mytable
is
  v_seq_no number;
begin
  v_seq_no := somesequence.nextval;

  merge into mytable tgt
  using (select firstno,
                v_seq_no secondno
         from   (select hdr.somenum firstno
                 from   someothertable1 hdr
                 where  -- some condition
                 union
                 select hdr.somenum firstno
                 from   someothertable2 hdr
                 where  -- some other conditions
                 union
                 select hdr.somenum firstno
                 from   someothertable3 hdr
                 where  -- some other other conditions
                 )
         ) src
    on (tgt.firstno = src.firstno and tgt.secondno = src.secondno)
  when not matched then
    insert (tgt.firstno, tgt.secondno)
    values (src.firstno, src.secondno);
end insert_mytable;
/

If this doesn't match with what you're trying to do, please edit your question to provide more information on what the aim of the procedure is. Example input and output data would be appreciated, so that we have a better idea of what you're wanting (since we can't see your table structures, data, etc).

---

ETA: Info on performance considerations between set-based and row-by-row approaches.

Here's a simple script to insert of a million rows, both row-by-row and as a single insert statement:

create table test (col1 number,
                   col2 number);

set timing on;

-- row-by-row (aka slow-by-slow) approach
begin
  for rec in (select level col1, level * 10 col2
              from   dual
              connect by level <= 1000000)
  loop
    insert into test (col1, col2)
    values (rec.col1, rec.col2);
  end loop;
end;
/

commit;
truncate table test;

-- set based approach (keeping in an anonymous block for comparison purposes)
begin  
  insert into test (col1, col2)
  select level, level*10
  from   dual
  connect by level <= 1000000;
end;
/

commit;

drop table test;

Here's the output I get, when I run the above in Toad:

Table created.
 PL/SQL procedure successfully completed.
Elapsed: 00:00:21.87
Commit complete.
Elapsed: 00:00:01.03
Table truncated.
Elapsed: 00:00:00.22
 PL/SQL procedure successfully completed.
Elapsed: 00:00:01.96
Commit complete.
Elapsed: 00:00:00.03
Table dropped.
Elapsed: 00:00:00.18

Do you see the elapsed time of 21 seconds for the row-by-row approach, and 2 seconds for the set-based approach? Massive difference in performance, don't you agree? If that's not reason to consider writing your code as set based in the first instance, then I don't know what else will convince you/your boss!