Fix error handling in getCopyDataMessage and pqFunctionCall3

On Wed, Jul 22, 2026 at 10:00 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:

Hi,

Currently, any OOM triggered within getNotify and getParameterStatus
will drop the connection and set error_result=true.

However, this error is currently ignored in 2 spots:
getCopyDataMessage and pqFunctionCall3. They will continue to process
the dead connection without reporting the error.

The attached patch fixes the issue by returning the error (-2 for
getCopyDataMessage, the PGresult for pqFunctionCall3) whenever a fatal
error was triggered.

Thanks for the patch.

I traced through this and it looks correct. I confirmed by code
inspection that the third
caller of these helpers, pqParseInput3(), does not need the same
change: it already
has the check, and handleFatalError() also flushes the input buffer and sets
asyncStatus = PGASYNC_READY, so it can't spin. The two functions you patched are
the only ones with private loops that ignored error_result, so the
scope looks complete.

+1 from me.

Regards,
Anthonin Bonnefoy

--
Regards,
Ewan Young

On Thu, Jul 23, 2026 at 4:02 PM Ewan Young <kdbase.hack@gmail.com> wrote:

+1 from me.

+1

I have one comment:

  /* Completed parsing this message, keep going */
  pqParseDone(conn, conn->inStart + 5 + msgLength);
  needInput = false;
+
+ /*
+ * An error may have been triggered while processing the message, bail
+ * out
+ */
+ if (conn->error_result && conn->status == CONNECTION_BAD)
+ return pqPrepareAsyncResult(conn);

getCopyDataMessage() checks for the fatal error before pqParseDone(),
but pqFunctionCall3() does it after pqParseDone(). Shouldn't
pqFunctionCall3() use the same ordering?

If getNotify() or getParameterStatus() calls handleFatalError(), the input
buffer has already been flushed, so calling pqParseDone() afterwards seems
unnecessary and a bit confusing.

Regards,

--
Fujii Masao

Thanks for the reviews!

On Thu, Jul 23, 2026 at 9:43 AM Fujii Masao <masao.fujii@gmail.com> wrote:

+ /*
+ * An error may have been triggered while processing the message, bail
+ * out
+ */
+ if (conn->error_result && conn->status == CONNECTION_BAD)
+ return pqPrepareAsyncResult(conn);

getCopyDataMessage() checks for the fatal error before pqParseDone(),
but pqFunctionCall3() does it after pqParseDone(). Shouldn't
pqFunctionCall3() use the same ordering?

If getNotify() or getParameterStatus() calls handleFatalError(), the input
buffer has already been flushed, so calling pqParseDone() afterwards seems
unnecessary and a bit confusing.

Right, I guess one possible reason would be to trace the message. But
on the other hand, pqParseDone is called for successfully parsed
messages, and other codepaths don't call pqParseDone on errors, so it
is definitely confusing.

I've updated the patch to move the error checks before pqParseDone in
both cases.

Regards,
Anthonin Bonnefoy

On Thu, Jul 23, 2026 at 5:19 PM Anthonin Bonnefoy
<anthonin.bonnefoy@datadoghq.com> wrote:

I've updated the patch to move the error checks before pqParseDone in
both cases.

Thanks for updating the patch!

Attached is an updated version.
I only revised the commit message, the code is unchanged.

Since seems this issue was introduced by commit f6f0542266f0, which was
backpatched to v18, I think this fix should be backpatched to v18 as well.
Thought?

Barring any objections, I'll commit it.

Regards,

--
Fujii Masao