From: | Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> |
---|---|
To: | Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | Andres Freund <andres(at)anarazel(dot)de>, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Ashutosh Sharma <ashu(dot)coek88(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Hang in pldebugger after git commit : 98a64d0 |
Date: | 2016-12-21 04:32:54 |
Message-ID: | CAA4eK1+y-jU=Vtf3P9Rm3Nu-jsOL9NWyZNov9_4S+1Qy7VQ8XQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On Tue, Dec 20, 2016 at 8:04 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Sat, Dec 17, 2016 at 5:46 AM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>> Yeah, but we are planning to add a generic flag in WaitEvent structure
>> which can be used for similar purpose. However, as per your
>> suggestion, I have changed it only for Win32 port. Also, I kept the
>> change in ModifyWaitEvent API as that seems to be okay considering
>> 'reset' is a generic flag in WaitEvent structure.
>
> Well, we don't really need the change in ModifyWaitEvent if we have
> the change in WaitEventSetWaitBlock, right?
>
Yes.
> I'd be inclined to ditch
> the former and keep the latter.
>
Okay, I think you want to keep the change just for Win32 which seems
fair as we haven't noticed such problem on any other platform.
> Also, this doesn't look right:
>
> + for (cur_event = set->events;
> + cur_event < (set->events + set->nevents)
> + && returned_events < nevents;
> + cur_event++)
> + {
> + if (cur_event->reset)
> + {
> + WaitEventAdjustWin32(set, cur_event);
> + cur_event->reset = false;
> + }
> + }
>
> There's no need to include returned_events < nevents in the loop
> condition here because returned_events isn't changing.
>
Fixed.
> I think I'd also guard the reset flag with #ifdef WIN32. If it's not
> properly supported elsewhere it's just a foot-gun, and there seems to
> be no reason to write the code to properly support it elsewhere,
> whatever that would mean.
>
Okay.
Ashutosh Sharma has helped to test that pldebugger issue is fixed with
attached version.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
Attachment | Content-Type | Size |
---|---|---|
reset_wait_events_v4.patch | application/octet-stream | 2.1 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Dilip Kumar | 2016-12-21 04:35:14 | Re: Proposal : Parallel Merge Join |
Previous Message | Craig Ringer | 2016-12-21 04:20:05 | Re: bigint vs txid user confusion |