On Thu, 20 Feb 2025 at 14:48, jian he <jian.universal...@gmail.com> wrote: > > hi. > about 0001 > > /* > * connectDatabase > * > * Make a database connection with the given parameters. An > * interactive password prompt is automatically issued if required. > * > * If fail_on_error is false, we return NULL without printing any message > * on failure, but preserve any prompted password for the next try. > * > * On success, the global variable 'connstr' is set to a connection string > * containing the options used. > */ > PGconn * > connectDatabase(const char *dbname, const char *connection_string, > const char *pghost, const char *pgport, const char *pguser, > trivalue prompt_password, bool fail_on_error, const > char *progname, > const char **connstr, int *server_version) > do the comments need to change? since no > global variable 'connstr' in common_dumpall_restore.c > maybe we need some words to explain server_version, (i don't have a > huge opinion though).
Fixed. > > > /*------------------------------------------------------------------------- > * > * common_dumpall_restore.c > * > * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group > * Portions Copyright (c) 1994, Regents of the University of California > * > * This is a common file for pg_dumpall and pg_restore. > * src/bin/pg_dump/common_dumpall_restore.c > * > *------------------------------------------------------------------------- > */ > > may change to > > /*------------------------------------------------------------------------- > * > * common_dumpall_restore.c > * This is a common file for pg_dumpall and pg_restore. > * > * Portions Copyright (c) 1996-2025, PostgreSQL Global Development Group > * Portions Copyright (c) 1994, Regents of the University of California > * > * IDENTIFICATION > * src/bin/pg_dump/common_dumpall_restore.c > * > *------------------------------------------------------------------------- > */ > so the style aligns with most other files. Fixed. > (we can apply the same logic to src/bin/pg_dump/common_dumpall_restore.h) We are already doing the same in the .h file. > > > in src/bin/pg_dump/pg_dumpall.c > #include "common_dumpall_restore.h" > imply include "pg_backup.h". > so in src/bin/pg_dump/pg_dumpall.c, we don't need include "pg_backup.h" Fixed. Also I removed some extra .h files from the patch. > > > attached are minor cosmetic changes for v19. - /* return number of errors */ > - if (AH->n_errors) > - n_errors = AH->n_errors; > - > /* AH may be freed in CloseArchive? */ > CloseArchive(AH); As per this comment, we can't return AH->n_errors as this might already be freed so we should copy before CloseArchive. Here, I am attaching updated patches for review and testing. -- Thanks and Regards Mahendra Singh Thalor EnterpriseDB: http://www.enterprisedb.com
v20_0001_move-common-code-of-pg_dumpall-and-pg_restore-to-new_file.patch
Description: Binary data
v20_0002_pg_dumpall-with-non-text_format-20th_feb.patch
Description: Binary data
