Commit f002ed2b authored by Tom Lane's avatar Tom Lane

Improve error reporting in pg_upgrade's file copying/linking/rewriting.

The previous design for this had copyFile(), linkFile(), and
rewriteVisibilityMap() returning strerror strings, with the caller
producing one-size-fits-all error messages based on that.  This made it
impossible to produce messages that described the failures with any degree
of precision, especially not short-read problems since those don't set
errno at all.

Since pg_upgrade has no intention of continuing after any error in this
area, let's fix this by just letting these functions call pg_fatal() for
themselves, making it easy for each point of failure to have a suitable
error message.  Taking this approach also allows dropping cleanup code
that was unnecessary and was often rather sloppy about preserving errno.
To not lose relevant info that was reported before, pass in the schema name
and table name of the current table so that they can be included in the
error reports.

An additional problem was the use of getErrorText(), which was flat out
wrong for all but a couple of call sites, because it unconditionally did
"_dosmaperr(GetLastError())" on Windows.  That's only appropriate when
reporting an error from a Windows-native API, which only a couple of
the callers were actually doing.  Thus, even the reported strerror string
would be unrelated to the actual failure in many cases on Windows.
To fix, get rid of getErrorText() altogether, and just have call sites
do strerror(errno) instead, since that's the way all the rest of our
frontend programs do it.  Add back the _dosmaperr() calls in the two
places where that's actually appropriate.

