Re: PQexec() hangs on OOM

On Mon, Aug 31, 2015 at 12:55 PM, Michael Paquier <michael(dot)paquier(at)gmail(dot)com
> wrote:
>
> On Sat, Aug 29, 2015 at 10:35 PM, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>
> wrote:
>>
>>
>> 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?
>

No wait, lets first try to see if this the best fix for Copy path.

The problem with the current approach is that even if we are able to
receive error, it doesn't completely clear the previous command
execution. As an example, if using debugger, I make getCopyStart()
return OOM error, then the next command execution fails.

postgres=# copy t1 from stdin;
out of memory
postgres=# copy t1 from stdin;
another command is already in progress

> 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 :)
>

Yeah, that could be another way and we already use that in
pqParseInput3() to distinguish the error path.

> 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.
>

I think you have already tried, but it seems to me that if we can handle
it based on result status, that would be better, rather than introducing
new state, but anyway lets first try to sort out above reported problem.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com