On Sat, Oct 10, 2015 at 9:06 PM, Amit Kapila wrote:
> Okay, in that case, you can revert that change in your first patch and
> then that patch will be Ready for committer.
Yep. Done.
> In second patch [1], the handling is to continue on error as below:
>
> - if (getParamDescriptions(conn))
> + if (getParamDescriptions(conn, msgLength))
> return;
> - break;
> + /* getParamDescriptions() moves inStart itself */
> + continue;
>
> Now, assume there is "out of memory" error in getParamDescription(),
> the next message is 'T' which leads to a call to getRowDescriptions()
> and in that function if there is an error in below part of code, then I
> think it will just overwrite the previous "out of memory" error (I have
> tried by manual debugging and forcefully generating the below error):
> Here, the point to note is that for similar handling of error from
> getRowDescriptions(), we just skip the consecutive 'D' messages
> by checking resultStatus.
> I think overwriting of error for this case is not the right behaviour.
> [1] - 0002-Fix-OOM-error-handling-in-BIND-protocol-of-libpq-2.patch
Yeah, this behavior is caused by this piece of code:
@@ -600,7 +601,9 @@ getRowDescriptions(PGconn *conn, int msgLength)
advance_and_error:
/* Discard unsaved result, if any */
if (result && result != conn->result)
PQclear(result);
An idea may be to check for conn->result->resultStatus !=
PGRES_FATAL_ERROR to concatenate correctly the error messages using
pqCatenateResultError. But just returning the first error encountered
as you mention would be more natural. So I updated the patch this way.
Regards,
--
Michael
