Horton

Thread

  1. PATCH: pg_basebackup (missing exit on error)

    Thomas Ogrisegg <tom-nic@patches.fnord.at> — 2012-01-24T09:39:19Z

    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

  2. Re: PATCH: pg_basebackup (missing exit on error)

    Robert Haas <robertmhaas@gmail.com> — 2012-01-26T14:48:08Z

    On Tue, Jan 24, 2012 at 4:39 AM, Thomas Ogrisegg
<tom-nic@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

  3. Re: PATCH: pg_basebackup (missing exit on error)

    Fujii Masao <masao.fujii@gmail.com> — 2012-03-27T11:13:34Z

    On Thu, Jan 26, 2012 at 11:48 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Tue, Jan 24, 2012 at 4:39 AM, Thomas Ogrisegg
> <tom-nic@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

  4. Re: PATCH: pg_basebackup (missing exit on error)

    Robert Haas <robertmhaas@gmail.com> — 2012-03-28T12:40:15Z

    On Tue, Mar 27, 2012 at 7:13 AM, Fujii Masao <masao.fujii@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

  5. Re: PATCH: pg_basebackup (missing exit on error)

    Fujii Masao <masao.fujii@gmail.com> — 2012-03-28T15:12:26Z

    On Wed, Mar 28, 2012 at 9:40 PM, Robert Haas <robertmhaas@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