Re: Patch: Allow formatting in log_line_prefix

On Tue, Aug 27, 2013 at 7:55 PM, Vik Fearing <vik(dot)fearing(at)dalibo(dot)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.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.

Regards

David