Re: pg_dump directory archive format / parallel pg_dump

Hi Jaime,

thanks for your review!

On Sun, Feb 6, 2011 at 2:12 PM, Jaime Casanova <jaime(at)2ndquadrant(dot)com> wrote:
> code review:
>
> something i found, and is a very simple one, is this warning (there's
> a similar issue in _StartMasterParallel with the buf variable)
> """
> pg_backup_directory.c: In function ‘_EndMasterParallel’:
> pg_backup_directory.c:856: warning: ‘status’ may be used uninitialized
> in this function
> """

Cool. My compiler didn't tell me about this.

> i guess the huge amount of info is showing the patch is just for
> debugging and will be removed before commit, right?

That's right.

> functional review:
>
> it works good most of the time, just a few points:
> - if i interrupt the process the connections stay, i guess it could
> catch the signal and finish the connections

Hm, well, recovering gracefully out of errors could be improved. In
your example you would signal the children implicitly because the
parent process dies and the pipes to the children would get broken as
well. Of course the parent could more actively terminate the children
but it might not be the best option to just kill them, as then there
will be a lot of "unexpected EOF" connections in the log. So if an
error condition comes up in the parent (as in your example, because
you canceled the process), then ideally the parent should signal the
children with a non-lethal signal and the children should catch this
"please terminate" signal and exit cleanly but as soon as possible. If
the error case comes up at the child however, then we'd need to make
sure that the user sees the error message from the child. This should
work well as-is but currently it could happen that the parent exists
before all of the children have exited. I'll investigate this a bit...

> - if i have an exclusive lock on a table and a worker starts dumping
> it, it fails because it can't take the lock but it just say "it was
> ok" and would prefer an error

I'm getting a clear

pg_dump: [Archivierer] could not lock table public.c: ERROR: could
not obtain lock on relation "c"

but I'll look into this as well.

Regarding your other post:

> - there is no docs

True...

> - pg_dump and pg_restore are inconsistent:
> pg_dump requires the directory to be provided with the -f option:
> pg_dump -Fd -f dir_dump
> pg_restore pass the directory as an argument for -Fd: pg_restore -Fd dir_dump

Well, this is there with pg_dump and pg_restore currently as well. -F
is the switch for the format and it just takes "d" as the format. The
dir_dump is an option without any switch.

See the output for the --help switches:

Usage:
pg_dump [OPTION]... [DBNAME]

Usage:
pg_restore [OPTION]... [FILE]

So in either case you don't need to give a switch for what you have.
If you run pg_dump you don't give the switch for the database but you
need to give it for the output (-f) and with pg_restore you don't give
a switch for the file that you're restoring but you'd need to give -d
for restoring to a database.

Joachim