pg_restore error message

Started by Euler Taveiraover 5 years ago5 messages
#1Euler Taveira
euler.taveira@2ndquadrant.com
1 attachment(s)

Hi,

While investigating a pg_restore error, I stumbled upon a message that is
not so useful.

pg_restore: error: could not close data file: No such file or directory

Which file? File name should be printed too like in the error check for
cfopen_read a few lines above.

Regards,

--
Euler Taveira http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Attachments:

0001-pg_restore-failure-message-does-not-provide-filename.patchtext/x-patch; charset=US-ASCII; name=0001-pg_restore-failure-message-does-not-provide-filename.patchDownload
From f3853512e827952402020b2b0f3003ac8c5c9d96 Mon Sep 17 00:00:00 2001
From: Euler Taveira <euler.taveira@2ndquadrant.com>
Date: Thu, 7 May 2020 18:17:28 -0300
Subject: [PATCH] pg_restore failure message does not provide filename

An error message like "pg_restore: error: could not close data file: No
such file or directory" is not informative without a file name. Since
error message for 'open' provides the file name (a few lines above), we
should also add it to 'close' error check.
---
 src/bin/pg_dump/pg_backup_directory.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/bin/pg_dump/pg_backup_directory.c b/src/bin/pg_dump/pg_backup_directory.c
index c9cce5ed8a..f178d6ac21 100644
--- a/src/bin/pg_dump/pg_backup_directory.c
+++ b/src/bin/pg_dump/pg_backup_directory.c
@@ -397,7 +397,7 @@ _PrintFileData(ArchiveHandle *AH, char *filename)
 
 	free(buf);
 	if (cfclose(cfp) !=0)
-		fatal("could not close data file: %m");
+		fatal("could not close data file \"%s\": %m", filename);
 }
 
 /*
-- 
2.20.1

#2Ranier Vilela
ranier.vf@gmail.com
In reply to: Euler Taveira (#1)
Re: pg_restore error message

Em qui., 7 de mai. de 2020 às 18:54, Euler Taveira <
euler.taveira@2ndquadrant.com> escreveu:

Hi,

While investigating a pg_restore error, I stumbled upon a message that is
not so useful.

pg_restore: error: could not close data file: No such file or directory

Which file? File name should be printed too like in the error check for
cfopen_read a few lines above.

Can suggest improvements?

1. free (398 line) must be pg_free(buf)';
2. %m, is a format to parameter, right?
But what parameter? Both fatal call, do not pass this parameter, or is
it implied?

regards,
Ranier Vilela

#3Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Euler Taveira (#1)
Re: pg_restore error message

On 2020-May-07, Euler Taveira wrote:

While investigating a pg_restore error, I stumbled upon a message that is
not so useful.

pg_restore: error: could not close data file: No such file or directory

Which file? File name should be printed too like in the error check for
cfopen_read a few lines above.

Thanks for reporting. Fix pushed to 9.5 and up.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Ranier Vilela (#2)
Re: pg_restore error message

On 2020-May-07, Ranier Vilela wrote:

Can suggest improvements?

1. free (398 line) must be pg_free(buf)';

Yeah, there's a lot of frontend code that uses free() instead of
pg_free(). There are too many of these that worrying about a single one
would not improve things much. I guess we could convert them all, but I
don't see much point.

2. %m, is a format to parameter, right?
But what parameter? Both fatal call, do not pass this parameter, or is
it implied?

%m is an implied "strerror(errno)", implemented by our snprintf
replacement.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#5Michael Paquier
michael@paquier.xyz
In reply to: Alvaro Herrera (#4)
Re: pg_restore error message

On Fri, May 08, 2020 at 07:45:16PM -0400, Alvaro Herrera wrote:

Yeah, there's a lot of frontend code that uses free() instead of
pg_free(). There are too many of these that worrying about a single one
would not improve things much. I guess we could convert them all, but I
don't see much point.

Doing a hard switch would have the disadvantage to create more
problems when back-patching. Even if such conflicts would be I guess
simple enough to address, that's less to worry about. I think however
that there is a point in switching to a more PG-like API if reworking
an area of the code for a new feature or a refactoring, but this is a
case-by-case judgement usually.

2. %m, is a format to parameter, right?
But what parameter? Both fatal call, do not pass this parameter, or is
it implied?

%m is an implied "strerror(errno)", implemented by our snprintf
replacement.

Originally, %m is a glibc extension, which has been added recently in
our port in src/port/snprintf.c as of d6c55de.
--
Michael