On Tue, Mar 28, 2017 at 12:50 AM, Andreas Karlsson <andr...@proxel.se> wrote:
> Hi, > > Here is my review. I agree with the goal of the refactoring, as we want to > make it easier to dump all the properties for the database object. But I > think we need to solve the issues with the special casing of postgres and > template1 which I personally would find very surprising if pg_dump -C did. > On the other hand I think that we cannot get away from having pg_dumpall > give them a special treatment. > Thanks for the review. I added a new option --enable-pgdumpall-behaviour to get the pg_dumpall behaviour for the database objects while dumping them through pg_dump. I am open to change the option name if we come up with any other better name. > The nitpicking section is for minor code style errors. > > = Functional review > > I have not done an in depth functional review due to the discussion about > how postgres and template1 should be handled. > > - The patch does not apply cleanly anymore > > - I do not like the change in behavior which causes "pg_dump -C postgres" > to no longer include CREATE DATABASE. Special treatment of specific > databases based on name makes sense in pg_dumpall, but not in pg_dump. > With the new additional option, CREATE DATABASE commands for postgres and special treatment of "SET default_transaction_read_only = off" still held. - There are test failures in the pg_dump tests. It seems like some could be > related to that you do not include CREATE DATABASE postgres in the dumps > but I also get errors like 'ERROR: syntax error at or near > "fault_tablespace"'. > > not ok 691 - createdb: dumps CREATE DATABASE postgres > not ok 3003 - pg_dumpall_dbprivs: dumps CREATE DATABASE dump_test > not ok 11 - restore full dump using environment variables for connection > parameters > not ok 12 - no dump errors > not ok 13 - restore full dump with command-line options for connection > parameters > not ok 14 - no dump errors > Fixed. Now all tests pass. = Code review > > - As a response to "TBD -- is it necessary to get the default encoding": I > think so, but either way changing this seems unrelated to this patch. > Removed. > - I know it is taken from the old pg_dumpall code, but the way the > database owner is handled seems I wrong.think we should set it like the > owner for other objects. And more importantly it should respect --no-owner. > Removed the code for owner, as it is handled in another place with ALTER DATABASE command. > - The logic for switching database when setting the default table space is > broken. You generate "\ connect" rather than "\connect". > Fixed. > - I saw the comment "Note that we do not support initial privileges > (pg_init_privs) on databases." and wondered: why not? I definitly think > that we should support this. > This is the existing code that moved from pg_dumpall. = Nitpicking > > - You should probably use SGML style </> over </command> and > </application> for inline tags. > Corrected. > - In "database-level properties such as Ownership, ACLs, [...]" I do not > think that "Ownerships" shuld be capitalized. > Fixed. - There are two extra spaces on the lines below, and a space is missing > after the closing tag. > > <command> ALTER ROLE IN DATABASE ... SET </command>commands. > > with --create option to dump <command> ALTER ROLE IN DATABASE ... SET > </command> > Fixed. > - On the following comment ".." should be "...", since that is the correct > way to write an ellipsis. > > * Frame the ALTER .. SET .. commands and fill it in buf. > Fixed. > - Rename arrayitem to configitem in makeAlterConfigCommand(). > Corrected. > - In makeAlterConfigCommand() you should do "*pos++ = '\0';" rather than > "*pos = 0;" and then remove the later + 1 so our code matches with the code > in dumpFunc(). Either is correct, but it would be nice if both pieces of > code looked more similar. > Corrected. > - You removed an empty line in pg_backup_utils.h between globals variables > and function declartions which I think should be left there. It should be > directly after g_verbose. Fixed. - There is something wrong with the indentation of the query for collecting > info about databases in dumpDatabase() for PG >= 9.6. > Fixed. - Missing space before "'' as rdatacl" in dumpDatabase(), and a missing > space at the end of the string. > Fixed. > - Double space in 'FROM pg_database "' in dumpDatabase(). Fixed. Updated patch attached. Regards, Hari Babu Fujitsu Australia
pg_dump_changes_4.patch
Description: Binary data
-- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
