Re: PQexec() hangs on OOM

On Sat, Sep 12, 2015 at 6:11 PM, Amit Kapila <amit.kapila16@gmail.com> wrote:
> On Mon, Sep 7, 2015 at 1:40 PM, Michael Paquier <michael.paquier@gmail.com>
> wrote:
>>
>> On Sat, Sep 5, 2015 at 9:45 PM, Amit Kapila <amit.kapila16@gmail.com>
>> wrote:
>>
>> Now, to move into the serious things...
>>
>> +       /*
>> +        * Advance inStart to show that the copy related message has been
>> +        * processed.
>> +        */
>> +       conn->inStart = conn->inCursor;
>> This change...
>>
>> +                                       /* getCopyStart() moves
>> inStart itself */
>>                                         conn->asyncStatus =
>> PGASYNC_COPY_IN;
>> -                                       break;
>> +                                       continue;
>> ...  And this change are risky for a backpatch. And they actually
>> break the existing replication protocol

First, many thanks for providing your input! I am really looking
forward into fixing those problems appropriately.

> Can you please explain how will it break replication protocol?
> I have done the required handling for Copy Both mode as well in attached
> patch similar to what was done for other Copy modes in previous patch.
> Check if you still find it as broken for replication?

When not enough data has been received from the server, it seems that
we should PQclear the existing result, and leave immediately
pqParseInput3 with the status PGASYNC_BUSY to be able to wait for more
data. Your patch, as-is, is breaking that promise (and this comes from
the first versions of my patches). It seems also that we should not
write an error message on connection in this case to be consistent in
the behavior of back-branches. For the OOM case, we definitely need to
take the error code path though, as your patch is correctly doing, and
what mine legendary failed to do.

+       if (pqGetInt(&result->numAttributes, 2, conn))
+       {
+               errmsg = libpq_gettext("extraneous data in COPY start message");
+               goto advance_and_error;
+       }
Here an error message is not needed, and this message should have been
formulated as "insufficient data in blah" either way.

> I have only kept the changes for COPY modes, so that once we settle on
> those, I think similar changes could be done for getParamDescriptions()

Yeah, I looked into this one as well, resulting in patch 0002
attached. In this case we have the advantage to only receive data from
the server, so pqPrepareAsyncResult is handling the failure just fine.
Attached as well is the test case I used (similar to previous one,
just extended a bit to report the error message), after getting the
result from PQsendDescribePrepared, PQresultStatus is set correctly on
error and reports "out of memory" to the user. What do you think about
it?
Regards,
--
Michael