|From:||Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com>|
|To:||Andreas Karlsson <andreas(at)proxel(dot)se>|
|Subject:||Re: Refactor handling of database attributes between pg_dump and pg_dumpall|
|Views:||Raw Message | Whole Thread | Download mbox|
On Tue, Mar 28, 2017 at 12:50 AM, Andreas Karlsson <andreas(at)proxel(dot)se>
> 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
> 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
> 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
> not ok 12 - no dump errors
> not ok 13 - restore full dump with command-line options for connection
> 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.
> - 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
> - The logic for switching database when setting the default table space is
> broken. You generate "\ connect" rather than "\connect".
> - 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.
> - You should probably use SGML style </> over </command> and
> </application> for inline tags.
> - In "database-level properties such as Ownership, ACLs, [...]" I do not
> think that "Ownerships" shuld be capitalized.
- 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
> - 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.
> - Rename arrayitem to configitem in makeAlterConfigCommand().
> - 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.
> - 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.
- There is something wrong with the indentation of the query for collecting
> info about databases in dumpDatabase() for PG >= 9.6.
- Missing space before "'' as rdatacl" in dumpDatabase(), and a missing
> space at the end of the string.
> - Double space in 'FROM pg_database "' in dumpDatabase().
Updated patch attached.
|Next Message||Kyotaro HORIGUCHI||2017-03-29 03:46:49||show "aggressive" or not in autovacuum logs|
|Previous Message||Michael Paquier||2017-03-29 03:42:17||Re: [PATCH] Reduce src/test/recovery verbosity|