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

"Magnus Hagander" <mha@sollentuna.net> writes:

What do you think of the following as a fix?

I dislike going through write_stderr because it requires an extra,
useless gettext() call, and because in the Windows case it imposes
an arbitrary length limit that is certain to be exceeded. The length
limit is probably OK for the existing limited usages of write_stderr,
but you can't assume that every log message the system ever prints
is going to be under 2K.

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.

regards, tom lane

What do you think of the following as a fix?

I dislike going through write_stderr because it requires an extra,
useless gettext() call,

It does? Isn't buf.data already "gettextified"? write_stderr just calls
vfprintf. Or is this an example of me not knowing how gettext works?

and because in the Windows case it imposes
an arbitrary length limit that is certain to be exceeded.
The length
limit is probably OK for the existing limited usages of write_stderr,
but you can't assume that every log message the system ever prints
is going to be under 2K.

We're not assuming that!
If the system is configured to log to eventlog, this code is never
called.
If the system is configured to log to a file (redirect_stderr), this
code is never called.

It is *only* called when:
- System has not read it's full configuration yet (which is the case we
are trying to address)
- System is configured for logging to stderr *without* capturing it to a
file *and* running as a service. I'd argue this is a configuration
error.

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?

There is special code in the send_message_to_server_log function to make
sure it's written directly to the file.

//Magnus

"Magnus Hagander" <mha@sollentuna.net> writes:

I dislike going through write_stderr because it requires an extra,
useless gettext() call,

It does?

First thing it does.

Isn't buf.data already "gettextified"?

Yeah; the gettext() call is against the "%s". (If it did gettext() on
the buffer data it would be outright wrong.) Useless, but potentially
a cycle-sucker under high log volume.

but you can't assume that every log message the system ever prints
is going to be under 2K.

We're not assuming that!

Yes you are (and no the "it's user error" argument doesn't impress me).
It's trivial to work around both these problems anyway; just a matter
of repeating the logic in-line instead of trying to use write_stderr.
The issue that's actually bothering me is:

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.

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?

regards, tom lane

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

Actually, I'd like it considered for 8.0.X as well. Right now if yuo
change the config file in a way that it won't load, you get no error
message. Which is cearly not very good. I'll certainly accept if you
think it's not safe enough for 8.0.x, but please consider it.

//Magnus

"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