Re: elog() patch - Mailing list pgsql-hackers

Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I assume with your changes that the password will no longer be echoed to
> > the client on failure at debug5, right?
> 
> Nope.
> 
> I've noticed a bunch of breakage with this patch at high (or is it low?)
> debug output levels; for example client disconnection leaves this in the
> log:
>     DEBUG:  proc_exit(0)
>     DEBUG:  shmem_exit(0)
>     LOG:  pq_flush: send() failed: Broken pipe
>     DEBUG:  exit(0)
>     LOG:  pq_flush: send() failed: Bad file number
>     DEBUG:  reaping dead processes
>     DEBUG:  child process (pid 12462) exited with exit code 0
> The problem is that elog is still trying to send stuff to the client
> after client disconnection.  I propose to fix that by resetting
> whereToSendOutput to None as soon as we detect client disconnect.

That is exactly the solution I would recommend.  Clearly we are
stressing elog() and the client by forcing more information than we used
to.

> A more serious problem has to do with error reports for communication
> problems, for example this fragment in pqcomm.c:
> 
>             /*
>              * Careful: an elog() that tries to write to the client
>              * would cause recursion to here, leading to stack overflow
>              * and core dump!  This message must go *only* to the postmaster
>              * log.  elog(LOG) is presently safe.
>              */
>             elog(LOG, "pq_recvbuf: recv() failed: %m");
> 
> elog(LOG) is NOT safe anymore :-(.

Sure isn't.  We know we can always output to the server logs, but not
always to the client.


> I am thinking of inventing an additional elog level, perhaps called
> COMMERR, to be used specifically for reports of client communication
> trouble.  This could be treated the same as LOG as far as output to
> the server log goes, but we would hard-wire it to never be reported
> to the client (for fear of recursive failure).
> 
> Comments?

Couldn't we just set whereToSendOutput to None to fix this, or is there
a sense that we may be able to send messages later.

If needed, we can put COMMERR into the existing numbering.  I would be
glad to add it for you if you wish.  There would be no mention of it in
the docs because it is just like LOG but only to server logs.

> BTW, I am also looking at normalizing all the backend/libpq reports
> that look like
>         snprintf(PQerrormsg, ...);
>         fputs(PQerrormsg, stderr);
>         pqdebug("%s", PQerrormsg);
> to just use elog.  As-is this code is fairly broken since it doesn't
> honor the syslog option.  Any objections?

Yes, I never liked this stuff.  Please remove it.

--  Bruce Momjian                        |  http://candle.pha.pa.us pgman@candle.pha.pa.us               |  (610)
853-3000+  If your life is a hard drive,     |  830 Blythe Avenue +  Christ can be your backup.        |  Drexel Hill,
Pennsylvania19026