Hi Jaime, thanks for your review!
On Sun, Feb 6, 2011 at 2:12 PM, Jaime Casanova <ja...@2ndquadrant.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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
