On Tue, 11 Feb 2025 at 20:40, jian he <jian.universal...@gmail.com> wrote: > > hi. > review based on v16. > > because of > https://postgr.es/m/cafc+b6pwqisl+3rvlxn9vhc8aonp4ov9c6u+bvd6kmwmdbd...@mail.gmail.com > > in copy_global_file_to_out_file, now it is: > if (strcmp(outfile, "-") == 0) > OPF = stdout; > I am confused, why "-" means stdout. > ``touch ./- `` command works fine. > i think dash is not special character, you may see > https://stackoverflow.com/a/40650391/15603477
"-" is used for stdout. This is mentioned in the doc. pg_restore link <https://www.postgresql.org/docs/current/app-pgrestore.html> > -f *filename* > --file=*filename* > > Specify output file for generated script, or for the listing when used > with -l. Use - for stdout. > > > > + /* Create a subdirectory with 'databases' name under main directory. */ > + if (mkdir(db_subdir, 0755) != 0) > + pg_log_error("could not create subdirectory \"%s\": %m", db_subdir); > here we should use pg_fatal? Yes, we should use pg_fatal. > > > pg_log_info("executing %s", sqlstatement.data); > change to > pg_log_info("executing query: %s", sqlstatement.data); > message would be more similar to the next pg_log_error(...) message. Okay. > > > + /* > + * User is suggested to use single database dump for --list option. > + */ > + if (opts->tocSummary) > + pg_fatal("option -l/--list cannot be used when using dump of pg_dumpall"); > maybe change to > + pg_fatal("option -l/--list cannot be used when restoring multiple databases"); okay. > > $BIN10/pg_restore --format=directory --list dir10_x > if the directory only has one database, then we can actually print out > the tocSummary. > if the directory has more than one database then pg_fatal. > To tolerate this corner case (only one database) means that pg_restore > --list requires a DB connection, > but I am not sure that is fine. > anyway, the attached patch allows this corner case. No, we don't need this corner case. If a user wants to restore a single database with --list option, then the user should give a particular dump file with pg_restore. > > > PrintTOCSummary can only print out summary for a single database. > so we don't need to change PrintTOCSummary. > > > + /* > + * To restore multiple databases, -C (create database) option should > be specified > + * or all databases should be created before pg_restore. > + */ > + if (opts->createDB != 1) > + pg_log_info("restoring dump of pg_dumpall without -C option, there > might be multiple databases in directory."); > > we can change it to > + if (opts->createDB != 1 && num_db_restore > 0) > + pg_log_info("restoring multiple databases without -C option."); okay. > > > Bug. > when pg_restore --globals-only can be applied when we are restoring a > single database (can be an output of pg_dump). As of now, we are ignoring this option. We can add an error in the "else" part of the global.dat file. Ex: option --globals-only is only supported with dump of pg_dumpall. Similarly --exclude-database also. > > > There are some tests per https://commitfest.postgresql.org/52/5495, I > will check it later. > The attached patch is the change for the above reviews. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
