Re: pg_basebackup for streaming base backups - Mailing list pgsql-hackers

2011/1/18 Fujii Masao <masao.fujii@gmail.com>:
> On Mon, Jan 17, 2011 at 10:30 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
>> Though I haven't seen the core part of the patch (i.e.,
>> ReceiveTarFile, etc..) yet,
>> here is the comments against others.
>
> Here are another comments:
>
>
> When I untar the tar file taken by pg_basebackup, I got the following
> messages:
>
>    $ tar xf base.tar
>    tar: Skipping to next header
>    tar: Archive contains obsolescent base-64 headers
>    tar: Error exit delayed from previous errors
>
> Is this a bug? This happens only when I create $PGDATA by using
> initdb -X (i.e., I relocated the pg_xlog directory elsewhere than
> $PGDATA).
>
>
> +               if (compresslevel > 0)
> +               {
> +                       snprintf(fn, sizeof(fn), "%s/%s.tar.gz", tardir, PQgetvalue(res,
> rownum, 0));
> +                       ztarfile = gzopen(fn, "wb");
>
> Though I'm not familiar with zlib, isn't gzsetparams() required
> here?
>
>
> +#ifdef HAVE_LIBZ
> +       if (!tarfile && !ztarfile)
> +#else
> +       if (!tarfile)
> +#endif
> +       {
> +               fprintf(stderr, _("%s: could not create file \"%s\": %s\n"),
> +                               progname, fn, strerror(errno));
>
> Instead of strerror, get_gz_error seems required when using zlib.
>
>
> +       if (!res || PQresultStatus(res) != PGRES_COPY_OUT)
>
> The check for "!res" is not needed since PQresultStatus checks that.
>
>
> +               r = PQgetCopyData(conn, ©buf, 0);
> +               if (r == -1)
>
> Since -1 of PQgetCopyData might indicate an error, in this case,
> we would need to call PQgetResult?.
>
>
> ReceiveTarFile seems refactorable by using GZWRITE and GZCLOSE
> macros.
>
>
> +                                       fprintf(stderr, _("%s: could not write to file '%s': %m\n"),
>
> %m in fprintf is portable?
>
>
> Can't you change '%s' to \"%s\" for consistency?
>
>
> +       /*
> +        * Make sure we're unpacking into an empty directory
> +        */
> +       verify_dir_is_empty_or_create(current_path);
>
> Can pg_basebackup take a backup of $PGDATA including a tablespace
> directory, without an error? The above code seems to prevent that....
>
>
> +                               if (compresslevel <= 0)
> +                               {
> +                                       fprintf(stderr, _("%s: invalid compression level \"%s\"\n"),
>
> It's better to check "compresslevel > 9" here.
>
>
> +/*
> + * Print a progress report based on the global variables. If verbose output
> + * is disabled, also print the current file name.
>
> Typo: s/disabled/enabled
>
>
> I request new option which specifies whether pg_start_backup
> executes immediate checkpoint or not. Currently it always executes
> immediate one. But I'd like to run smoothed one in busy system.
> What's your opinion?
>

*if* it is possible, this is welcome, the checkpoint hit due to
pg_start_backup is visible, even outside pg_basebasckup.
(it sync everything then it blast cache memory)

--
Cédric Villemain               2ndQuadrant
http://2ndQuadrant.fr/     PostgreSQL : Expertise, Formation et Support