On Tue, Oct 24, 2017 at 8:21 PM, Robert Emery <robertem...@codeweavers.net> wrote:
> Hello again, > > I can't see any other way of solving this problem without either exposing > a method on JobThread to delete the Job owned by the thread which is called > by https://github.com/postgres/pgagent/blob/master/pgAgent.cpp#L139 or > another method that only gives up the connection which is called in the > same place as the delete jt. > > Is there a problem with chaining the ~JobThread() and ~Job() together as I > suggested previously? This does solve the leak. > Nothing wrong in it. My intention was to release the object asap. As per your findings, it won't go in that execution flow in all cases. I will commit your patch. -- Thanks, Ashesh > > Many Thanks, > Rob > > On 23 October 2017 at 20:09, Rob Emery <re-pg...@codeweavers.net> wrote: > >> Hiya, >> >> I've just confirmed neither of these patches resolve the issue, it >> appears in the error case I'm experiencing the JobThread::Entry() never >> executes. >> >> To clarify I'm talking about the path entering the else in >> pgAgent.cpp:125 which will save into pga_joblog with jlgstatus = "i" >> >> This path explicitly deletes jt however deleting jt doesn't clean up the >> connection, hence why I cascaded the delete in the original patch. >> >> I hope that makes sense >> >> Thanks, >> Rob >> >> On 23 October 2017 at 08:18, Neel Patel <neel.pa...@enterprisedb.com> >> wrote: >> >>> Hi Ashesh, >>> >>> I just added condition before delete the job otherwise it looks good. >>> Correct me if I am wrong. >>> >>> if (job != NULL) >>> { >>> delete job; >>> job = NULL; >>> } >>> >>> I have created two instances of pgagent on database cluster. As of now >>> not able to see any leak and keep you updated if found anything. >>> >>> @Robert - Can you also test at your environment and keep us updated for >>> any leak ? >>> >>> Thanks, >>> Neel Patel >>> >>> On Mon, Oct 23, 2017 at 10:30 AM, Ashesh Vashi < >>> ashesh.va...@enterprisedb.com> wrote: >>> >>>> Hi Rob, >>>> >>>> How about this? >>>> >>>> >>>> -- >>>> >>>> Thanks & Regards, >>>> >>>> Ashesh Vashi >>>> EnterpriseDB INDIA: Enterprise PostgreSQL Company >>>> <http://www.enterprisedb.com> >>>> >>>> >>>> *http://www.linkedin.com/in/asheshvashi* >>>> <http://www.linkedin.com/in/asheshvashi> >>>> >>>> On Sat, Oct 21, 2017 at 8:36 PM, Rob Emery <re-pg...@codeweavers.net> >>>> wrote: >>>> >>>>> Hi, >>>>> >>>>> Following on from https://www.postgresql.org/mes >>>>> sage-id/CA%2BOCxoz4tONxSpd1rdU-9SPKRzucz8Bar2CXkEDnCwV6H77Zy >>>>> A%40mail.gmail.com >>>>> >>>>> I think I've identified and fixed the issue, please see the patch >>>>> attached. >>>>> >>>>> As I understand it when there are multiple pgagent instances and they >>>>> clash executing a job (i.e rc != 1 on job.cpp:38), the loser of the >>>>> conflict's thread will never be executed (i.e. job.cpp:418 >>>>> JobThread::Entry), which is responsible for deleting the job owned by the >>>>> thread, meaning that the connection is never returned to the pool. By >>>>> moving the delete of the job into the destructor, we can assure that the >>>>> connection is tidied up in both cases as the thread is deleted in the >>>>> error >>>>> case explicitly in pgAgent.cpp:185. >>>>> >>>>> The only possibly unintended difference that I can see with doing this >>>>> is that the log "Completed job: %s" is now output when before it wasn't, >>>>> however I think this new behaviour is actually correct as the job object >>>>> is >>>>> completed at that time. >>>>> >>>>> Thanks, >>>>> Rob >>>>> >>>>> <https://codeweavers.net> >>>>> >>>>> >>>>> <http://us15.campaign-archive1.com/?u=fcb361cfa194cf70551bc5169&id=f556b0bf09> >>>>> Codeweavers >>>>> October >>>>> Newsletter >>>>> <http://us15.campaign-archive1.com/?u=fcb361cfa194cf70551bc5169&id=f556b0bf09> >>>>> *l *Auto Trader extends partnership with Codeweavers >>>>> <https://codeweavers.net/company-blog/auto-trader-extends-partnership-with-codeweavers-to-power-finance-on-auto-trader-websites> >>>>> >>>>> >>>>> <https://codeweavers.net/automotive-vision-conference-agenda> >>>>> >>>>> *What are Codeweavers doing to gear up for GDPR? >>>>> <https://codeweavers.net/company-blog/what-are-codeweavers-doing-to-gear-up-for-gdpr>* >>>>> >>>>> >>>>> >>>>> *Phone:* 0800 021 0888 * Email: *contac...@codeweavers.net >>>>> *Codeweavers Ltd* | Barn 4 | Dunston Business Village | Dunston | >>>>> ST18 9AB >>>>> Registered in England and Wales No. 04092394 | VAT registration no. >>>>> 974 9705 63 >>>>> >>>>> -- >>>>> Robert Emery >>>>> Infrastructure Director >>>>> >>>>> E: robertem...@codeweavers.net | T: 01785 711633 | W: >>>>> www.codeweavers.net >>>>> >>>> > > > -- > Robert Emery > Infrastructure Director > > E: robertem...@codeweavers.net | T: 01785 711633 | W: www.codeweavers.net > > <https://codeweavers.net> > > > <http://us15.campaign-archive1.com/?u=fcb361cfa194cf70551bc5169&id=f556b0bf09> > Codeweavers > October > Newsletter > <http://us15.campaign-archive1.com/?u=fcb361cfa194cf70551bc5169&id=f556b0bf09> > *l *Auto Trader extends partnership with Codeweavers > <https://codeweavers.net/company-blog/auto-trader-extends-partnership-with-codeweavers-to-power-finance-on-auto-trader-websites> > > > <https://codeweavers.net/automotive-vision-conference-agenda> > > *What are Codeweavers doing to gear up for GDPR? > <https://codeweavers.net/company-blog/what-are-codeweavers-doing-to-gear-up-for-gdpr>* > > > > *Phone:* 0800 021 0888 * Email: *contac...@codeweavers.net > *Codeweavers Ltd* | Barn 4 | Dunston Business Village | Dunston | ST18 9AB > Registered in England and Wales No. 04092394 | VAT registration no. 974 > 9705 63 > > <https://www.linkedin.com/company/codeweavers-limited> > <https://vimeo.com/codeweaversltd> [image: > https://plus.google.com/b/105942302039373248738/+CodeweaversNet] > <https://plus.google.com/b/105942302039373248738/+CodeweaversNet> [image: > https://twitter.com/CodeweaversTeam] <https://twitter.com/CodeweaversTeam> >
