Re: PQexec() hangs on OOM

On Sat, Aug 29, 2015 at 10:35 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
wrote:

> On Thu, Jul 9, 2015 at 6:34 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com>
> wrote:
> >
> > On Wed, Jul 8, 2015 at 1:01 AM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
> wrote:
> > > On 07/07/2015 09:32 AM, Michael Paquier wrote:
> > >
> > >
> > > Ok, committed and backpatched the latest patch I posted. Yeah, we'll
> need
> > > additional patches for the refactoring and the remaining issues. I'm
> not
> > > sure if it's worth trying to backpatch them; let's see how big the
> patch is.
> >
> > So, here are two patches aimed at fixing the hangling issues with
> > getStartCopy and getParamDescriptions. After trying a couple of
> > approaches, I found out that the most elegant way to prevent the
> > infinite loop in parseInput is to introduce a new PGASYNC flag to
> > control the error and let the caller know what happened.
>
> One thing that looks slightly non-elegant about this approach is that
> new async status (PGASYNC_FATAL) is used to deal with errors only
> in few paths in function pqParseInput3() and not-other paths which should
> be okay if there is no other better way.
>

I considered using this logic in more code paths, but I kept focused on
having a not-too-invasive back-patchable patch as the refactoring that
would occur may be too heavy for what is usually pushed to the stable
branches. Do you think it would be better to get a cleaner refactoring
patch first and globalize the approach with PGASYNC_FATAL? As this is a
flag internal to libpq not exposed to the user this is fine on a code base,
but I am worrying about putting in stable branches more complexity than
really needed (upthread I said the same thing and Heikki mentioned that it
is fine as long as it is easy to backpatch). Note that I don't mind
spending more time on it, or to put in other works reworking the whole
patch to have something fully refactored, my goal being to have a clean fix
for all supported versions for those OOM problems.

I have not spent enough time on
> it to suggest any better alternative, but would like to hear what other
> approaches you have considered?
>

The other approach I have considered was to use the error string status to
decide if a failure happened, and this did not finish well in PQgetResult
:) It was actually a far cleaner approach to have a failure flag to decide
if based on the async state process should move on to a failure code path
or not.
--
Michael