On Tue, Oct 24, 2017 at 8:30 PM, Ashesh Vashi <ashesh.va...@enterprisedb.com > wrote:
> 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. > Committed your original patch. -- 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> > > -- 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> >> > >
