Re: BUG #1466: syslogger issues

Started by Magnus Haganderalmost 21 years ago6 messages

mha@sollentuna.net

almost 21 years ago

1 attachment(s)

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.

That's sort of what I thought, but without being certain at all.

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).

Ok. I've included these changes in the attached patch. Haven't tested
those specific codepaths, but the other changes still work...

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

It's already the default if you install from the MSI installer.

//Magnus

Attachments:

stderr.patchapplication/octet-stream; name=stderr.patchDownload

Index: backend/postmaster/syslogger.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/postmaster/syslogger.c,v
retrieving revision 1.12
diff -c -r1.12 syslogger.c
*** backend/postmaster/syslogger.c	1 Jan 2005 20:44:16 -0000	1.12
--- backend/postmaster/syslogger.c	21 Feb 2005 19:49:55 -0000
***************
*** 698,706 ****
  #endif
  
  	if (rc != count)
! 		ereport(LOG,
! 				(errcode_for_file_access(),
! 				 errmsg("could not write to log file: %m")));
  }

  #ifdef WIN32
--- 698,704 ----
  #endif

  	if (rc != count)
! 		write_stderr("could not write to log file: %m");
  }

  #ifdef WIN32
Index: backend/utils/error/elog.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/backend/utils/error/elog.c,v
retrieving revision 1.155
diff -c -r1.155 elog.c
*** backend/utils/error/elog.c	31 Dec 2004 22:01:27 -0000	1.155
--- backend/utils/error/elog.c	21 Feb 2005 19:51:11 -0000
***************
*** 1630,1640 ****
  #endif   /* WIN32 */
  	/* Write to stderr, if enabled */
  	if ((Log_destination & LOG_DESTINATION_STDERR) || whereToSendOutput == Debug)
! 		fprintf(stderr, "%s", buf.data);
!
! 	/* If in the syslogger process, try to write messages direct to file */
! 	if (am_syslogger)
! 		write_syslogger_file(buf.data, buf.len);

  	pfree(buf.data);
  }
--- 1630,1653 ----
  #endif   /* WIN32 */
  	/* Write to stderr, if enabled */
  	if ((Log_destination & LOG_DESTINATION_STDERR) || whereToSendOutput == Debug)
! 	{
! 		/* If in the syslogger process, try to write messages direct to file */
! 		if (am_syslogger)
! 			write_syslogger_file(buf.data, buf.len);
! 		else
! 		{
! #ifdef WIN32
! 			/* In a win32 service environment, there is no usable stderr. Capture
! 			   anything going there and write it to the eventlog instead.
! 			   If stderr redirection is active, leave it to stderr because the
! 			   logger will capture it to a file. */
! 			if (!Redirect_stderr && pgwin32_is_service())
! 				write_eventlog(EVENTLOG_ERROR_TYPE, buf.data);
! 			else
! #endif
! 				fprintf(stderr, "%s", buf.data);
! 		}
! 	}

  	pfree(buf.data);
  }

Bruce Momjian

pgman@candle.pha.pa.us

almost 21 years ago

In reply to: Magnus Hagander (#1)

Previous version of patch removed from queue.

Your patch has been added to the PostgreSQL unapplied patches list at:

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

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.

That's sort of what I thought, but without being certain at all.

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).

Ok. I've included these changes in the attached patch. Haven't tested
those specific codepaths, but the other changes still work...

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

It's already the default if you install from the MSI installer.

//Magnus

Content-Description: stderr.patch

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  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

Bruce Momjian

pgman@candle.pha.pa.us

almost 21 years ago

In reply to: Magnus Hagander (#1)

Re: [PATCHES] BUG #1466: syslogger issues

I would like to apply this patch, and I think it is important enough
that it should be backpatched in to 8.0.X. Any objections?

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

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.

That's sort of what I thought, but without being certain at all.

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).

Ok. I've included these changes in the attached patch. Haven't tested
those specific codepaths, but the other changes still work...

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

It's already the default if you install from the MSI installer.

//Magnus

Content-Description: stderr.patch

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  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

Tom Lane

tgl@sss.pgh.pa.us

almost 21 years ago

In reply to: Bruce Momjian (#3)

Re: [PATCHES] BUG #1466: syslogger issues

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I would like to apply this patch, and I think it is important enough
that it should be backpatched in to 8.0.X. Any objections?

I wanted to review the patch before it went in. Will try to get to it
soon.

regards, tom lane

Bruce Momjian

pgman@candle.pha.pa.us

almost 21 years ago

In reply to: Tom Lane (#4)

Re: [PATCHES] BUG #1466: syslogger issues

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

I would like to apply this patch, and I think it is important enough
that it should be backpatched in to 8.0.X. Any objections?

I wanted to review the patch before it went in. Will try to get to it
soon.

OK, I will just keep it in the patch queue.

-- 
  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

Bruce Momjian

pgman@candle.pha.pa.us

almost 21 years ago

In reply to: Magnus Hagander (#1)

Patch applied by Tom. Thanks.

Backpatched to 8.0.X.

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

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.

That's sort of what I thought, but without being certain at all.

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).

Ok. I've included these changes in the attached patch. Haven't tested
those specific codepaths, but the other changes still work...

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

It's already the default if you install from the MSI installer.

//Magnus

Content-Description: stderr.patch

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  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