Subject: Re: Non-text mode for pg_dumpall

From: jian he <[email-masked]>
Date: Thu, 20 Feb 2025 17:18:13 +0800

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).

/*-------------------------------------------------------------------------
 *
 * 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.
(we can apply the same logic to src/bin/pg_dump/common_dumpall_restore.h)

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"

attached are minor cosmetic changes for v19.

---

From: Mahendra Singh Thalor <[email-masked]>
Date: Thu, 20 Feb 2025 19:19:27 +0530

> maybe we need some words to explain server_version, (i don't have a

Fixed.

> 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.

> 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

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 4 Mar 2025 20:32:13 +0100

A database name containing a newline breaks things for this patch:

CREATE DATABASE "foo
bar";

$ pg_dumpall -Fc --file test
shell command argument contains a newline or carriage return: " dbname='foo
bar'"

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/