Re: pipe_read_line for reading arbitrary strings

> On 22 Nov 2023, at 13:47, Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
>
> On 2023-Mar-07, Daniel Gustafsson wrote:
>
>> The attached POC diff replace fgets() with pg_get_line(), which may not be an
>> Ok way to cross the streams (it's clearly not a great fit), but as a POC it
>> provided a neater interface for reading one-off lines from a pipe IMO. Does
>> anyone else think this is worth fixing before too many callsites use it, or is
>> this another case of my fear of silent subtle truncation bugs? =)
>
> I think this is generally a good change.

Thanks for review!

> I think pipe_read_line should have a "%m" in the "no data returned"
> error message.

Good point.

> pg_read_line is careful to retain errno (and it was
> already zero at start), so this should be okay ... or should we set
> errno again to zero after popen(), even if it works?

While it shouldn't be needed, reading manpages from a variety of systems
indicates that popen() isn't entirely reliable when it comes to errno so I've
added an explicit errno=0 just to be certain.

> (I'm not sure I buy pg_read_line's use of perror in the backend case.
> Maybe this is only okay because the backend doesn't use this code?)

In EXEC_BACKEND builds the postmaster will use find_other_exec which in turn
calls pipe_read_line, so there is a possibility. I agree it's proabably not a
good idea, I'll have a look at it separately and will raise on a new thread.

> pg_get_line says caller can distinguish error from no-input-before-EOF
> with ferror(), but pipe_read_line does no such thing. I wonder what
> happens if an NFS mount containing a file being read disappears in the
> middle of reading it, for example ... will we think that we completed
> reading the file, rather than erroring out?

Interesting, that's an omission which should be fixed. I notice there are a
number of callsites using pg_get_line which skip validating with ferror(), I'll
have a look at those next (posting findings to a new thread).

--
Daniel Gustafsson