Re: [pgAgent][Patch] Fixing connection pool leak

On Tue, Oct 24, 2017 at 8:59 PM, Rob Emery <re-pgsql(at)codeweavers(dot)net> wrote:

> Awesome thanks.
>
> What's the release schedule like for pgagent?
>
I have no idea.
That can be answered by Dave only.

Dave?

--

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,
> Rob
>
>
>
> On 24 October 2017 at 16:05, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
> wrote:
>
>> On Tue, Oct 24, 2017 at 8:30 PM, Ashesh Vashi <
>> ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> On Tue, Oct 24, 2017 at 8:21 PM, Robert Emery <
>>> robertemery(at)codeweavers(dot)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/pg
>>>> agent/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-pgsql(at)codeweavers(dot)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(dot)patel(at)enterprisedb(dot)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(dot)vashi(at)enterprisedb(dot)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-pgsql(at)codeweavers(dot)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: *contactus(at)codeweavers(dot)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: robertemery(at)codeweavers(dot)net | T: 01785 711633 | W:
>>>>>>>> www.codeweavers.net
>>>>>>>>
>>>>>>>
>>>>
>>>>
>>>> --
>>>> Robert Emery
>>>> Infrastructure Director
>>>>
>>>> E: robertemery(at)codeweavers(dot)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: *contactus(at)codeweavers(dot)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: robertemery(at)codeweavers(dot)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: *contactus(at)codeweavers(dot)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>
>