Re: Non-text mode for pg_dumpall

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.

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com