Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Wed, Nov 8, 2017 at 8:48 AM, Robert Haas wrote: > On Tue, Nov 7, 2017 at 4:35 AM, Haribabu Kommi > wrote: > > The newly added option is not recommended to be used in normal cases and > > it is used only for upgrade utilities. > > I don't know why it couldn't be used in normal cases. That seems like > a totally legitimate thing for somebody to want. Maybe nobody does, > but I see no reason to worry if they do. Ok. Removed the documentation changes that it cannot be used for normal scenarios, and also added a Note section explaining the problem of using the dump with pg_restore command with --clean and --create options. Regards, Hari Babu Fujitsu Australia pg_dump-and-pg_dumpall-database-handling-refactoring_v10.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Tue, Nov 7, 2017 at 4:35 AM, Haribabu Kommi wrote: > The newly added option is not recommended to be used in normal cases and > it is used only for upgrade utilities. I don't know why it couldn't be used in normal cases. That seems like a totally legitimate thing for somebody to want. Maybe nobody does, but I see no reason to worry if they do. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Thu, Oct 26, 2017 at 10:01 PM, Robert Haas wrote: > On Mon, Oct 23, 2017 at 7:36 AM, Haribabu Kommi > wrote: > > Apologies for not providing much details. > > > > pg_dumpall is used to produce the following statements for database, > > > > "Create database" (other than default database) or > > "Alter database set tablespace" for default database (if required) > > > > ACL queries related to database > > Alter database config > > Alter database role config > > > > whereas, pg_dump used to produce only "create database statement". > > How about adding a new flag --set-db-properties that doesn't produce > CREATE DATABASE but does dump the other stuff? -C would dump both > CREATE DATABASE *and* the other stuff. Then you could dump built-in > databases with --set-db-properties and others with -C. Thanks for the idea, Here I attached the patch that implements the same. The newly added option is not recommended to be used in normal cases and it is used only for upgrade utilities. In case if user issues pg_dump with --set-db-properties option along with --create or --clean options, an error is raised. Currently there is no way to throw an error in case if the dump is generated with --set-db-properties and try to restore with --clean option. To avoid this change, we may need to add additional details in the archive handler, but is it really needed? Regards, Hari Babu Fujitsu Australia pg_dump-and-pg_dumpall-database-handling-refactoring_v9.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Mon, Oct 23, 2017 at 7:36 AM, Haribabu Kommi wrote: > Apologies for not providing much details. > > pg_dumpall is used to produce the following statements for database, > > "Create database" (other than default database) or > "Alter database set tablespace" for default database (if required) > > ACL queries related to database > Alter database config > Alter database role config > > whereas, pg_dump used to produce only "create database statement". How about adding a new flag --set-db-properties that doesn't produce CREATE DATABASE but does dump the other stuff? -C would dump both CREATE DATABASE *and* the other stuff. Then you could dump built-in databases with --set-db-properties and others with -C. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Sun, Oct 22, 2017 at 3:08 AM, Robert Haas wrote: > On Sat, Oct 21, 2017 at 1:30 AM, Haribabu Kommi > wrote: > > Before refactoring, pg_dumpall doesn't print "create database" commands > > for both tempalte1 and postgres database, but on the other hand pg_dump > > dump the create database commands with --create option. > > > > To keep the behavior of all the database attributes in the dump of both > > pg_dump and pg_dumpall, the code is unified and moved into pg_dump. > > But to retain the pg_dumpall behavior of not dumping the "create > database" > > commands, a new option is added to pg_dump to skip dumping the > > create database commands. > > > > The new option name is now "--skip-create-default-db", this can be used > > normal user also when try to dump the postgres database to not let create > > the database commands in the dump. > > I don't get this at all. If you don't want to create the database, > just don't pass the -C argument. It doesn't make sense to have a -C > argument which makes it create the database and then a > --skip-create-default-db argument which makes it sometimes not create > the database after all. Apologies for not providing much details. pg_dumpall is used to produce the following statements for database, "Create database" (other than default database) or "Alter database set tablespace" for default database (if required) ACL queries related to database Alter database config Alter database role config whereas, pg_dump used to produce only "create database statement". With the refactoring, all the pg_dumpall database statements are moved into pg_dump. -C/--create option of pg_dump produces all the statements of pg_dumpall. The --skip-default-create-db option is to make sure that it doesn't produce "Create database" statement and instead may produce "Alter database set tablespace" for default databases of (postgres and template1). -C/--create option is to control the entire database statements. --skip-create-default-db is option to control the "create" or "Alter" database statement for default database. During restore the dump, the -C/--create restores all the Database statements. comments? or any better approach? Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Sat, Oct 21, 2017 at 1:30 AM, Haribabu Kommi wrote: > Before refactoring, pg_dumpall doesn't print "create database" commands > for both tempalte1 and postgres database, but on the other hand pg_dump > dump the create database commands with --create option. > > To keep the behavior of all the database attributes in the dump of both > pg_dump and pg_dumpall, the code is unified and moved into pg_dump. > But to retain the pg_dumpall behavior of not dumping the "create database" > commands, a new option is added to pg_dump to skip dumping the > create database commands. > > The new option name is now "--skip-create-default-db", this can be used > normal user also when try to dump the postgres database to not let create > the database commands in the dump. I don't get this at all. If you don't want to create the database, just don't pass the -C argument. It doesn't make sense to have a -C argument which makes it create the database and then a --skip-create-default-db argument which makes it sometimes not create the database after all. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Fri, Oct 6, 2017 at 12:29 AM, Robert Haas wrote: > On Wed, Oct 4, 2017 at 3:40 AM, Haribabu Kommi > wrote: > > There are some differences in handling database objects > > between pg_dump and pg_dumpall, To retain both pg_dump > > and pg_dumpall behavior even after refactoring, this option > > is added. Currently this option is used mainly for the three > > purposes. > > > > 1. Don't print unnecessary CREATE DATABASE options like > > ENCODING, LC_COLLATE and LC_CTYPE options if the > > default encoding is same with the above values. > > > > The above behavior is as per the pg_dumpall, but it can be > > changed to print irrespective of the default encoding. > > > > 2. Do not dump postgres and template0 databases. > > > > 3. Set default_transaction_read_only = off. > > To me it seems that a refactoring which requires pg_dump to behave > differently in small ways like this based on whether it is being > called by pg_dumpall or not is probably not a good refactoring. And I > don't see why the proposal from Tom that started this thread would > require such a thing to be true. > Before refactoring, pg_dumpall doesn't print "create database" commands for both tempalte1 and postgres database, but on the other hand pg_dump dump the create database commands with --create option. To keep the behavior of all the database attributes in the dump of both pg_dump and pg_dumpall, the code is unified and moved into pg_dump. But to retain the pg_dumpall behavior of not dumping the "create database" commands, a new option is added to pg_dump to skip dumping the create database commands. The new option name is now "--skip-create-default-db", this can be used normal user also when try to dump the postgres database to not let create the database commands in the dump. >From your list, I would say that (1) and (3) seem like behaviors that > we either want or do not want. Whether pg_dump or pg_dumpall is > involved seems irrelevant. (2) seems like it might need some special > handling, but that could be handled in pg_dumpall by just not calling > pg_dump at all for those database. I didn't any better way other than creating a new option to not let the default db create database commands to dump, so I renamed the older option to better one and change the behavior to use by the normal users also. Updated patch attached. Regards, Hari Babu Fujitsu Australia pg_dump-and-pg_dumpall-database-handling-refactoring_v8.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Wed, Oct 4, 2017 at 3:40 AM, Haribabu Kommi wrote: > There are some differences in handling database objects > between pg_dump and pg_dumpall, To retain both pg_dump > and pg_dumpall behavior even after refactoring, this option > is added. Currently this option is used mainly for the three > purposes. > > 1. Don't print unnecessary CREATE DATABASE options like > ENCODING, LC_COLLATE and LC_CTYPE options if the > default encoding is same with the above values. > > The above behavior is as per the pg_dumpall, but it can be > changed to print irrespective of the default encoding. > > 2. Do not dump postgres and template0 databases. > > 3. Set default_transaction_read_only = off. To me it seems that a refactoring which requires pg_dump to behave differently in small ways like this based on whether it is being called by pg_dumpall or not is probably not a good refactoring. And I don't see why the proposal from Tom that started this thread would require such a thing to be true. >From your list, I would say that (1) and (3) seem like behaviors that we either want or do not want. Whether pg_dump or pg_dumpall is involved seems irrelevant. (2) seems like it might need some special handling, but that could be handled in pg_dumpall by just not calling pg_dump at all for those database. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Sat, Sep 30, 2017 at 3:31 AM, Robert Haas wrote: > On Fri, Sep 29, 2017 at 12:44 AM, Vaishnavi Prabakaran > wrote: > > Option name "--enable-pgdumpall-behaviour" is very generic > > Yeah, that's a terrible name, at least in my opinion. > OK. I will use a new name based on the discussion. > and it is better > > to rename it to something that reflects its functionality like > > --skip-default-db-create/--no-default-db-create > > But I wonder why this patch needs a new option at all? > There are some differences in handling database objects between pg_dump and pg_dumpall, To retain both pg_dump and pg_dumpall behavior even after refactoring, this option is added. Currently this option is used mainly for the three purposes. 1. Don't print unnecessary CREATE DATABASE options like ENCODING, LC_COLLATE and LC_CTYPE options if the default encoding is same with the above values. The above behavior is as per the pg_dumpall, but it can be changed to print irrespective of the default encoding. 2. Do not dump postgres and template0 databases. 3. Set default_transaction_read_only = off. As per the following comment in pg_dumpall, based on that flag the GUC is set, to retain the same behavior even after this refactoring. /* * Restore will need to write to the target cluster. This connection * setting is emitted for pg_dumpall rather than in the code also used * by pg_dump, so that a cluster with databases or users which have * this flag turned on can still be replicated through pg_dumpall * without editing the file or stream. With pg_dump there are many * other ways to allow the file to be used, and leaving it out allows * users to protect databases from being accidental restore targets. */ fprintf(OPF, "SET default_transaction_read_only = off;\n\n"); we can remove the usage -1 and retain the usage-2 with modified option name as --no-default-database or similar. Any opinions about the usage-3, In case if we need to retain that change, any best solution to the option name? Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Fri, Sep 29, 2017 at 12:44 AM, Vaishnavi Prabakaran wrote: > Option name "--enable-pgdumpall-behaviour" is very generic Yeah, that's a terrible name, at least in my opinion. > and it is better > to rename it to something that reflects its functionality like > --skip-default-db-create/--no-default-db-create But I wonder why this patch needs a new option at all? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
Hi, On Sat, Sep 9, 2017 at 1:29 PM, Haribabu Kommi wrote: > > Fixed patch is attached. > Patch applies and has lot of noise due to indent with spaces. I did ran regression tests located in - src/test/regress, src/test/modules/test_pg_dump, src/bin/pg_dump, src/bin/pg_upgrade folders and no issues observed. + --enable-pgdumpall-behaviour + + +This option is for the use of pg_dumpall or +pg_upgrade utility to dump the database objects +by pg_dump for a complete dump. +This option can only be used with -C/--create. +Its use for other ourposes is not recommended or supported. +The behavior of the option may change in future releases without notice. + + + + + s/ourposes/purposes/ Option name "--enable-pgdumpall-behaviour" is very generic and it is better to rename it to something that reflects its functionality like --skip-default-db-create/--no-default-db-create Thanks & Regards, Vaishnavi, Fujitsu Australia.
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Fri, Sep 8, 2017 at 10:24 AM, Thomas Munro wrote: > On Mon, Aug 21, 2017 at 4:35 PM, Haribabu Kommi > wrote: > > On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut > > wrote: > >> On 4/4/17 01:06, Haribabu Kommi wrote: > >> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached > >> > I will add this patch to the next commitfest. > >> > >> This patch needs to be rebased for the upcoming commit fest. > > > > Thanks for checking. Rebased patch is attached. > > Hi Haribabu, > > This patch breaks the documentation build, possibly because of these empty > tags: > > +--create option to dump <>ALTER ROLE IN > DATABASE ... SET > Thanks for checking the patch. Fixed patch is attached. Regards, Hari Babu Fujitsu Australia 0001-pg_dump-and-pg_dumpall-database-handling-refactoring_v7.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Mon, Aug 21, 2017 at 4:35 PM, Haribabu Kommi wrote: > On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut > wrote: >> On 4/4/17 01:06, Haribabu Kommi wrote: >> > Both pg_dump and pg_upgrade tests are passed. Updated patch attached >> > I will add this patch to the next commitfest. >> >> This patch needs to be rebased for the upcoming commit fest. > > Thanks for checking. Rebased patch is attached. Hi Haribabu, This patch breaks the documentation build, possibly because of these empty tags: +--create option to dump <>ALTER ROLE IN DATABASE ... SET -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Tue, Aug 15, 2017 at 7:29 AM, Peter Eisentraut < peter.eisentr...@2ndquadrant.com> wrote: > On 4/4/17 01:06, Haribabu Kommi wrote: > > Both pg_dump and pg_upgrade tests are passed. Updated patch attached > > I will add this patch to the next commitfest. > > This patch needs to be rebased for the upcoming commit fest. > Thanks for checking. Rebased patch is attached. Regards, Hari Babu Fujitsu Australia 0001-pg_dump-and-pg_dumpall-database-handling-refactoring_v6.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On 4/4/17 01:06, Haribabu Kommi wrote: > Both pg_dump and pg_upgrade tests are passed. Updated patch attached > I will add this patch to the next commitfest. This patch needs to be rebased for the upcoming commit fest. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Thu, Mar 30, 2017 at 12:00 PM, Haribabu Kommi wrote: > > > On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson > wrote: > >> On 03/29/2017 05:43 AM, Haribabu Kommi wrote: >> > Updated patch attached. >> >> I get a test failure in the pg_upgrade tests, but I do not have time >> right now to investigate. >> >> The failing test is "Restoring database schemas in the new cluster". >> > > Thanks for test. > > I found the reason for failure. > > Before this refactor patch, in case of --binary-upgrade, the pg_dumpall > dumps all the global objects and also the database objects. These objects > will be restored first during the preparation of the new cluster and later > each individual database is restored. > > Because of the refactoring of the database objects, currently as part of > globals dump with --binary-upgrade, no database objects gets dumped. > During restore no databases are created. so while restoring individual > database, it leads to failure as it not able to connect to the target > database. > I modified the pg_upgrade code to use template1 database as a connecting database while restoring the dump along with --create option to pg_restore to create the database objects instead of connecting to the each individual database. And also while dumping the database objects, passed the new option of --enable-pgdumpall-behaviour to pg_dump to dump the database objects as it expected dump during pg_dumpall --binary-upgrade. Both pg_dump and pg_upgrade tests are passed. Updated patch attached I will add this patch to the next commitfest. Regards, Hari Babu Fujitsu Australia pg_dump_changes_5.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Wed, Mar 29, 2017 at 11:04 PM, Andreas Karlsson wrote: > On 03/29/2017 05:43 AM, Haribabu Kommi wrote: > > Updated patch attached. > > I get a test failure in the pg_upgrade tests, but I do not have time right > now to investigate. > > The failing test is "Restoring database schemas in the new cluster". > Thanks for test. I found the reason for failure. Before this refactor patch, in case of --binary-upgrade, the pg_dumpall dumps all the global objects and also the database objects. These objects will be restored first during the preparation of the new cluster and later each individual database is restored. Because of the refactoring of the database objects, currently as part of globals dump with --binary-upgrade, no database objects gets dumped. During restore no databases are created. so while restoring individual database, it leads to failure as it not able to connect to the target database. Currently I marked the patch in the commitfest as "returned with feedback" as in the current situation, this needs some analysis in handling database objects in --binary-upgrade mode. Regards, Hari Babu Fujitsu Australia
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On 03/29/2017 05:43 AM, Haribabu Kommi wrote: > Updated patch attached. I get a test failure in the pg_upgrade tests, but I do not have time right now to investigate. The failing test is "Restoring database schemas in the new cluster". I get the following in the log: command: "/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_dump" --host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 --username andreas --schema-only --quote-all-identifiers --binary-upgrade --format=custom --file="pg_upgrade_dump_16385.custom" 'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' >> "pg_upgrade_dump_16385.log" 2>&1 command: "/home/andreas/dev/postgresql/src/bin/pg_upgrade/tmp_check/install//home/andreas/dev/postgresql-inst/bin/pg_restore" --host /home/andreas/dev/postgresql/src/bin/pg_upgrade --port 50848 --username andreas --exit-on-error --verbose --dbname 'dbname='"'"'./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ'"'"'' "pg_upgrade_dump_16385.custom" >> "pg_upgrade_dump_16385.log" 2>&1 pg_restore: connecting to database for restore pg_restore: [archiver (db)] connection to database "./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" failed: FATAL: database "./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ" does not exist Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Tue, Mar 28, 2017 at 12:50 AM, Andreas Karlsson 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 and > 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. > > ALTER ROLE IN DATABASE ... SET commands. > > with --create option to dump ALTER ROLE IN DATABASE ... SET > > 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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
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 and 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. ALTER ROLE IN DATABASE ... SET commands. with --create option to dump 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(). Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On 03/21/2017 08:02 AM, Haribabu Kommi wrote: Solution -1) Just ignore dumping these CREATE DATABASE commands and provide the user information in the documentation to create "postgres" and "template1" database in the target in case if they don't exist. If this kind of cases are very rare. Solution-2) Add a new command line option/some other settings to indicate the pg_dump execution is from pg_dumpall and follow the current refactored behavior, otherwise follow the earlier pg_dump behavior in handling CREATE DATABASE commands for "postgres" and "template1" databases. I am leaning towards (2) since I feel having pg_dump act differently depending on the name of the database is a quite surprising behavior. It makes more sense to let a tool like pg_dumpall handle logic like that. 2. In dumpDatabases function before calling the runPgDump command, Before refactoring, it used to connect to the database and dump "SET default_transaction_read_only = off;" to prevent some accidental overwrite of the target. I fixed it in the attached patch by removing the connection and dumping the set command. Does it needs the similar approach of solution-2) in previous problem and handle dumping the "SET default_transaction_read_only = off;" whenever the CREATE DATABASE and \connect command is issued? Hm, that is a bit annoying. I do not think we want to change any behavior here, either of pg_dump or pg_dumpall, but I also do not like having to add two new flags to pg_dump (one for including the ALTER DATABASE commands but not CREATE DATABASE, and another flag for default_transaction_read_only) or a special flag similar to --binary-upgrade. None of these options seem optimal to me, and I do not have any strong preference other than that we should avoid breaking pg_dump or changing behavior not related to the database attributes. Andreas -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
Because of this refactor handing of database objects between pg_dump and pg_dumpall, the latest pg_dump tap tests are failing in the following scenarios. 1. CREATE DATABASE postgres Before this patch, the pg_dump uses to dump the CREATE DATABASE command of postgres but not by pg_dumpall. During this refactor handling, the approach that I took in pg_dump for the --create option to use the similar appraoch of pg_dumpall to not to print the CREATE DATABASE commands for "postgres" and "template1" databases. It just prints the ALTER DATABASE commands to SET the TABLESPACE for those two databases. Solution -1) Just ignore dumping these CREATE DATABASE commands and provide the user information in the documentation to create "postgres" and "template1" database in the target in case if they don't exist. If this kind of cases are very rare. Solution-2) Add a new command line option/some other settings to indicate the pg_dump execution is from pg_dumpall and follow the current refactored behavior, otherwise follow the earlier pg_dump behavior in handling CREATE DATABASE commands for "postgres" and "template1" databases. 2. In dumpDatabases function before calling the runPgDump command, Before refactoring, it used to connect to the database and dump "SET default_transaction_read_only = off;" to prevent some accidental overwrite of the target. I fixed it in the attached patch by removing the connection and dumping the set command. Does it needs the similar approach of solution-2) in previous problem and handle dumping the "SET default_transaction_read_only = off;" whenever the CREATE DATABASE and \connect command is issued? Documentation is yet to update to reflect the above changes. Regards, Hari Babu Fujitsu Australia pg_dump_changes_3.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
Re: [HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
On Wed, Mar 1, 2017 at 12:59 PM, Haribabu Kommi wrote: > > Patch attached. Still some more docs needs to be added. > Updated patch attached to resolve the conflicts with following commit. commit 9a83d56b38c870ce47b7651385ff2add583bf136 Author: Simon Riggs Date: Tue Mar 7 22:00:54 2017 +0800 Allow pg_dumpall to dump roles w/o user passwords Add new option --no-role-passwords which dumps roles without passwords. Since we don’t need passwords, we choose to use pg_roles in preference to pg_authid since access may be restricted for security reasons in some configrations. Robins Tharakan and Simon Riggs Regards, Hari Babu Fujitsu Australia pg_dump_changes_2.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
[HACKERS] Refactor handling of database attributes between pg_dump and pg_dumpall
Subject changed for better context of the patch. (was - Re: Question about grant create on database and pg_dump/pg_dumpall) On Fri, Sep 30, 2016 at 12:29 AM, Tom Lane wrote: > >1. pg_dump without --create continues to do what it does today, ie it just >dumps objects within the database, assuming that database-level properties >will already be set correctly for the target database. > >2. pg_dump with --create creates the target database and also sets all >database-level properties (ownership, ACLs, ALTER DATABASE SET, etc etc). > >3. pg_dumpall loses all code relating to individual-database creation >and property setting and instead relies on pg_dump --create to do that. >This would leave only the code relating to "pg_dumpall -g" (ie, dump roles >and tablespaces) within pg_dumpall itself. I removed all the database related code from pg_dumpall and moved the necessary part of the code into pg_dump and called pg_dump with --create option from pg_dumpall to ensure that all the database create commands are getting dumped. Except postgres, template1 databases for rest of the databases the CREATE DATABASE command is issued. And all other properties dump is same for every database. >One thing that would still be messy is that presumably "pg_dumpall -g" >would issue ALTER ROLE SET commands, but it's unclear what to do with >ALTER ROLE IN DATABASE SET commands. Should those become part of >"pg_dump --create"'s charter? It seems like not, but I'm not certain. Yes, I moved the ALTER ROLE IN DATABASE SET commands also as part of pg_dump --create command, this way it will be easier to dump all the database objects using (pg_dumpall -g and pg_dump -C ). >Another thing that requires some thought is that pg_dumpall is currently >willing to dump ACLs and other properties for template1/template0, though >it does not invoke pg_dump on them. If we wanted to preserve that >behavior while still moving the code that does those things to pg_dump, >pg_dump would have to grow an option that would let it do that. But >I'm not sure how much of that behavior is actually sensible. Currently the ACLs and other changes related to template database are getting dumped with --create option in pg_dump. do we still need another option? >This would probably take a pg_dump archive version bump, since I think >we don't currently record enough information for --create to do this >(and we can't just cram the extra commands into the DATABASE entry, >since we don't know whether --create will be specified to pg_restore). >But we've done those before. There is no specific code is required related to the archive version check. Still do we need to bump the archive version? As it just adds some new commands as part of --create with pg_dump. Patch attached. Still some more docs needs to be added. comments? [1] - https://www.postgresql.org/message-id/21573.1475162...@sss.pgh.pa.us Regards, Hari Babu Fujitsu Australia pg_dump_changes_1.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
