Re: pg_basebackup for streaming base backups

On Mon, Jan 17, 2011 at 7:14 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> Oh, sorry about that. There is only one that contains postgresql though :P
>>
>> http://github.com/mhagander/postgres, branch streaming_base.
>
> Thanks!

Though I haven't seen the core part of the patch (i.e.,
ReceiveTarFile, etc..) yet,
here is the comments against others.

+ if (strcmp(argv[1], "-h") == 0 || strcmp(argv[1], "--help") == 0 ||
+ strcmp(argv[1], "-?") == 0)

strcmp(argv[1], "-h") should be removed

+ printf(_(" -p, --progress show progress information\n"));

-p needs to be changed to -P

+ printf(_(" -D, --pgdata=directory receive base backup into directory\n"));
+ printf(_(" -T, --tardir=directory receive base backup into tar files\n"
+ " stored in specified directory\n"));
+ printf(_(" -Z, --compress=0-9 compress tar output\n"));
+ printf(_(" -l, --label=label set backup label\n"));
+ printf(_(" -p, --progress show progress information\n"));
+ printf(_(" -v, --verbose output verbose messages\n"));

Can we list those options in alphabetical order as other tools do?

Like -f and -F option of pg_dump, it's more intuitive to provide one option for
output directory and that for format. Something like

-d directory
--dir=directory

-F format
--format=format

p
plain

t
tar

+ case 'p':
+ if (atoi(optarg) == 0)
+ {
+ fprintf(stderr, _("%s: invalid port number \"%s\""),
+ progname, optarg);
+ exit(1);
+ }

Shouldn't we emit an error message when the result of atoi is *less than* or
equal to 0? \n should be in the tail of the error message. Is this error check
really required here? IIRC libpq does. If it's required, atoi for compresslevel
should also be error-checked.

+ case 'v':
+ verbose++;

Why is the verbose defined as integer?

+ if (optind < argc)
+ {
+ fprintf(stderr,
+ _("%s: too many command-line arguments (first is \"%s\")\n"),
+ progname, argv[optind + 1]);

You need to reference to argv[optind] instead.

What about using PGDATA environment variable when no target directory is
specified?

+ * Verify that the given directory exists and is empty. If it does not
+ * exist, it is created. If it exists but is not empty, an error will
+ * be give and the process ended.
+ */
+static void
+verify_dir_is_empty_or_create(char *dirname)

Since verify_dir_is_empty_or_create can be called after the connection has
been established, it should call PQfinish before exit(1).

+ keywords[2] = "fallback_application_name";
+ values[2] = "pg_basebackup";

Using the progname variable seems better rather than the fixed word
"pg_basebackup".

+ if (dbgetpassword == 1)
+ {
+ /* Prompt for a password */
+ password = simple_prompt(_("Password: "), 100, false);

PQfinish should be called for the password retry case.

+ if (PQstatus(conn) != CONNECTION_OK)
+ {
+ fprintf(stderr, _("%s: could not connect to server: %s\n"),
+ progname, PQerrorMessage(conn));
+ exit(1);
+ }

PQfinish seems required before exit(1).

+ if (PQsendQuery(conn, current_path) == 0)
+ {
+ fprintf(stderr, _("%s: coult not start base backup: %s\n"),

Typo: s/coult/could

+ /*
+ * Get the header
+ */
+ res = PQgetResult(conn);

After this, PQclear seems required before each exit(1) call.

+ if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
+ {
+ fprintf(stderr, _("%s: final receive failed: %s\n"),
+ progname, PQerrorMessage(conn));
+ exit(1);
+ }

PQfinish seems required before exit(1).

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center