Thread: Re: [COMMITTERS] pgsql: Reserve the shared memory region during backend startup on

mha@postgresql.org (Magnus Hagander) writes:
> Log Message:
> -----------
> Reserve the shared memory region during backend startup on Windows, so
> that memory allocated by starting third party DLLs doesn't end up
> conflicting with it.

I am wondering why failure of the various TerminateProcess calls in
postmaster.c is elog(ERROR) and not elog(LOG).  While that probably
shouldn't happen, aborting the postmaster isn't a good response if it
does.  This patch introduces a new occurrence, but I see it is just
copying what was there already.
        regards, tom lane

On Sat, Jul 25, 2009 at 19:50, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> mha@postgresql.org (Magnus Hagander) writes:
>> Log Message:
>> -----------
>> Reserve the shared memory region during backend startup on Windows, so
>> that memory allocated by starting third party DLLs doesn't end up
>> conflicting with it.
>
> I am wondering why failure of the various TerminateProcess calls in
> postmaster.c is elog(ERROR) and not elog(LOG).  While that probably
> shouldn't happen, aborting the postmaster isn't a good response if it
> does.  This patch introduces a new occurrence, but I see it is just
> copying what was there already.

The case where it's doing it now is really a "can't happen" place, so
I don't think it's a big issue there. It could be argued that if we
can't terminate a process we just created (but never even started!),
something is very very badly broken on the system and we might as well
give up. Same for the part where we fail to ResumeThread() on the main
thread of a new process.

However, it seems that for example the one at line 3629 - where we're
just failing to save our backend state - shouldn't be such a FATAL
error. But if you actually look up into the function
save_backend_variables(), it's actually hardcoded to return true... In
case something goes wrong in there, there's an actual ereport(ERROR)
happening deep down already
(write_inheritable_socket/write_duplicated_handle).

To fix that we'd just have to turn those functions all into returning
boolean and log with LOG instead. AFAIK, we've had zero reports of
this actually happening, so I'm not sure it's worth redesigning.
Thoughts?

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

Magnus Hagander <magnus@hagander.net> writes:
> To fix that we'd just have to turn those functions all into returning
> boolean and log with LOG instead. AFAIK, we've had zero reports of
> this actually happening, so I'm not sure it's worth redesigning.
> Thoughts?

I'm not really insisting on a redesign.  I'm talking about the places
where the code author appears not to have understood that ERROR means
FATAL, because the code keeps plugging on after it anyway.  As far as
I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just
plain bogus, and changing to LOG there requires no other fixing.
        regards, tom lane

On Mon, Jul 27, 2009 at 16:14, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> To fix that we'd just have to turn those functions all into returning
>> boolean and log with LOG instead. AFAIK, we've had zero reports of
>> this actually happening, so I'm not sure it's worth redesigning.
>> Thoughts?
>
> I'm not really insisting on a redesign.  I'm talking about the places
> where the code author appears not to have understood that ERROR means
> FATAL, because the code keeps plugging on after it anyway.  As far as
> I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just
> plain bogus, and changing to LOG there requires no other fixing.

3630: can't happen, because we already elog(ERROR) deep in the
function, which is what I tried to outline above. That's the one
requiring a redesign - because the errors *inside* the function it
calls can certainly happen.
3657: is one of those "should never happen" issues - which we haven't
had any reports of.
3674: see above
3693 is a comment, I assume you mean 3683, which is also one of those
can't happen.

But. I'll look into cleaning those up for HEAD anyway, but due to lack
of reports I think we should skip backpatch. Reasonable?


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

Magnus Hagander <magnus@hagander.net> writes:
> On Mon, Jul 27, 2009 at 16:14, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>> I'm not really insisting on a redesign. �I'm talking about the places
>> where the code author appears not to have understood that ERROR means
>> FATAL, because the code keeps plugging on after it anyway. �As far as
>> I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just
>> plain bogus, and changing to LOG there requires no other fixing.

> But. I'll look into cleaning those up for HEAD anyway, but due to lack
> of reports I think we should skip backpatch. Reasonable?

Fair enough.
        regards, tom lane

On Tue, Jul 28, 2009 at 15:45, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>> On Mon, Jul 27, 2009 at 16:14, Tom Lane<tgl@sss.pgh.pa.us> wrote:
>>> I'm not really insisting on a redesign.  I'm talking about the places
>>> where the code author appears not to have understood that ERROR means
>>> FATAL, because the code keeps plugging on after it anyway.  As far as
>>> I can see, using ERROR at lines 3630, 3657, 3674, and 3693 is just
>>> plain bogus, and changing to LOG there requires no other fixing.
>
>> But. I'll look into cleaning those up for HEAD anyway, but due to lack
>> of reports I think we should skip backpatch. Reasonable?
>
> Fair enough.

Here's what I came up with. Seems ok?


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

Magnus Hagander <magnus@hagander.net> writes:
>>> But. I'll look into cleaning those up for HEAD anyway, but due to lack
>>> of reports I think we should skip backpatch. Reasonable?
>> 
>> Fair enough.

> Here's what I came up with. Seems ok?

Works for me.
        regards, tom lane

On Wed, Aug 5, 2009 at 16:53, Tom Lane<tgl@sss.pgh.pa.us> wrote:
> Magnus Hagander <magnus@hagander.net> writes:
>>>> But. I'll look into cleaning those up for HEAD anyway, but due to lack
>>>> of reports I think we should skip backpatch. Reasonable?
>>>
>>> Fair enough.
>
>> Here's what I came up with. Seems ok?
>
> Works for me.

Applied.


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