Re: [bug fix] pg_ctl always uses the same event source

On Sun, Jul 20, 2014 at 7:44 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Fri, Jul 18, 2014 at 7:08 PM, MauMau <maumau307@gmail.com> wrote:
>>
>> From: "Magnus Hagander" <magnus@hagander.net>
>>
>>> On Fri, Jul 18, 2014 at 5:33 AM, Amit Kapila <amit.kapila16@gmail.com>
>>> wrote:
>>>>
>>>> On Thu, Jul 17, 2014 at 4:51 PM, Magnus Hagander <magnus@hagander.net>
>>>> wrote:
>>>>>
>>>>>
>>>>> Did anyone actually test this patch? :)
>>>>>
>>>>> I admit I did not build it on Windows specifically because I assumed
>>>>> that was done as part of the development and review. And the changes
>>>>> to pg_event.c can never have built, since the file does not include
>>>>> the required header.
>>>>
>>>>
>>>> I have tested it on Windows and infact on Linux as well to see if there
>>>> is any side impact before marking it as Ready For Committer.
>>>>
>>>> It seems to me that the required header is removed in last version
>>>> (pg_ctl_eventsrc_v11) where MessageBox() related changes have been
>>>> removed from patch as per recent discussion.  Sorry for not being able
>>>> to check last version posted.
>>>
>>>
>>> Gotcha. Thanks for clarifying, and I apologize if I came across a bit
>>> harsh even with the smiley.
>
> The statement was not at all harsh.  I think you are right in asking that
> question and I believe that is the minimum expectation once the patch
> reaches Ready To Committer stage.
>
>
>>
>> I'm sorry to have caused both of you trouble.  I have to admit that I
>> didn't compile the source when I removed the MessageBox()-related changes.
>> The attached patch fixes that.  I confirmed successful build this time.
>
> I have tested the attached patch on windows, it works fine both for
> default and non-default cases.  It passes other regression tests as well
> and build went fine on Linux.
>
> One more thing about inclusion of new header file in pgevent.c,  I think
> that is okay because we include it in other modules (client side) present
> in bin directory like reindex.

Thanks to you both for confirming it works, applied in the current state.

-- Magnus HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/