What's inherently wrong with this code ??

[Horace]

Hi All,

Our Developers have been at it again... They claim the following code runs slowly, and due to Horace's bizarre world of DBAing, this is low priority on my list of things to look at.

I've had a quick look, and although the code appears functional, it just doesn't look right.....

I'd greatly appreciate any input from you coding gurus.

Thanks.

--------------------------------------------------------------------

declare

CURSOR mem_cur
IS
SELECT membership_number FROM w_member_updates_ext; /* returns approx 10,000 rows */

str_sql1 varchar2 (2000):='insert /*+ append nologging*/ into temp select membership_number from question_response_001 WHERE membership_number = :1 AND latest_answer = ''Y''';
str_sql2 varchar2 (2000):='insert /*+ append nologging*/ into temp select membership_number from question_response_002 WHERE membership_number = :1 AND latest_answer = ''Y''';
str_sql3 varchar2 (2000):='insert /*+ append nologging*/ into temp select membership_number from question_response_003 WHERE membership_number = :1 AND latest_answer = ''Y''';
....... /* FOR 50 question_response tables */

BEGIN

FOR mem_rec IN mem_cur LOOP

execute IMMEDIATE str_sql1 USING mem_rec.membership_number;
execute IMMEDIATE str_sql2 USING mem_rec.membership_number;
execute IMMEDIATE str_sql3 USING mem_rec.membership_number;
....... /* FOR 50 str_sql */

END LOOP;

END;

[hrishy]

Hi

Try this instead of Dynamic sql's like that

declare
begin
insert into temp
select membership_number
from question_response_001
WHERE membership_number
in (SELECT membership_number
FROM w_member_updates_ext
AND latest_answer = 'Y')
AND latest_answer = 'Y'
union all
select membership_number
from question_response_002
WHERE membership_number
= :1 AND latest_answer = ''Y''';

[hrishy]

Hi

Try this instead of Dynamic sql's like that

declare
begin
insert into temp
select membership_number
from question_response_001
WHERE membership_number
in (SELECT membership_number
FROM w_member_updates_ext
AND latest_answer = 'Y')
AND latest_answer = 'Y'
union all
select membership_number
from question_response_002
WHERE membership_number
in (SELECT membership_number
FROM w_member_updates_ext
AND latest_answer = 'Y')
AND latest_answer = 'Y'
union all
select membership_number
from question_response_003
WHERE membership_number
in (SELECT membership_number
FROM w_member_updates_ext
AND latest_answer = 'Y')
AND latest_answer = 'Y' ;

end;
/
commit;
regards
Hrishy

[padders]

Main problem is that original solution is conceptually lacking - it is not necessary to return and process each membership number in turn, the requirement can and should be satisfied by a join between w_member_updates_ext and response_table_00n, as Hrishy correctly observes.

Presumably dynamic SQL is being used here because "TEMP" does not exist at compile time. It would make more sense to use a global temporary table here although if tables are large the overall parse overhead from the dynamic SQLs, while unnecessary, may not be a significant part of the run-time.

I would be tempted to abstract the complexity of the fifty tables. A view to UNION ALL the identical tables would do that nicely - if you can't be bothered to write it then just generate it, e.g.

Oracle Database 10g Enterprise Edition Release 10.1.0.2.0 - 64bit Production
With the Partitioning, OLAP and Data Mining options

SQL> DECLARE
  2    v_sql VARCHAR2 (32767);
  3  BEGIN
  4    FOR i IN 1..50 LOOP
  5      IF NOT (v_sql IS NULL) THEN
  6        v_sql := v_sql || ' UNION ALL ';
  7      END IF;
  8      v_sql := v_sql ||
  9        'SELECT membership_number, latest_answer ' ||
 10        'FROM question_response_' || LPAD (i, 3, '0');
 11    END LOOP;
 12    v_sql := 'CREATE OR REPLACE VIEW v_all_question_response AS ' || v_sql;
 13    EXECUTE IMMEDIATE v_sql;
 14  END;
 15  /

PL/SQL procedure successfully completed.

SQL> DESC v_all_question_response;
 Name                                      Null?    Type
 ----------------------------------------- -------- ----------------------------
 MEMBERSHIP_NUMBER                                  NUMBER
 LATEST_ANSWER                                      VARCHAR2(1)

SQL>

...and having concealed the underlying complexity the INSERT is (I think) no more complicated than...

SQL> INSERT /*+ APPEND */ INTO temp
  2    SELECT aqr.membership_number
  3    FROM   w_member_updates_ext mue, v_all_question_response aqr
  4    WHERE  aqr.membership_number = mue.membership_number
  5    AND    aqr.latest_answer = 'Y';

5477 rows created.

SQL>

[Horace]

Thanks Guys,

Your input is invaluable, and has saved me a stack of time. I think Padders nails it with his explanation.

So this afternoon, I'll stroll nonchalantly up to the dev team and mention that their code is "conceptually lacking", walk off and wait for the bun fight....

Cheers.

[jmodic]

Originally posted by Horace
So this afternoon, I'll stroll nonchalantly up to the dev team and mention that their code is "conceptually lacking", walk off and wait for the bun fight....

That's not quite the action which Mr.Hanky would recommend ....

[OracleDoc]

Originally posted by jmodic
That's not quite the action which Mr.Hanky would recommend ....

Indeed... I think his response would be along the lines of calling a meeting of all the developers and placing a gun with one bullet in the middle of the table and walking off.

*peers above forum wall* see if MH is lurking near by..

[gandolf989]

Originally posted by jmodic
That's not quite the action which Mr.Hanky would recommend ....

I would think that Mr. Hanky would call a meeting, toss a live hand grenade into the room and close the door.

[Horace]

But if I blow up all the developers, who do you think will end up writing all the code??

[Horace]

Ouch! - Had time to test this now. Ignoring the fact that the dev team thought this was slow for the time being...

Using the current dev code , the temp table creates in 4 seconds.

*************************************************************

Using Padders code, it takes 29 seconds.

INSERT STATEMENT |
LOAD AS SELECT |
HASH JOIN |
EXTERNAL TABLE ACCESS FULL| W_MEMBER_UPDATES_EXT
VIEW | V_ALL_QUESTION_RESPONSE
UNION-ALL |
TABLE ACCESS FULL | QUESTION_RESPONSE_001
TABLE ACCESS FULL | QUESTION_RESPONSE_002
TABLE ACCESS FULL | QUESTION_RESPONSE_003


*************************************************************

Using Hrishy's code, it had not created the table after 30 minutes so I killed the session.

INSERT STATEMENT |
UNION-ALL |
FILTER |
TABLE ACCESS FULL | QUESTION_RESPONSE_001
FILTER |
EXTERNAL TABLE ACCESS FULL| W_MEMBER_UPDATES_EXT
FILTER |
TABLE ACCESS FULL | QUESTION_RESPONSE_002
FILTER |
EXTERNAL TABLE ACCESS FULL| W_MEMBER_UPDATES_EXT
FILTER |
TABLE ACCESS FULL | QUESTION_RESPONSE_003
FILTER |
EXTERNAL TABLE ACCESS FULL| W_MEMBER_UPDATES_EXT

*************************************************************

The question is - How am I going to get the poison out of the tea urn??