Thread: Proposal for better PQExpBuffer out-of-memory behavior

I've been chewing on the problem described here:
http://archives.postgresql.org/pgsql-general/2008-11/msg01220.php

It's not particularly easy to fix without making annoyingly large
changes to the API for PQExpBuffer.  The best idea I have come up
with so far goes like this:

* Upon failure to malloc or realloc the buffer for a PQExpBuffer,
the pqexpbuffer.c code should release whatever buffer it might have
had and setdata = pointer to empty, statically allocated stringlen = 0maxlen = 0
This is distinguishable from the normal non-error case because maxlen
can never be zero in non-error cases.

* All subsequent operations except resetPQExpBuffer will do nothing
to such a PQExpBuffer.  resetPQExpBuffer will attempt to restore the
string to "normal" empty status by allocating a new default-size buffer.


The result of this would be that in cases such as the one exhibited
by Sam Mason, we'd end up with a guaranteed-empty string rather than
one that had had subsections unexpectedly removed.  Also, we could
add out-of-memory checks to callers where it seems important to do so.

The main advantage of this approach is that it avoids making ABI breaks
(such as would occur if we added an error flag field to
PQExpBufferData).  The main disadvantage is the need to add explicit
error checks to callers anyplace we're not satisfied with just letting
the string become empty.

The only alternative that I can think of that avoids the latter
disadvantage is to allow the pqexpbuffer routines to abort on
out-of-memory (ie, printf(stderr) and exit(1)).  This seems pretty
unpleasant though for functions that are part of libpq's infrastructure.
In particular, although we could allow the calling application to
override such behavior via some sort of callback hook function, it's
far from clear what it could do instead without risking bizarre
misbehavior by libpq.

Comments?
        regards, tom lane

On Tue, Nov 25, 2008 at 10:33:05AM -0500, Tom Lane wrote:
> I've been chewing on the problem described here:
> http://archives.postgresql.org/pgsql-general/2008-11/msg01220.php
> 
> It's not particularly easy to fix without making annoyingly large
> changes to the API for PQExpBuffer.

Yup, I've just realized that my very naive suggestion have having a
matching function to return something useful wouldn't be good as almost
all the functions return void and would introduce the most enormous
duplicity.

> The best idea I have come up with so far goes like this:
> 
> * Upon failure to malloc or realloc the buffer for a PQExpBuffer,
> the pqexpbuffer.c code should release whatever buffer it might have
> had and set
>     data = pointer to empty, statically allocated string
>     len = 0
>     maxlen = 0
> This is distinguishable from the normal non-error case because maxlen
> can never be zero in non-error cases.
> 
> * All subsequent operations except resetPQExpBuffer will do nothing
> to such a PQExpBuffer.  resetPQExpBuffer will attempt to restore the
> string to "normal" empty status by allocating a new default-size buffer.

Sounds like a reasonable compromise.  Would it be better to have this
string be something like "## out of memory in enlargePQExpBuffer ##"?
That way, if something doesn't check correctly we've got some way to
determine where things went wrong rather than just ending up with an
empty string?  Or are strings the only special case and most other types
will bomb out upon receiving an empty literal.

> The only alternative that I can think of that avoids the latter
> disadvantage is to allow the pqexpbuffer routines to abort on
> out-of-memory (ie, printf(stderr) and exit(1)).  This seems pretty
> unpleasant though for functions that are part of libpq's infrastructure.

If there's no way to avoid the abort then this sounds nasty!

> In particular, although we could allow the calling application to
> override such behavior via some sort of callback hook function, it's
> far from clear what it could do instead without risking bizarre
> misbehavior by libpq.

It doesn't seem obvious to me how these callback functions could do
anything useful.  They would still need some way of returning an error
to the outside world which would imply some sort of mechanism, like the
one above, to allow this to happen.

I'd be happy writing a patch for this if you want.  There appear to be
a couple of thousand references to the PQExpBuffer code, but I can't
imagine needing to touch all of them.

 Sam