On 08/03/2023 00:05, Daniel Gustafsson wrote:
> When skimming through pg_rewind during a small review I noticed the use of
> pipe_read_line for reading arbitrary data from a pipe, the mechanics of which
> seemed odd.
>
> Commit 5b2f4afffe6 refactored find_other_exec() and broke out pipe_read_line()
> as a static convenience routine for reading a single line of output to catch a
> version number. Many years later, commit a7e8ece41 exposed it externally in
> order to read a GUC from postgresql.conf using "postgres -C ..". f06b1c598
> also make use of it for reading a version string much like find_other_exec().
> Funnily enough, while now used for arbitrary string reading the variable is
> still "pgver".
>
> Since the function requires passing a buffer/size, and at most size - 1 bytes
> will be read via fgets(), there is a truncation risk when using this for
> reading GUCs (like how pg_rewind does, though the risk there is slim to none).
Good point.
> If we are going to continue using this for reading $stuff from pipes, maybe we
> should think about presenting a nicer API which removes that risk? Returning
> an allocated buffer which contains all the output along the lines of the recent
> pg_get_line work seems a lot nicer and safer IMO.
+1
> /*
> * Execute a command in a pipe and read the first line from it. The returned
> * string is allocated, the caller is responsible for freeing.
> */
> char *
> pipe_read_line(char *cmd)
I think it's worth being explicit here that it's palloc'd, or malloc'd
in frontend programs, rather than just "allocated". Like in pg_get_line.
Other than that, LGTM.
--
Heikki Linnakangas
Neon (https://neon.tech)