In passing, make assorted messages hew more closely to project style
guidelines, notably by removing initial capitals in not-complete-sentence
primary error messages.  (I didn't make any effort to clean up places
I didn't have another reason to touch, though.)

Per discussion of a report from Thomas Kellerer.  Back-patch to 9.6,
but no further; given the relative infrequency of reports of problems
here, it's not clear it's worth adapting the patch to older branches.

Patch by me, but with credit to Alvaro Herrera for spotting the issue
with getErrorText's misuse of _dosmaperr().

Discussion: <nsjrbh$8li$1@blaine.gmane.org>
parent 5afcd2aa
......@@ -431,8 +431,8 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
SCRIPT_PREFIX, SCRIPT_EXT);
if ((script = fopen_priv(*analyze_script_file_name, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
*analyze_script_file_name, getErrorText());
pg_fatal("could not open file \"%s\": %s\n",
*analyze_script_file_name, strerror(errno));
#ifndef WIN32
/* add shebang header */
......@@ -486,8 +486,8 @@ create_script_for_cluster_analyze(char **analyze_script_file_name)
#ifndef WIN32
if (chmod(*analyze_script_file_name, S_IRWXU) != 0)
pg_fatal("Could not add execute permission to file \"%s\": %s\n",
*analyze_script_file_name, getErrorText());
pg_fatal("could not add execute permission to file \"%s\": %s\n",
*analyze_script_file_name, strerror(errno));
#endif
termPQExpBuffer(&user_specification);
......@@ -559,8 +559,8 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
prep_status("Creating script to delete old cluster");
if ((script = fopen_priv(*deletion_script_file_name, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
*deletion_script_file_name, getErrorText());
pg_fatal("could not open file \"%s\": %s\n",
*deletion_script_file_name, strerror(errno));
#ifndef WIN32
/* add shebang header */
......@@ -615,8 +615,8 @@ create_script_for_old_cluster_deletion(char **deletion_script_file_name)
#ifndef WIN32
if (chmod(*deletion_script_file_name, S_IRWXU) != 0)
pg_fatal("Could not add execute permission to file \"%s\": %s\n",
*deletion_script_file_name, getErrorText());
pg_fatal("could not add execute permission to file \"%s\": %s\n",
*deletion_script_file_name, strerror(errno));
#endif
check_ok();
......@@ -819,8 +819,8 @@ check_for_isn_and_int8_passing_mismatch(ClusterInfo *cluster)
{
found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
output_path, getErrorText());
pg_fatal("could not open file \"%s\": %s\n",
output_path, strerror(errno));
if (!db_used)
{
fprintf(script, "Database: %s\n", active_db->db_name);
......@@ -922,8 +922,8 @@ check_for_reg_data_type_usage(ClusterInfo *cluster)
{
found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
output_path, getErrorText());
pg_fatal("could not open file \"%s\": %s\n",
output_path, strerror(errno));
if (!db_used)
{
fprintf(script, "Database: %s\n", active_db->db_name);
......@@ -1013,8 +1013,8 @@ check_for_jsonb_9_4_usage(ClusterInfo *cluster)
{
found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
output_path, getErrorText());
pg_fatal("could not open file \"%s\": %s\n",
output_path, strerror(errno));
if (!db_used)
{
fprintf(script, "Database: %s\n", active_db->db_name);
......@@ -1089,8 +1089,8 @@ get_bin_version(ClusterInfo *cluster)
if ((output = popen(cmd, "r")) == NULL ||
fgets(cmd_output, sizeof(cmd_output), output) == NULL)
pg_fatal("Could not get pg_ctl version data using %s: %s\n",
cmd, getErrorText());
pg_fatal("could not get pg_ctl version data using %s: %s\n",
cmd, strerror(errno));
pclose(output);
......
......@@ -119,8 +119,8 @@ get_control_data(ClusterInfo *cluster, bool live_check)
fflush(stderr);
if ((output = popen(cmd, "r")) == NULL)
pg_fatal("Could not get control data using %s: %s\n",
cmd, getErrorText());
pg_fatal("could not get control data using %s: %s\n",
cmd, strerror(errno));
/* Only in <= 9.2 */
if (GET_MAJOR_VERSION(cluster->major_version) <= 902)
......
......@@ -191,7 +191,7 @@ pid_lock_file_exists(const char *datadir)
/* ENOTDIR means we will throw a more useful error later */
if (errno != ENOENT && errno != ENOTDIR)
pg_fatal("could not open file \"%s\" for reading: %s\n",
path, getErrorText());
path, strerror(errno));
return false;
}
......@@ -285,7 +285,7 @@ check_data_dir(const char *pg_data)
if (stat(subDirName, &statBuf) != 0)
report_status(PG_FATAL, "check for \"%s\" failed: %s\n",
subDirName, getErrorText());
subDirName, strerror(errno));
else if (!S_ISDIR(statBuf.st_mode))
report_status(PG_FATAL, "%s is not a directory\n",
subDirName);
......@@ -309,7 +309,7 @@ check_bin_dir(ClusterInfo *cluster)
/* check bindir */
if (stat(cluster->bindir, &statBuf) != 0)
report_status(PG_FATAL, "check for \"%s\" failed: %s\n",
cluster->bindir, getErrorText());
cluster->bindir, strerror(errno));
else if (!S_ISDIR(statBuf.st_mode))
report_status(PG_FATAL, "%s is not a directory\n",
cluster->bindir);
......@@ -352,7 +352,7 @@ validate_exec(const char *dir, const char *cmdName)
*/
if (stat(path, &buf) < 0)
pg_fatal("check for \"%s\" failed: %s\n",
path, getErrorText());
path, strerror(errno));
else if (!S_ISREG(buf.st_mode))
pg_fatal("check for \"%s\" failed: not an executable file\n",
path);
......
......@@ -19,9 +19,7 @@
#include <fcntl.h>
#ifndef WIN32
static int copy_file(const char *fromfile, const char *tofile);
#else
#ifdef WIN32
static int win32_pghardlink(const char *src, const char *dst);
#endif
......@@ -29,73 +27,29 @@ static int win32_pghardlink(const char *src, const char *dst);
/*
* copyFile()
*
* Copies a relation file from src to dst.
* Copies a relation file from src to dst.
* schemaName/relName are relation's SQL name (used for error messages only).
*/
const char *
copyFile(const char *src, const char *dst)
void
copyFile(const char *src, const char *dst,
const char *schemaName, const char *relName)
{
#ifndef WIN32
if (copy_file(src, dst) == -1)
#else
if (CopyFile(src, dst, true) == 0)
#endif
return getErrorText();
else
return NULL;
}
/*
* linkFile()
*
* Creates a hard link between the given relation files. We use
* this function to perform a true in-place update. If the on-disk
* format of the new cluster is bit-for-bit compatible with the on-disk
* format of the old cluster, we can simply link each relation
* instead of copying the data from the old cluster to the new cluster.
*/
const char *
linkFile(const char *src, const char *dst)
{
if (pg_link_file(src, dst) == -1)
return getErrorText();
else
return NULL;
}
#ifndef WIN32
static int
copy_file(const char *srcfile, const char *dstfile)
{
#define COPY_BUF_SIZE (50 * BLCKSZ)
int src_fd;
int dest_fd;
char *buffer;
int ret = 0;
int save_errno = 0;
if ((srcfile == NULL) || (dstfile == NULL))
{
errno = EINVAL;
return -1;
}
if ((src_fd = open(srcfile, O_RDONLY | PG_BINARY, 0)) < 0)
return -1;
if ((src_fd = open(src, O_RDONLY | PG_BINARY, 0)) < 0)
pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s\n",
schemaName, relName, src, strerror(errno));
if ((dest_fd = open(dstfile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
if ((dest_fd = open(dst, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR)) < 0)
{
save_errno = errno;
pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n",
schemaName, relName, dst, strerror(errno));
if (src_fd != 0)
close(src_fd);
errno = save_errno;
return -1;
}
/* copy in fairly large chunks for best efficiency */
#define COPY_BUF_SIZE (50 * BLCKSZ)
buffer = (char *) pg_malloc(COPY_BUF_SIZE);
......@@ -105,47 +59,62 @@ copy_file(const char *srcfile, const char *dstfile)
ssize_t nbytes = read(src_fd, buffer, COPY_BUF_SIZE);
if (nbytes < 0)
{
save_errno = errno;
ret = -1;
break;
}
pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s\n",
schemaName, relName, src, strerror(errno));
if (nbytes == 0)
break;
errno = 0;
if (write(dest_fd, buffer, nbytes) != nbytes)
{
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
errno = ENOSPC;
save_errno = errno;
ret = -1;
break;
pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s\n",
schemaName, relName, dst, strerror(errno));
}
}
pg_free(buffer);
close(src_fd);
close(dest_fd);
if (src_fd != 0)
close(src_fd);
#else /* WIN32 */
if (CopyFile(src, dst, true) == 0)
{
_dosmaperr(GetLastError());
pg_fatal("error while copying relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
schemaName, relName, src, dst, strerror(errno));
}
if (dest_fd != 0)
close(dest_fd);
#endif /* WIN32 */
}
if (save_errno != 0)
errno = save_errno;
return ret;
/*
* linkFile()
*
* Hard-links a relation file from src to dst.
* schemaName/relName are relation's SQL name (used for error messages only).
*/
void
linkFile(const char *src, const char *dst,
const char *schemaName, const char *relName)
{
if (pg_link_file(src, dst) < 0)
pg_fatal("error while creating link for relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
schemaName, relName, src, dst, strerror(errno));
}
#endif
/*
* rewriteVisibilityMap()
*
* Transform a visibility map file, copying from src to dst.
* schemaName/relName are relation's SQL name (used for error messages only).
*
* In versions of PostgreSQL prior to catversion 201603011, PostgreSQL's
* visibility map included one bit per heap page; it now includes two.
* When upgrading a cluster from before that time to a current PostgreSQL
......@@ -156,8 +125,9 @@ copy_file(const char *srcfile, const char *dstfile)
* remain set for the pages for which they were set previously. The
* all-frozen bits are never set by this conversion; we leave that to VACUUM.
*/
const char *
rewriteVisibilityMap(const char *fromfile, const char *tofile)
void
rewriteVisibilityMap(const char *fromfile, const char *tofile,
const char *schemaName, const char *relName)
{
int src_fd;
int dst_fd;
......@@ -172,24 +142,18 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile)
/* Compute number of old-format bytes per new page */
rewriteVmBytesPerPage = (BLCKSZ - SizeOfPageHeaderData) / 2;
if ((fromfile == NULL) || (tofile == NULL))
return "Invalid old file or new file";
if ((src_fd = open(fromfile, O_RDONLY | PG_BINARY, 0)) < 0)
return getErrorText();
pg_fatal("error while copying relation \"%s.%s\": could not open file \"%s\": %s\n",
schemaName, relName, fromfile, strerror(errno));
if (fstat(src_fd, &statbuf) != 0)
{
close(src_fd);
return getErrorText();
}
pg_fatal("error while copying relation \"%s.%s\": could not stat file \"%s\": %s\n",
schemaName, relName, fromfile, strerror(errno));
if ((dst_fd = open(tofile, O_RDWR | O_CREAT | O_EXCL | PG_BINARY,
S_IRUSR | S_IWUSR)) < 0)
{
close(src_fd);
return getErrorText();
}
pg_fatal("error while copying relation \"%s.%s\": could not create file \"%s\": %s\n",
schemaName, relName, tofile, strerror(errno));
/* Save old file size */
src_filesize = statbuf.st_size;
......@@ -218,9 +182,12 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile)
if ((bytesRead = read(src_fd, buffer, BLCKSZ)) != BLCKSZ)
{
close(dst_fd);
close(src_fd);
return getErrorText();
if (bytesRead < 0)
pg_fatal("error while copying relation \"%s.%s\": could not read file \"%s\": %s\n",
schemaName, relName, fromfile, strerror(errno));
else
pg_fatal("error while copying relation \"%s.%s\": partial page found in file \"%s\"\n",
schemaName, relName, fromfile);
}
totalBytesRead += BLCKSZ;
......@@ -288,11 +255,14 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile)
((PageHeader) new_vmbuf)->pd_checksum =
pg_checksum_page(new_vmbuf, new_blkno);
errno = 0;
if (write(dst_fd, new_vmbuf, BLCKSZ) != BLCKSZ)
{
close(dst_fd);
close(src_fd);
return getErrorText();
/* if write didn't set errno, assume problem is no disk space */
if (errno == 0)
errno = ENOSPC;
pg_fatal("error while copying relation \"%s.%s\": could not write file \"%s\": %s\n",
schemaName, relName, tofile, strerror(errno));
}
/* Advance for next new page */
......@@ -306,8 +276,6 @@ rewriteVisibilityMap(const char *fromfile, const char *tofile)
pg_free(new_vmbuf);
close(dst_fd);
close(src_fd);
return NULL;
}
void
......@@ -320,16 +288,16 @@ check_hard_link(void)
snprintf(new_link_file, sizeof(new_link_file), "%s/PG_VERSION.linktest", new_cluster.pgdata);
unlink(new_link_file); /* might fail */
if (pg_link_file(existing_file, new_link_file) == -1)
{
pg_fatal("Could not create hard link between old and new data directories: %s\n"
if (pg_link_file(existing_file, new_link_file) < 0)
pg_fatal("could not create hard link between old and new data directories: %s\n"
"In link mode the old and new data directories must be on the same file system volume.\n",
getErrorText());
}
strerror(errno));
unlink(new_link_file);
}
#ifdef WIN32
/* implementation of pg_link_file() on Windows */
static int
win32_pghardlink(const char *src, const char *dst)
{
......@@ -338,7 +306,10 @@ win32_pghardlink(const char *src, const char *dst)
* http://msdn.microsoft.com/en-us/library/aa363860(VS.85).aspx
*/
if (CreateHardLinkA(dst, src, NULL) == 0)
{
_dosmaperr(GetLastError());
return -1;
}
else
return 0;
}
......@@ -353,7 +324,8 @@ fopen_priv(const char *path, const char *mode)
FILE *fp;
fp = fopen(path, mode);
umask(old_umask);
umask(old_umask); /* we assume this can't change errno */
return fp;
}
......@@ -213,9 +213,9 @@ check_loadable_libraries(void)
found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("Could not open file \"%s\": %s\n",
output_path, getErrorText());
fprintf(script, "Could not load library \"%s\"\n%s\n",
pg_fatal("could not open file \"%s\": %s\n",
output_path, strerror(errno));
fprintf(script, "could not load library \"%s\":\n%s\n",
lib,
PQerrorMessage(conn));
}
......
......@@ -422,8 +422,8 @@ adjust_data_dir(ClusterInfo *cluster)
if ((output = popen(cmd, "r")) == NULL ||
fgets(cmd_output, sizeof(cmd_output), output) == NULL)
pg_fatal("Could not get data directory using %s: %s\n",
cmd, getErrorText());
pg_fatal("could not get data directory using %s: %s\n",
cmd, strerror(errno));
pclose(output);
......
......@@ -225,7 +225,7 @@ setup(char *argv0, bool *live_check)
/* get path to pg_upgrade executable */
if (find_my_exec(argv0, exec_path) < 0)
pg_fatal("Could not get path name to pg_upgrade: %s\n", getErrorText());
pg_fatal("%s: could not find own program executable\n", argv0);
/* Trim off program name and keep just path */
*last_dir_separator(exec_path) = '\0';
......
......@@ -367,10 +367,12 @@ bool pid_lock_file_exists(const char *datadir);
/* file.c */
const char *copyFile(const char *src, const char *dst);
const char *linkFile(const char *src, const char *dst);
const char *rewriteVisibilityMap(const char *fromfile, const char *tofile);
void copyFile(const char *src, const char *dst,
const char *schemaName, const char *relName);
void linkFile(const char *src, const char *dst,
const char *schemaName, const char *relName);
void rewriteVisibilityMap(const char *fromfile, const char *tofile,
const char *schemaName, const char *relName);
void check_hard_link(void);
FILE *fopen_priv(const char *path, const char *mode);
......@@ -431,7 +433,6 @@ void pg_fatal(const char *fmt,...) pg_attribute_printf(1, 2) pg_attribute_noret
void end_progress_output(void);
void prep_status(const char *fmt,...) pg_attribute_printf(1, 2);
void check_ok(void);
const char *getErrorText(void);
unsigned int str2uint(const char *str);
void pg_putenv(const char *var, const char *val);
......
......@@ -183,7 +183,6 @@ transfer_single_new_db(FileNameMap *maps, int size, char *old_tablespace)
static void
transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_frozenbit)
{
const char *msg;
char old_file[MAXPGPATH];
char new_file[MAXPGPATH];
int segno;
......@@ -229,7 +228,7 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
else
pg_fatal("error while checking for file existence \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
map->nspname, map->relname, old_file, new_file,
getErrorText());
strerror(errno));
}
/* If file is empty, just return */
......@@ -242,35 +241,24 @@ transfer_relfile(FileNameMap *map, const char *type_suffix, bool vm_must_add_fro
/* Copying files might take some time, so give feedback. */
pg_log(PG_STATUS, "%s", old_file);
if (user_opts.transfer_mode == TRANSFER_MODE_COPY)
if (vm_must_add_frozenbit && strcmp(type_suffix, "_vm") == 0)
{
pg_log(PG_VERBOSE, "copying \"%s\" to \"%s\"\n", old_file, new_file);
/* Rewrite visibility map if needed */
if (vm_must_add_frozenbit && (strcmp(type_suffix, "_vm") == 0))
msg = rewriteVisibilityMap(old_file, new_file);
else
msg = copyFile(old_file, new_file);
if (msg)
pg_fatal("error while copying relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
map->nspname, map->relname, old_file, new_file, msg);
/* Need to rewrite visibility map format */
pg_log(PG_VERBOSE, "rewriting \"%s\" to \"%s\"\n",
old_file, new_file);
rewriteVisibilityMap(old_file, new_file, map->nspname, map->relname);
}
else if (user_opts.transfer_mode == TRANSFER_MODE_COPY)
{
pg_log(PG_VERBOSE, "copying \"%s\" to \"%s\"\n",
old_file, new_file);
copyFile(old_file, new_file, map->nspname, map->relname);
}
else
{
pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"\n", old_file, new_file);
/* Rewrite visibility map if needed */
if (vm_must_add_frozenbit && (strcmp(type_suffix, "_vm") == 0))
msg = rewriteVisibilityMap(old_file, new_file);
else
msg = linkFile(old_file, new_file);
if (msg)
pg_fatal("error while creating link for relation \"%s.%s\" (\"%s\" to \"%s\"): %s\n",
map->nspname, map->relname, old_file, new_file, msg);
pg_log(PG_VERBOSE, "linking \"%s\" to \"%s\"\n",
old_file, new_file);
linkFile(old_file, new_file, map->nspname, map->relname);
}
}
return;
}
......@@ -90,8 +90,8 @@ get_tablespace_paths(void)
os_info.old_tablespaces[tblnum]);
else
report_status(PG_FATAL,
"cannot stat() tablespace directory \"%s\": %s\n",
os_info.old_tablespaces[tblnum], getErrorText());
"could not stat tablespace directory \"%s\": %s\n",
os_info.old_tablespaces[tblnum], strerror(errno));
}
if (!S_ISDIR(statBuf.st_mode))
report_status(PG_FATAL,
......
......@@ -232,21 +232,6 @@ get_user_info(char **user_name_p)
}
/*
* getErrorText()
*
* Returns the text of the most recent error
*/
const char *
getErrorText(void)
{
#ifdef WIN32
_dosmaperr(GetLastError());
#endif
return pg_strdup(strerror(errno));
}
/*
* str2uint()
*
......
......@@ -52,7 +52,8 @@ new_9_0_populate_pg_largeobject_metadata(ClusterInfo *cluster, bool check_mode)
PQExpBufferData connectbuf;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s\n", output_path, getErrorText());
pg_fatal("could not open file \"%s\": %s\n", output_path,
strerror(errno));
initPQExpBuffer(&connectbuf);
appendPsqlMetaConnect(&connectbuf, active_db->db_name);
......@@ -150,7 +151,8 @@ old_9_3_check_for_line_data_type_usage(ClusterInfo *cluster)
{
found = true;
if (script == NULL && (script = fopen_priv(output_path, "w")) == NULL)
pg_fatal("could not open file \"%s\": %s\n", output_path, getErrorText());
pg_fatal("could not open file \"%s\": %s\n", output_path,
strerror(errno));
if (!db_used)
{
fprintf(script, "Database: %s\n", active_db->db_name);
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment