Thread: Re: [BUGS] BUG #1466: #maintenace_work_mem = 16384

>>> The proposed test on Redirect_stderr looks pretty fishy too; for one
>>> thing it will almost certainly not be the right thing
>inside the stderr
>>> logger subprocess itself.
>
>> Could you explain further what the issue is there?
>
>Inside the logger subprocess, Redirect_stderr is guaranteed true (since
>it'll be inherited from the postmaster) and therefore the proposed
>change ensures that anything the logger might want to complain about
>goes to the original stderr, ie, into the bit bucket rather than
>someplace useful.  Perhaps something like
>
>    if ((!Redirect_stderr || am_syslogger) && pgwin32_is_service())
>
>would be reasonable.

<snip lots of others>

Here is an updated patch, that should take care of this. Tested that it
solves the problem reported.


>> There is special code in the send_message_to_server_log
>function to make
>> sure it's written directly to the file.
>
>If the logger is complaining, it's quite possibly because it's
>unable to
>write to its file.  Now that you mention it, doesn't this code go into
>infinite recursion if write_syslogger_file_binary() tries to ereport?
>

I haven't looked at this part, it appears a separate (but closely
related) issue.

Perhaps Andreas can comment on this?

//Magnus

Magnus Hagander wrote:

>
>
>>>There is special code in the send_message_to_server_log
>>
>>function to make
>>
>>>sure it's written directly to the file.
>>
>>If the logger is complaining, it's quite possibly because it's
>>unable to
>>write to its file.  Now that you mention it, doesn't this code go into
>>infinite recursion if write_syslogger_file_binary() tries to ereport?

Yes, apparently.

Actually, elog.c code should look like this:

if ((Log_destination & LOG_DESTINATION_STDERR) ...)
{
    if (am_syslogger)
       write_syslogger_file(buf.data, buf.len);
    else
       fwrite(buf.data, 1, buf.len, stderr);
}

This avoids unnecessary pipe traffic (which might fail too) and gettext
translation.

Next, the elog call in write_syslogger_file_binary will almost certainly
loop, so it should call write_stderr then (since eventlog is usually
fixed-size with cyclic writing, even in out-of-disk-space conditions
something might get logged).

3rd, I've been proposing to have redirect_stderr=true on by default at
least on win32 earlier, I still think this is reasonable.

Regards,
Andresa

Patch applied.  Thanks.

I assume this is not for 8.0.X.

---------------------------------------------------------------------------


Magnus Hagander wrote:
> >>> The proposed test on Redirect_stderr looks pretty fishy too; for one
> >>> thing it will almost certainly not be the right thing
> >inside the stderr
> >>> logger subprocess itself.
> >
> >> Could you explain further what the issue is there?
> >
> >Inside the logger subprocess, Redirect_stderr is guaranteed true (since
> >it'll be inherited from the postmaster) and therefore the proposed
> >change ensures that anything the logger might want to complain about
> >goes to the original stderr, ie, into the bit bucket rather than
> >someplace useful.  Perhaps something like
> >
> >    if ((!Redirect_stderr || am_syslogger) && pgwin32_is_service())
> >
> >would be reasonable.
>
> <snip lots of others>
>
> Here is an updated patch, that should take care of this. Tested that it
> solves the problem reported.
>
>
> >> There is special code in the send_message_to_server_log
> >function to make
> >> sure it's written directly to the file.
> >
> >If the logger is complaining, it's quite possibly because it's
> >unable to
> >write to its file.  Now that you mention it, doesn't this code go into
> >infinite recursion if write_syslogger_file_binary() tries to ereport?
> >
>
> I haven't looked at this part, it appears a separate (but closely
> related) issue.
>
> Perhaps Andreas can comment on this?
>
> //Magnus

Content-Description: stderr.patch

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: Have you checked our extensive FAQ?
>
>                http://www.postgresql.org/docs/faq

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

"Magnus Hagander" <mha@sollentuna.net> writes:
> Here is an updated patch, that should take care of this. Tested that it
> solves the problem reported.

I compared this to the version Bruce applied earlier and decided that
his version was good.  I don't think we should change the original logic
that treated write_syslogger_file as an independent special destination
for the syslogger process only.

I've backpatched that version to 8.0 branch.

>> If the logger is complaining, it's quite possibly because it's
>> unable to
>> write to its file.  Now that you mention it, doesn't this code go into
>> infinite recursion if write_syslogger_file_binary() tries to ereport?

> I haven't looked at this part, it appears a separate (but closely
> related) issue.

Actually, your change to make write_syslogger_file_binary() use
write_stderr seems like a fine solution to this problem.  However
you didn't get it quite right: the call has to be more like

    /* can't use ereport here because of possible recursion */
    if (rc != count)
        write_stderr("could not write to log file: %s\n", strerror(errno));

since write_stderr doesn't know about %m and doesn't supply a free
newline.

Applied and backpatched to 8.0.

            regards, tom lane