Re: Refactor handling of database attributes betweenpg_dump and pg_dumpall - Mailing list pgsql-hackers

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.

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.

- 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

= 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.

- 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.

= Nitpicking

- 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 
</command>

- 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().

Andreas