Re: Reasons not to like asprintf - Mailing list pgsql-hackers

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > Sounds reasonable, and I haven't got a better name, but I'm trying to
> > figure out why psprintf hasn't got the same issues which you mention in
> > your other thread (eg: depending on vsnprintf to return the "would-be"
> > result size).
>
> It does, though not directly in psprintf but rather in the underlying
> vasprintf implementation.  (Although psprintf is exposed to the problem
> that it can't tell out-of-memory from format errors.)

Right, pvsprintf().  Sorry for any confusion.

> What I'm on about in this thread is the API used by all the call sites,
> not the underlying implementation issues.  As long as we're okay with
> "exit or elog on any error", I think we should just have the widely-used
> functions returning a buffer pointer.  Underneath that, we need some work
> so that we can print more-specific error messages, but that will not be
> of concern to the callers.

I agree that exit/Assert-or-elog is the right API here.  I wonder if
it'd be reasonable or worthwhile to try and actually make that happen-
that is to say, we really do only have one implementation for both
front-end and back-end here, but on the front-end we exit, while on the
back-end we elog(ERROR).  I'm guessing that's a pipe-dream, but figured
I'd mention it anyway, in case people have crafty ideas about how that
might be made to work (and if others think it's even a good idea).

> The other possible class of failures is format string
> or encoding errors, but those seem to reduce to the same cases: I can't
> envision that frontend code would have any useful recovery strategies.
> I'd like the printed error message to distinguish these cases, but I don't
> think the callers need to care.

Agreed.

> > I'm also a bit nervous that we only check vsnprintf()
> > success in Assert-enabled builds in psprintf, though I suppose that's
> > all we're doing in appendStringInfo too.
>
> Actually, appendStringInfo treats result < 0 as a buffer overrun report;

I had been looking at the Assert() that the last character is always
'\0' in allocStringInfoVA.  Probably a stretch to say that has to be
upgraded to an elog(), but I'd certainly be much happier with a runtime
"did this error?" check of some kind than not; hence my support for your
errno notion below..

> if the failure is persistent because it's really a format/encoding
> problem, it'll loop till it runs out of memory, and then report the error
> as that.

Ah, yes, I see that in enlargeStringInfo and as long as we keep
MaxAllocSize reasonable that isn't terrible.

> (Hmm ...
> current POSIX requires *printf to set errno on error, so maybe we could
> look at errno?)

Sounds like a good idea to me..  Being explicit about checking for error
results, rather than hoping-against-hope that the only reason we'd ever
get a value less than the total length is when we need to provide more
memory, would be a lot better.  Perhaps it'd be worthwhile to run a test
against the build farm and see what kind of errno's are being set in
those cases where we're now getting the 'out of memory' failures you
mentioned upthread?
Thanks,
    Stephen