Re: [HACKERS] PATCH: pg_basebackup (missing exit on error)
On Tue, Mar 27, 2012 at 7:13 AM, Fujii Masao masao.fu...@gmail.com wrote: Attached patch removes the fflush() part, changes the log message and removes the check of tarfile, as above. With this patch applied, we end up with: if (strcmp(basedir, -) == 0) { #ifdef HAVE_LIBZ if (ztarfile != NULL) gzclose(ztarfile); #endif } else { #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); } } } I think it would make sense to rearrange that so that we don't have two tests for ztarfile != NULL; do that test first, and then if it fails, do the strcmp after that. Also, if we're going to test the return value of fclose(), shouldn't we also be checking the return value of gzclose()? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pg_basebackup (missing exit on error)
On Wed, Mar 28, 2012 at 9:40 PM, Robert Haas robertmh...@gmail.com wrote: I think it would make sense to rearrange that so that we don't have two tests for ztarfile != NULL; do that test first, and then if it fails, do the strcmp after that. Makes sense. Also, if we're going to test the return value of fclose(), shouldn't we also be checking the return value of gzclose()? Yes, we should. Attached patch does the above two changes. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center missing_exit_on_error_in_pgbasebackup_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] PATCH: pg_basebackup (missing exit on error)
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
Re: [HACKERS] PATCH: pg_basebackup (missing exit on error)
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? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] PATCH: pg_basebackup (missing exit on error)
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). Regards, Thomas diff -uNr postgresql-9.1.2/src/bin/pg_basebackup/pg_basebackup.c postgresql-9.1.2-patch/src/bin/pg_basebackup/pg_basebackup.c --- postgresql-9.1.2/src/bin/pg_basebackup/pg_basebackup.c 2011-12-01 22:47:20.0 +0100 +++ postgresql-9.1.2-patch/src/bin/pg_basebackup/pg_basebackup.c 2012-01-23 13:14:16.0 +0100 @@ -410,6 +410,7 @@ { fprintf(stderr, _(%s: could not write to compressed file \%s\: %s\n), progname, filename, get_gz_error(ztarfile)); + disconnect_and_exit(1); } } else @@ -428,7 +429,14 @@ #ifdef HAVE_LIBZ if (ztarfile) gzclose(ztarfile); +else #endif + if (fflush (tarfile) != 0) + { + fprintf(stderr, _(%s: error flushing stdout: %s\n), +strerror (errno)); + disconnect_and_exit(1); + } } else { @@ -437,7 +445,14 @@ gzclose(ztarfile); #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); + } +} } break; @@ -456,6 +471,7 @@ { 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