Hi all, (CC-ing Joe as of dc7d70e) I was just looking at the offline checksum patch, and noticed some sloppy coding in controldata_utils.c. The control file gets opened in get_controlfile(), and if it generates an error then the file descriptor remains open. As basic code rule in the backend we should only use BasicOpenFile() when opening files, so I think that the issue should be fixed as attached, even if this requires including fd.h for the backend compilation which is kind of ugly.
Another possibility would be to just close the file descriptor before any error, saving appropriately errno before issuing any %m portion, but that still does not respect the backend policy regarding open(). We also do not have a wait event for the read() call, maybe we should have one, but knowing that this gets called only for the SQL-level functions accessing the control file, I don't really think that's worth it. Thoughts? -- Michael
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c index abfe7065f5..0b011741e3 100644 --- a/src/common/controldata_utils.c +++ b/src/common/controldata_utils.c @@ -27,6 +27,9 @@ #include "catalog/pg_control.h" #include "common/controldata_utils.h" #include "port/pg_crc32c.h" +#ifndef FRONTEND +#include "storage/fd.h" +#endif /* * get_controlfile(char *DataDir, const char *progname, bool *crc_ok_p) @@ -45,13 +48,20 @@ get_controlfile(const char *DataDir, const char *progname, bool *crc_ok_p) char ControlFilePath[MAXPGPATH]; pg_crc32c crc; int r; + int flags = O_RDONLY | PG_BINARY; AssertArg(crc_ok_p); ControlFile = palloc(sizeof(ControlFileData)); snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir); - if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1) +#ifndef FRONTEND + fd = BasicOpenFile(ControlFilePath, flags); +#else + fd = open(ControlFilePath, flags, 0); +#endif + + if (fd < 0) #ifndef FRONTEND ereport(ERROR, (errcode_for_file_access(),
signature.asc
Description: PGP signature
