> On 22 Nov 2023, at 13:47, Alvaro Herrera <alvhe...@alvh.no-ip.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
v4-0001-Refactor-pipe_read_line-to-return-the-full-line.patch
Description: Binary data
