On 08/27/2013 05:06 AM, David Rowley wrote: > Heikki wrote that it might be useful to allow formatting in the > log_line_prefix here > http://www.postgresql.org/message-id/5187CADB.50306@vmware.com > > I thought I'd take a bash at implementing space padding part on > log_line_prefix options. > > It's been a while since I worked with the PostgreSQL source, so please > forgive me if my coding style is a bit off or the patch is not quite > in the correct format. > > It's a little rough around the edges at the moment and likely the > documentation needs more work, but I'm at the stage of wanting to know > if this is even wanted or needed by people. > > If you apply this patch you can do things like have %10u %-10d in your > log_line_prefix which will include spaces to give the option the > number you specify as the minimum width. Left and right aligning is > supported. > > Let this be the thread to collect consensus to whether this needed and > useful.
I was just working on this last week! I didn't get a chance to post it because I was still polishing it. I first took the same approach as you but decided it reduced the clarity of the code so I re-did it a different way. I'll attach it here along with yours to see which way others like it.
I guess it's not too surprising someone else took this up too.
I was not too happy either at having to edit code for each option, even more so with the temp buffers I had to create in 2 places where 2 variables we used in the formatting.
I did not do any performance testing yet. I didn't think the regression would be very big with my patch as I'm only doing extra copying for %c and %r. I had also removed the strlen() before the loop starts, which I thought might actually speed the whole thing up a bit.
I've not tested applying your patch yet and I'm not the best at reading context diffs to tell, but I get the impression that we both decided to pad even when the variable is not valid, e.g. the host and database being empty during start-up.
Did you test performance? You should add your patch to the next commitfest. -- Vik
I could add it, but I'm just thinking it's a bit strange to have 2 patches in a commitfest which do the same thing, but saying that we both went about it quite differently, I don't see a way to take the best out of both and make one. For me it looks like a bit of a choice between performance and more compact and cleaner code. I didn't think processing the log_line_prefix would be too big a hotspot, but I know performance regression is normally avoided at great cost, if necessary. Perhaps someone else can chime in here with some advice about preferences.
I could perhaps run a few benchmarks to test performance of the 2 of someone thought it was a good idea.
