RE: Patch proposal for leak in DBD::Oracle when calling 'execute_array' with UTF-8 NLS...

Hi guys had some time today to play again
 
Seems we have one of three choices
 
SvREFCNT_dec(tuples_utf8_av);

as pathced
 
or 
 
av_clear(tuples_utf8_av);
 
at the send of the 'i' loop or
 
sv_2mortal((SV*)tuples_utf8_av);
 
at the begining
 
 
I have tried them all and they all get rid of the leak.
 
I also noticed that we are useing 
 
safemalloc
 
which is sort of ugly  Newz is better
 
My choice is to use the 
 
sv_2mortal((SV*)tuples_utf8_av);
 
at the beginning and let perl worry about it.
 
 
I will do another pathc I think to get rid of the malloc code as I see it in other places as well
 
Any more thoughts before I stick it in trunk???
 
cheers
John

 

> Date: Tue, 9 Oct 2012 21:21:13 +0200
> From: [email protected]
> To: [email protected]
> CC: [email protected]
> Subject: Re: Patch proposal for leak in DBD::Oracle when calling 'execute_array' with UTF-8 NLS...
> 
> 
> 
> Hello Martin,
> 
> Sorry for the late reply.
> Basically what my script tries to do is to copy lots of records from one 
> database to another. To do this, I've come to the idea of using "execute 
> array" to speed up the process. I generally use NLS_LANG set to UTF-8 
> for Oracle, because it's the default encoding on my Linux distribution, 
> so it was not a choice I've made for that specific application.
> 
> It's when reviewing the code (after a little help from valgrind 
> memory-checker), that I got the idea to turn UTF-8 off. Specifically the 
> lines around ~ 3825 : ...
> /* update the utf8_flgs for this value */
> if (SvUTF8(sv)) {
> utf8_flgs[i] |= ARRAY_BIND_UTF8;
> if (SvTRUE(tuples_status)){
> av_push(tuples_utf8_av,newSViv(j));
> }
> 
> ... made me think it was probably worth a try.
> 
> As so far I understand, the 'tuples_utf8_av' is not given back to perl 
> interpreter, I assumed that it was safe to decrement it's reference 
> count to allow it to be garbage-collected. But as I said in my previous 
> mail, I've no experience in Perl internals and that may be a false 
> assumption.
> 
> Thanks very much for your feedback.
> Best regards,
> Pierre-Alain Blanc
> 
> On 09/10/12 09:53, Martin J. Evans wrote:
> > On 08/10/12 13:46, Pierre-Alain Blanc wrote:
> >>
> >>
> >> Hello,
> >>
> >> I've had a problem when using 'execute_array' to insert (lots of)
> >> records with DBD::Oracle (version 1.50): the script consumed too much
> >> memory and finally crashed (killed by kernel). I tried to trigger the
> >> garbage-collection with some code rewrite but it didn't help. But if
> >> I told Oracle *not* to use an UTF-8 charset (changing NLS_LANG from
> >> (for example) "german_germany.utf8" to "german_germany.we8dec"), the
> >> problem disappeared.
> >>
> >> After some investigations, I think the leak is in
> >> 'ora_st_execute_array' method of dbdimp.c. Please find the patch as
> >> attachment. As I'm completly new to writing C for Perl, it may be
> >> something I did not understand or did not correctly fixed. Sorry if
> >> it would be the case.
> >>
> >> Thanks & best regards, Pierre-Alain Blanc
> >>
> >>
> >
> > Thanks for looking into this. The patch:
> >
> > Index: dbdimp.c
> > ===================================================================
> > --- dbdimp.c (revision 15435)
> > +++ dbdimp.c (working copy)
> > @@ -3839,6 +3839,7 @@
> > }
> > Safefree(phs);
> > Safefree(utf8_flgs);
> > + SvREFCNT_dec(tuples_utf8_av);
> > /* Store array of bind typles, for use in OCIBindDynamic() 
> > callback. */
> > imp_sth->bind_tuples = tuples_av;
> > imp_sth->rowwise = (columns_av == NULL);
> >
> > looks good but it does not explain how changing chrset made the 
> > problem go away. Perhaps you could give me a better idea of what you 
> > were doing then I can replicate and test it.
> >
> > Martin
>