Re: PQexec() hangs on OOM

On Fri, Sep 18, 2015 at 11:25 PM, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>
wrote:

> On 09/14/2015 04:36 AM, Michael Paquier wrote:
>
> Patches 0001 and 0002 look good to me. Two tiny nitpicks:
>
> --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>> +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c
>> @@ -445,6 +445,7 @@ libpqrcv_PQexec(const char *query)
>> if (PQresultStatus(lastResult) == PGRES_COPY_IN ||
>> PQresultStatus(lastResult) == PGRES_COPY_OUT ||
>> PQresultStatus(lastResult) == PGRES_COPY_BOTH ||
>> + PQresultStatus(lastResult) == PGRES_FATAL_ERROR ||
>> PQstatus(streamConn) == CONNECTION_BAD)
>> break;
>> }
>>
>
> Is this really needed? AFAICS, the loop will terminate on
> PGRES_FATAL_ERROR too.

IIRC, this is required to sanely report "out of memory" error in case
of replication protocol (master-standby setup). This loop and in-particular
this check is quite similar to PQexecFinish() functions check and loop
where we return last result. I think it is better to keep both the places
in-sync
and also I think this is required to report the error appropriately. I have
tried manual debugging for the out of memory error for this case and
it works well with this check and without the check it doesn't report
the error in an appropriate way(don't remember exactly what was
the difference). If required, I can again try to reproduce the scenario
and share the exact report.

> Patches 0001 and 0002 look small enough that we probably could backpatch
> them, so that would leave 0003 as a optional master-only cleanup.
>
>
+
+advance_and_error:
+ /* Discard unsaved result, if any */

This part seems to be common in both patches and is also being
used in getRowDescriptions(), we can have a common routine for
that part, if you see any benefit in same.

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