Re: return value from pq_putmessage() is widely ignored

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> pq_putmessage() is a macro which calls a function that is normally
> socket_putmessage(), which returns either 0 on success or EOF in the
> case of failure. Most callers ignore the return value, sometimes with
> an explicit cast to void, and other times without such a cast. As far
> as I can see, the only case where we actually do anything significant
> with the return value is in basebackup.c, where two of the calls to
> pq_putmessage() do this:
> if (pq_putmessage('d', buf, cnt))
> ereport(ERROR,
> (errmsg("base
> backup could not send data, aborting backup")));

A preliminary survey says that basebackup.c is wrong here, and it
should be ignoring the return value just like the rest of the world.
pqformat.c is of the opinion that pqcomm.c is taking care of it:

(void) pq_putmessage(buf->cursor, buf->data, buf->len);
/* no need to complain about any failure, since pqcomm.c already did */

and in fact that appears to be the case. As far as I can see, the
only place that's doing anything appropriate with the result is
socket_putmessage_noblock:

res = pq_putmessage(msgtype, s, len);
Assert(res == 0); /* should not fail when the message fits in
* buffer */

Perhaps the value of that Assert is not worth the amount of
confusion generated by having a result value, and we should
just drop it and change pq_putmessage to return void.

> One problem is that we might get into error recursion trouble: we
> don't want to try to send an ErrorResponse, fail, and then respond by
> generating another ErrorResponse, which will again fail, leading to
> blowing out the error stack.

Yup. This is why it's dealt with internally to pqcomm.c.

regards, tom lane