exception
when others then
dbms_output.put_line(SQLERRM);
end;I hate your code
http://asktom.oracle.com/pls/ask/search?p_string=%22i+hate+your+code%22 start over, rewrite this entire procedure - I'm dead serious, the error handling is actually error INDUCING.
why would you do this?
select substr(rpt.file_or_text,1,v_dir_length - 1) into
v_directory from dual;
instead of
v_directory := substr(rpt.file_or_text,1,v_dir_length - 1);
Ok, way too many things wrong here - no to mention that this is "production code" and yet, here I am, looking up the definition of instr to be sure I can reverse engineer it (eg: comments? logic flow?)
And it is really long - why no procedures/functions in the package to make it modular and understandable?
There are logic issues, I don't even know what to say about, consider that set error msg to null in one update and then follow it by setting to v_outmsg right after???
Please don't tell me you have a column name "P_KEY" - given that you also have a coding convention of p_ for parameter names????? sigh, p_value too - not too good of an idea... You'll ALWAYS WANT TO USE A CORRELATION NAME AND DOT NOTATION WITH THAT, ALWAYS.
This is stunning:
begin
select p_value into v_clean_dur
from pfecoder_parameters
where p_key = 'CLEAN_EMAIL_LOG' and
rownum < 2;
exception
when others then
dbms_output.put_line(SQLERRM);
end;
one would presume that p_key is a primary key, so one wonders about the rownum clause and one really wonders about that truly bad when others :(
You will want to ensure you have an index that starts with SENT_TS and has at least one attribute after it that is NOT NULL. For example, an index like:
create index sent_ts_idx on pfecoder_mail_queue(sent_ts,0)
would be useful - so the "where sent_ts is null" can do a full scan efficiently (this by the way is probably your 'hang', it isn't a hang, you are waiting for the full scan of this probably large table with lots of legacy records in it to finish....) see
http://asktom.oracle.com/Misc/something-about-nothing.html for details on what I'm talking about
I'm going to do something I don't often do, I'm going to rewrite this from scratch as an example...
<b>now, I had to stub out this package - just to compile the real package..</b>
ops$tkyte%ORA10GR2> create or replace package pfecoder_generic_mailer
2 as
3 type mailList is table of varchar2(200) index by binary_integer;
4
5 procedure send_message
6 (p_from in varchar2, p_maillist in mailList, p_dir in varchar2,
7 p_file in varchar2, p_subject in varchar2, p_flag in varchar2,
8 p_outmsg out varchar2);
9
10 end;
11 /
Package created.
ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2>
ops$tkyte%ORA10GR2> CREATE OR REPLACE Package PFECODER_MAILER
2 IS<b>
3 -- rule #1
4 -- never use global variables unless and until you have to
5 -- rule #2
6 -- you never "have" to
7 -- therefore, that nasty awkward global you had is history...
8</b>
9 -- this is the main and only entry point to this package, it
10 -- is designed to be called by blah blah blah...... and so on
11 -- basically, it
12 -- a) flags as an error all emails in the queue without a to/from or body
13 -- b) processes all emails in the queue with a sent_ts is null
14 -- c) cleans out old emails based on the parameter CLEAN_EMAIL_LOG
15 -- in the parameters table
16 procedure send_mail;
17 END;
18 /
Package created.
<b>Note in the following how each routine is small, bit sized, and pretty clear as to precisely what it does. They each fit on a screen. This is really important, especially if you ever want someone to look at your code, you have to make it easy for our brains to understand. It took me way (far far far) too long to figure out what the heck was happening in this code.</b>
ops$tkyte%ORA10GR2> CREATE OR REPLACE Package Body PFECODER_MAILER
2 IS
3
4 -- internal routine, probably actually belongs in the pfecoder_generic_mailer
5 -- package. Turns a string into a mailList
6
7 function string_to_table( p_str in varchar2 ) return pfecoder_generic_mailer.mailList
8 is
9 l_str long default p_str || ',';
10 l_n number;
11 l_data pfecoder_generic_mailer.mailList;
12 begin
13 loop
14 l_n := instr( l_str, ',' );
15 exit when (nvl(l_n,0) = 0);
16 l_data( l_data.count+1 ) := ltrim(rtrim(substr(l_str,1,l_n-1)));
17 l_str := substr( l_str, l_n+1 );
18 end loop;
19 return l_data;
20 end;
21
22
23 -- routine to take an input string, find the last '/' in it
24 -- if the '/' exists - then break into DIR and FILENAME
25 -- else, set DIR to null and FILENAME to string
26
27 procedure split_into_dir_and_file
28 ( p_str in varchar2,
29 p_dir out varchar2,
30 p_file out varchar2 )
31 is
32 l_n number := instr( p_str, '/', -1, 1 );
33 begin
34 if ( l_n > 0 )
35 then
36 p_dir := substr( p_str, 1, l_n-1 );
37 p_file := substr( p_str, l_n+1 );
38 else
39 p_dir := null; -- not necessary, it is null, but more clear
40 p_file := p_str;
41 end if;
42 end;
43
44 -- based on the setting int he parameters table
45 -- remove all old emails...
46
47 procedure clean_up_old_emails
48 is
49 begin
50 delete from pfecoder_email_queue
51 where sent_ts < (select sysdate-pfecoder_parameters.p_value
52 from pfecoder_parameters
53 where pfecoder_parameters.p_key = 'CLEAN_EMAIL_LOG');
54 end;
55
56 -- instead of procedural row by row code, let us just avoid hitting emails
57 -- we know we cannot send. Flag them as errors. We will not process them in the
58 -- main body below - the main body will take our where clause and negate it, what
59 -- we flag - they skip!
60
61 procedure flag_as_errors_bad_emails
62 is
63 begin
64 update pfecoder_email_queue
65 set error_msg = case when to_address is null
66 then 'Email error : NoToEmailAdress, No From email address'
67 when from_address is null
68 then 'Email error : No From Address'
69 when subject is null and file_or_text is null
70 then 'Email error : No Subject and no Message'
71 end
72 where sent_ts is null
73 and ((to_address is null) or (from_address is null) or (subject is null and file_or_text is null));
74 end;
75
76
77 Procedure send_mail
78 is
79
80 v_SQLErrMsg varchar2(150);
81 v_file_name varchar2(100);
82 v_directory varchar2(100);
83 v_mailList pfecoder_generic_mailer.mailList;
84 v_outmsg varchar2(4000);
85
86 -- notice we only select records we have a chance of actually
87 -- processing, the and NOT () takes care of that for us.
88
89 cursor c_rpt_cursor is select TO_ADDRESS,
90 FROM_ADDRESS,
91 SUBJECT,
92 file_or_text,
93 IS_FILE,
94 SEND_AS_ATTACHMENT,
95 ERROR_MSG
96 FROM pfecoder_email_queue
97 where sent_ts is null
98 and NOT ((to_address is null) or
99 (from_address is null) or
100 (subject is null and file_or_text is null))
101 for update;
102
103 BEGIN
104 flag_as_errors_bad_emails;
105
106 for rpt in c_rpt_cursor
107 loop
108 v_mailList := string_to_table( rpt.to_address );
109 split_into_dir_and_file( rpt.file_or_text, v_directory, v_file_name );
110
111 begin
112 pfecoder_generic_mailer.send_message
113 (rpt.FROM_ADDRESS, v_maillist, v_directory,
114 v_file_name, rpt.subject, 'Y', v_outmsg);
115
116 update pfecoder_email_queue
117 set sent_ts = sysdate,
118 error_msg = v_outmsg
119 where current of c_rpt_cursor;
120 exception
121 when utl_tcp.network_error then
122 v_SQLErrMsg := SUBSTR(SQLERRM, 1, 150);
123 update pfecoder_email_queue
124 set ERROR_MSG = 'Email error :' || v_SQLErrMsg||', Network Error'
125 where current of c_rpt_cursor;
126 end;
127 end loop;
128
129 clean_up_old_emails;
130 end send_mail;
131
132
133
134 END; -- Package Body pfecoder_mailer
135 /
Package body created.
Now, if it was my code, I would have added calls to dbms_applicaiton.set_session_longops (search for that please) to the code in order to be able to monitor it's progress from another session.....
Also, I left the commit out on purpose, really. Only the client knows if this should be committed or not.