Re: Hang in pldebugger after git commit : 98a64d0

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
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

In response to

Responses

Browse pgsql-hackers by date

  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