pipe_read_line for reading arbitrary strings
Daniel Gustafsson <daniel@yesql.se>
From: Daniel Gustafsson <daniel@yesql.se>
To: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Date: 2023-03-07T22:05:12Z
Lists: pgsql-hackers
Attachments
- pipe_read_line.diff (application/octet-stream) patch
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). 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. 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? =) -- Daniel Gustafsson
Commits
-
Fix errorhandling for reading from a pipe
- be41a9b03807 17.0 landed
-
Refactor pipe_read_line to return the full line
- 5c7038d70bb9 17.0 landed
-
Add -c/--restore-target-wal to pg_rewind
- a7e8ece41cf7 13.0 cited
-
Here is a patch that fixes the pipes used in find_other_exec() when
- 5b2f4afffe69 8.0.0 cited