Patch: Allow formatting in log_line_prefix

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.

Regards

David Rowley

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.

Did you test performance? You should add your patch to the next commitfest.

--
Vik

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