On Tue, Aug 27, 2013 at 7:55 PM, Vik Fearing <vik.fear...@dalibo.com> wrote:
> 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.50...@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. Regards David
