On Thu, 20 Feb 2025 at 14:48, jian he <
jian.universality@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.
