Re: TODO list

Bruce Momjian wrote:

>Andrew Dunstan wrote:
>  
>
>>Bruce Momjian wrote:
>>
>>    
>>
>>>Andrew Dunstan wrote:
>>> 
>>>
>>>      
>>>
>>>>I thought we had thrashed this out back in August. Certainly the only
>>>>thing I recall seeing after I submitted the patch was some stylistic
>>>>criticism from Neil, which I addressed in a revised patch.
>>>>
>>>>Anyway, it is in principle doable. That's partly why I adopted a printf
>>>>style format string. There are some wrinkles, though:
>>>>. interaction with syslog pid/timestamp logging
>>>>   
>>>>
>>>>        
>>>>
>>>Yes.  If you use syslog, just don't ask for pid/timestamp and let syslog
>>>do it.  Of course, right now we are able to send non-pid/timestamp to
>>>syslog _and_ send pid/timestamp to a log file, but that seems like a
>>>rare operation that doesn't justify keeping the various log parameters
>>>separate.
>>>
>>>      
>>>
>>
>>I'm OK with that as long as it is the consensus view. It does seem a 
>>little odd to remove functionality (however rare) for the sake of 
>>configuration neatness, though.
>>    
>>
>
>Yes, agreed.  Let's explore it.  The rare functionality we would be
>removing is for:
>    
>    o  User uses syslog
>    o  User wants to log postmaster output to a file too
>    o  User wants timestamp info in the postmaster output file
>
>And the downside is that they get duplicate timestamps in syslog.  
>
>Now, if we don't merge timestamp into your new per-line log string, we
>end up with a rather illogical and inflexible output format, only to
>allow the rare case listed above.
>
>Clearly this isn't a 100% clear decision, but it seems to lean in the
>direction of removing the functionality with the goal of providing a
>more logical logging API to the users.
>
>  
>
>>>Also, I would like to see some kind of session identifier that is more
>>>unique than pid, which wraps around.  Ideally we could have 10{pid},
>>>then then the pid wraps around, 20{pid), or something like that.
>>>
>>>      
>>>
>>This requires some thought. ISTM it wouldn't buy you much unless you 
>>made it persistent across server restarts, and possibly not even then. I 
>>don't see it as a reason to hold up these features, though. If someone 
>>wants to tackle this it could be plugged in to the loginfo feature very 
>>easily as an extra escape sequence.
>>    
>>
>
>Yes, that was my idea --- let's get this in and we can then add a
>session id, and your point about restarts is a good one.
>
>  
>
>>>>. making sure the info is available when you need to log it - I had to
>>>>rearrange a few thing to avoid getting SEGVs, IIRC.
>>>>   
>>>>
>>>>        
>>>>
>>>Of course some messages, like postmaster status messages, don't have
>>>some of these fields, like username or host.  Is that going to cause
>>>problems for tools that read our log files?
>>>
>>>      
>>>
>>If users want a non-empty info string they will have to teach the tools 
>>to handle it anyway. The point was, however, that rolling up PID and 
>>timestamp into the printf-style format will require some significant 
>>work, because we wouldn't want to lose that info if the user/db weren't 
>>known, whereas the patch currently suppresses all log-info output if 
>>these are not present (i.e. when MyProcPort == NULL).
>>    
>>
>
>Oh, good point.  That would suggest that maybe we are better off leaving
>pid and timestamp as separate options and _not_ merge them into your new
>string.  I am starting to think having your string print only
>session-specific information is the best way.
>
>I wonder if we should rename your GUC log_session_line or something like
>that.
>
>  
>
>>>>Also, the session duration logging part of the patch is orthogonal to the
>>>>issue.
>>>>        
>>>>
>>>You mean query duration?  Yes, it is orthoginal.
>>>      
>>>
>>No, I meant the logging of the end of a session, including its duration, 
>>which was also in the patch.
>>    
>>
>
>Oh, I missed that one.  It seems like a nice addition.  I see you added
>it when they ask for log_connections.  Good idea.
>


I think you are looking at the original patch, not the revised patch, 
which is here: http://candle.pha.pa.us/mhonarc/patches2/msg00091.html 
and provides a separate GUC var called "log_session_end" - Neil wanted 
these not to be combined, IIRC.

I am agnostic as to the names of the variables.

cheers

andrew