Thanks Alvaro and Jian for the review. > > otherwise if the cluster is not setting up. > ``pg_dumpall --format=d`` > error would be about connection error, not > "pg_dumpall: error: no output directory specified" > > we want ``pg_dumpall --format`` invalid options > to error out even if the cluster is not setting up.
Fixed. Apart from this, added handling to support empty directory also with --file option. > > you also need change > <varlistentry> > <term><option>-f <replaceable > class="parameter">filename</replaceable></option></term> > <term><option>--file=<replaceable > class="parameter">filename</replaceable></option></term> > <listitem> > <para> > Send output to the specified file. If this is omitted, the > standard output is used. > </para> > </listitem> > </varlistentry> > ? > > since if --format=d, > <option>--file=<replaceable class="parameter">filename</replaceable></option> > can not be omitted. No, we don't need this change. With --fromat=d, we can omit the --file option. > On Sat, 11 Jan 2025 at 14:14, Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote: > > Hmm, this patch adds a function connectDatabase() to pg_restore, but a > function that's almost identical already exists in pg_dumpall. I > suggest they should be unified. Maybe create a new file for connection > management routines? (since this clearly doesn't fit common.c nor > dumputils.c). I will make a new file in follow-up patches. > On Sat, 11 Jan 2025 at 21:38, Mahendra Singh Thalor <mahi6...@gmail.com> > wrote: > > > On Sat, 11 Jan 2025 at 9:30 PM, jian he <jian.universal...@gmail.com> wrote: >> >> hi. >> the following two tests, you can add to src/bin/pg_dump/t/001_basic.pl >> >> command_fails_like( >> [ 'pg_restore', '--globals-only', '-f', 'xxx' ], >> qr/\Qpg_restore: error: option -g\/--globals-only requires option >> -d\/--dbname\E/, >> 'pg_restore: error: option -g/--globals-only requires option -d/--dbname' I removed this error form code as we can dump global sql commands in file also. >> ); >> command_fails_like( >> [ 'pg_restore', '--globals-only', '--file=xxx', '--exclude-database=x',], >> qr/\Qpg_restore: error: option --exclude-database cannot be used >> together with -g\/--globals-only\E/, >> 'pg_restore: error: option --exclude-database cannot be used >> together with -g/--globals-only' >> ); Fixed. >> >> >> in pg_restore.sgml. >> <varlistentry> >> <term><option>--exclude-database=<replaceable >> class="parameter">pattern</replaceable></option></term> >> <listitem> >> the position should right after >> <varlistentry> >> <term><option>-d <replaceable >> class="parameter">dbname</replaceable></option></term> >> <term><option>--dbname=<replaceable >> class="parameter">dbname</replaceable></option></term> Fixed. >> >> >> should >> pg_restore --globals-only >> pg_restore --exclude-database=pattern >> be in a separate patch? I think we can keep these 2 options in one patch only as both are for pg_restore and there are not many code changes. If we want, we can make separate patches for pg_dumpall and pg_restore options. >> >> >> i am also wondering what will happen: >> pg_restore --exclude-database=pattern --dbname=pattern > For restore, we will make server connection with ‘pattern’ database and we will skip restoring for ‘pattern’ database as we are giving ‘pattern’ with --exclude-database. With server connection, we will restore global.dat at the start of pg_restore and for each database, we will fire the db creation command from 'pattern' db. Here, I am attaching an updated patch for review and testing. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
v08_pg_dumpall-with-directory-tar-custom-format-12-jan.patch
Description: Binary data
