Re: Cleaner API for appendStringInfoVA

Attached patch moves decision how much more room to allocate
from callers of appendStringInfoVA to inside the function,
where more info is available.

On systems with broken vsnprintf() it falls back
to doubleing the buffer.

Fixme: the +1 could be something larger? Aligned?

--
marko

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> Attached patch moves decision how much more room to allocate
> from callers of appendStringInfoVA to inside the function,
> where more info is available.

This is by no stretch of the imagination "cleaner".

> On systems with broken vsnprintf() it falls back
> to doubleing the buffer.

The problem with this is that you are defining one particular vsnprintf
behavior as "non broken", without any evidence for that opinion.
(Indeed, one could argue that that behavior is contradictory to what
the Single Unix Spec says, although the SUS is a bit vague about it.)

Our own vsnprintf doesn't follow that behavior, for instance, so we
couldn't even get there by forcing it to be used always.

I'd want to see some significant evidence of a performance issue
before considering hacking this up like this.

regards, tom lane

On 11/23/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
> > Attached patch moves decision how much more room to allocate
> > from callers of appendStringInfoVA to inside the function,
> > where more info is available.
>
> This is by no stretch of the imagination "cleaner".

"Cleaner" as in avoiding unnecessary work and avoiding
unnecessary guesswork for callers when in can be done
in function.

Also conforming to current coding standards, see below.

> > On systems with broken vsnprintf() it falls back
> > to doubleing the buffer.
>
> The problem with this is that you are defining one particular vsnprintf
> behavior as "non broken", without any evidence for that opinion.
> (Indeed, one could argue that that behavior is contradictory to what
> the Single Unix Spec says, although the SUS is a bit vague about it.)

FWIW, SUS says that vsnprintf should act like snprintf and snprintf:

The snprintf() function shall be equivalent to sprintf(), with
the addition of the n argument which states the size of the
buffer referred to by s. If n is zero, nothing shall be written
and s may be a null pointer. Otherwise, output bytes beyond the
n-1st shall be discarded instead of being written to the array,
and a null byte is written at the end of the bytes actually
written into the array.

RETURN VALUE:

Upon successful completion, the snprintf() function shall return
the number of bytes that would be written to s had n been
sufficiently large excluding the terminating null byte.

> Our own vsnprintf doesn't follow that behavior, for instance, so we
> couldn't even get there by forcing it to be used always.

It's one of those broken implementations then.

> I'd want to see some significant evidence of a performance issue
> before considering hacking this up like this.

*shrug* It's a minor cleanup. I think it's worthwhile to remove
historical warts from code but if you are not interested, no problem.

Are you interested in fixing src/port/snprintf.c behaviour?
I can prepare a patch, it does not seem to be very hard.

--
marko

"Marko Kreen" <markokr(at)gmail(dot)com> writes:
> FWIW, SUS says that vsnprintf should act like snprintf and snprintf:

I dunno where you're reading that, but it's certainly nowhere to be
found in the version that I read:

http://www.opengroup.org/onlinepubs/007908799/xsh/vfprintf.html

regards, tom lane

On 11/23/07, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> "Marko Kreen" <markokr(at)gmail(dot)com> writes:
> > FWIW, SUS says that vsnprintf should act like snprintf and snprintf:
>
> I dunno where you're reading that, but it's certainly nowhere to be
> found in the version that I read:
>
> http://www.opengroup.org/onlinepubs/007908799/xsh/vfprintf.html

http://www.opengroup.org/onlinepubs/009695399/functions/vsnprintf.html

Seems its clarified in SUSv3.

--
marko

Patch rejected. Sorry. Comment is:

The patch of very dubious portability and I'm not even convinced that
it'd provide a net performance improvement.

---------------------------------------------------------------------------

Marko Kreen wrote:
> Attached patch moves decision how much more room to allocate
> from callers of appendStringInfoVA to inside the function,
> where more info is available.
>
> On systems with broken vsnprintf() it falls back
> to doubleing the buffer.
>
> Fixme: the +1 could be something larger? Aligned?
>
> --
> marko

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: Have you checked our extensive FAQ?
>
> http://www.postgresql.org/docs/faq

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://postgres.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +