On Thu, Jan 26, 2012 at 11:48 PM, Robert Haas <robertmh...@gmail.com> wrote:
> On Tue, Jan 24, 2012 at 4:39 AM, Thomas Ogrisegg
> <tom-...@patches.fnord.at> wrote:
>> While testing a backup script, I noticed that pg_basebackup exits with
>> 0, even if it had errors while writing the backup to disk when the
>> backup file is to be sent to stdout. The attached patch should fix this
>> problem (and some special cases when the last write fails).
>
> This part looks like a typo:
>
> + if (fflush (tarfile) != 0)
> + {
> + fprintf(stderr, _("%s:
> error flushing stdout: %s\n"),
> +
> strerror (errno));
> + disconnect_and_exit(1);
> + }
>
> The error is in flushing the tarfile, not stdout, right?
No, in this case tarfile is set to stdout as follows.
-------------
if (strcmp(basedir, "-") == 0)
{
#ifdef HAVE_LIBZ
if (compresslevel != 0)
{
ztarfile = gzdopen(dup(fileno(stdout)), "wb");
if (gzsetparams(ztarfile, compresslevel,
Z_DEFAULT_STRATEGY) != Z_OK)
{
fprintf(stderr, _("%s: could not set
compression level %d: %s\n"),
progname,
compresslevel, get_gz_error(ztarfile));
disconnect_and_exit(1);
}
}
else
#endif
tarfile = stdout;
}
-------------
I don't think that pg_basebackup really needs to fflush() stdout for
each file. Right?
-------------
#endif
if (tarfile != NULL)
- fclose(tarfile);
+ {
+ if (fclose(tarfile) != 0)
+ {
+ fprintf(stderr, _("%s: error
closing file \"%s\": %s\n"),
+ progname,
filename, strerror (errno));
+ disconnect_and_exit(1);
+ }
+ }
-------------
This message doesn't obey the PostgreSQL message style.
It's guaranteed that the tarfile must not be NULL here, so the above check of
tarfile is not required. The remaining code of pg_basebackup relies on this
assumption.
Attached patch removes the fflush() part, changes the log message and removes
the check of tarfile, as above.
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
***************
*** 584,589 **** ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
--- 584,590 ----
{
fprintf(stderr, _("%s: could not write to compressed file \"%s\": %s\n"),
progname, filename, get_gz_error(ztarfile));
+ disconnect_and_exit(1);
}
}
else
***************
*** 600,606 **** ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
if (strcmp(basedir, "-") == 0)
{
#ifdef HAVE_LIBZ
! if (ztarfile)
gzclose(ztarfile);
#endif
}
--- 601,607 ----
if (strcmp(basedir, "-") == 0)
{
#ifdef HAVE_LIBZ
! if (ztarfile != NULL)
gzclose(ztarfile);
#endif
}
***************
*** 609,617 **** ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
#ifdef HAVE_LIBZ
if (ztarfile != NULL)
gzclose(ztarfile);
#endif
! if (tarfile != NULL)
! fclose(tarfile);
}
break;
--- 610,625 ----
#ifdef HAVE_LIBZ
if (ztarfile != NULL)
gzclose(ztarfile);
+ else
#endif
! {
! if (fclose(tarfile) != 0)
! {
! fprintf(stderr, _("%s: could not close file \"%s\": %s\n"),
! progname, filename, strerror (errno));
! disconnect_and_exit(1);
! }
! }
}
break;
***************
*** 630,635 **** ReceiveTarFile(PGconn *conn, PGresult *res, int rownum)
--- 638,644 ----
{
fprintf(stderr, _("%s: could not write to compressed file \"%s\": %s\n"),
progname, filename, get_gz_error(ztarfile));
+ disconnect_and_exit(1);
}
}
else
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
