From: | Cédric Villemain <cedric(dot)villemain(dot)debian(at)gmail(dot)com> |
---|---|
To: | Fujii Masao <masao(dot)fujii(at)gmail(dot)com> |
Cc: | Magnus Hagander <magnus(at)hagander(dot)net>, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: pg_basebackup for streaming base backups |
Date: | 2011-01-18 10:33:09 |
Message-ID: | AANLkTinu8bK51Uuz3N75PFn5m5dNTXQdgJLXmoa1YKpp@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
2011/1/18 Fujii Masao <masao(dot)fujii(at)gmail(dot)com>:
> On Mon, Jan 17, 2011 at 10:30 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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
From | Date | Subject | |
---|---|---|---|
Next Message | Dimitri Fontaine | 2011-01-18 10:49:03 | Re: REVIEW: Extensions support for pg_dump |
Previous Message | Cédric Villemain | 2011-01-18 10:26:51 | Re: Spread checkpoint sync |