Re: Re: Server is not getting started with log level as debug5 on master after commit 3147ac

Amit Kapila <amit.kapila16@gmail.com> writes:
> On Fri, Nov 22, 2013 at 9:30 AM, Amit Kapila <amit.kapila16@gmail.com> wrote:
>> I could think of below possible ways to fix the problem:
>> a. in function pvsnprintf(), save the errno before setting it to 0 and
>> then before exiting function reset it back to saved errno. I think
>> this is sane because in function pvsnprintf, if any error occurs due
>> to which errno is changed, it will not return control, so errno will
>> not be required by callers.
>> b. we can change the callers like _dosmaperr() who have responsibility
>> to save errno for their callers.
>> 
>> Patch with approach a) is attached with mail, we can change code as
>> per approach b) or any other better method as well, but for now I have
>> prepared patch with approach a), as I could not see any problem with
>> it.

> Again looking at it, I think better fix would be to restore 'errno'
> from 'edata->saved_errno' in errfinish() incase the control returns
> back to caller (elevel <= WARNING). It seems to me that this fix is
> required anyway because if we return from errfinish (ereport/elog) to
> caller, it should restore back 'errno', as caller might need to take
> some action based on errno.
> Patch to restore 'errno' in errfinish() is attached with this mail.

I think this is pretty misguided.  In general, there should be no
expectation that a function call doesn't stomp on errno unless it's
specifically documented not to --- which surely ereport() never has
been.  So generally it's the responsibility of any code that needs
errno to be preserved to do so itself.  In particular, in the case
at hand:
       if (doserrors[i].winerr == e)       {           errno = doserrors[i].doserr;
#ifndef FRONTEND           ereport(DEBUG5,                   (errmsg_internal("mapped win32 error code %lu to %d",
                             e, errno)));
 
#elif FRONTEND_DEBUG           fprintf(stderr, _("mapped win32 error code %lu to %d"), e, errno);
#endif           return;       }

even if we were to do what you suggest to make the ereport() call
preserve errno, the FRONTEND_DEBUG code path would still be utterly
broken, because fprintf() is certainly capable of changing errno.

So basically, _dosmaperr() is broken and always has been, and it's
surprising we've not seen evidence of that before.  It needs to not
try to set the real errno variable till it's done logging about it.

Normally I avoid touching Windows-specific code for lack of ability
to test it, but in this case the needed fix seems pretty clear, so
I'll go make it.
        regards, tom lane