Re: logging enhancements, minor code cleanup

On Sun, Aug 10, 2003 at 06:19:05PM -0400, Andrew Dunstan wrote:
> Neil Conway wrote:
> >On Sat, Aug 09, 2003 at 07:22:56PM -0400, Andrew Dunstan wrote:
> Sure. Very easy. Nobody suggested it when I floated the idea of logging
> session end on the hackers list, but it's a simple change. How does
> 'log_session_end' sound? Or else we could make log_connections have
> levels - 0 = none, 1 = start, 2 = start + end. What's the concensus?

Either one works for me.

> >This patch should also update the documentation.
>
> I'll submit a doc patch when the code is accepted.

Fine with me -- at one point, I believe there was a policy that the docs
needed to be updated before a patch was committed (to avoid people
intending to update the docs but never remembering too), but I'm not
sure how strictely we're following that.

> >Can you add an assertion that end.tv_sec >= port->session_start.tv_sec
> >before
> >this line, please?
>
> Sure. Presumably to catch the situation where the machine's time is
> changed (backwards) during the session - an event with fairly low
> probability.

No -- an assertion should test a condition that always yields true, unless
some critical assumption about the state of the world has been violated
(i.e. something has gone wrong). Your point about changing the time is
a good one, although I agree the probability of it actually occurring
is very small. It's your call: if you want to worry about that case,
add actual code (not an assertion) to handle it.

> print_timestamp() and print_pid() in the same file use static buffers
> which they return, so I just copied the style more or less, making
> allowance for the fact I don't know the reasonable size at compile time.

Fair enough -- if the other code in that area uses the same style,
that's fine. It's worth noting that if the format string changes
between calls to the function, you may end up writing off the end of
your malloc'ed buffer -- since the GUC var can be set only after a
SIGHUP, I believe this can't happen, but IMHO it speaks to the
hokiness of using static variables unless really necessary.

> >Shouldn't the commented-out line have the default value? (which is "",
> >right?)
>
> Yes, default is "". I'll move the example to the comment, if you like.

Please do.

-Neil