Re: Non-text mode for pg_dumpall

On Thu, 20 Feb 2025 at 14:48, jian he <jian(dot)universality(at)gmail(dot)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