Date: Tue, 15 Jul 2025 17:19:57 -0700
From: Noah Misch <noah@leadboat.com>
To: Mahendra Singh Thalor <mahi6run@gmail.com>
Cc: Andrew Dunstan <andrew@dunslane.net>,
	=?iso-8859-1?Q?=C1lvaro?= Herrera <alvherre@alvh.no-ip.org>,
	jian he <jian.universality@gmail.com>,
	Srinath Reddy <srinath2133@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Non-text mode for pg_dumpall
Message-ID: <20250716001957.c6.nmisch@google.com>
References: <cd24aa81-152e-4194-9bbe-0f34a0a3f539@dunslane.net>
 <202503311812.vxg5b7rzfgf6@alvherre.pgsql>
 <CAKYtNArtO8T-u5=E4ibxFfW+zQ8dBmOZQFgRa36+27NFkdtK0Q@mail.gmail.com>
 <616efe2c-3986-43cf-b88c-4435849acf9e@dunslane.net>
 <CAKYtNAoE1xUcMmBq-1qCaZGUDQLfMSVa6hzBVA-PqbmSTzPf5A@mail.gmail.com>
 <CAKYtNAq530UTAtnVs5xfONQ0j6vS-Sys50p5+SNfC7G7_ghCVQ@mail.gmail.com>
 <948154fe-0278-4f4c-8f5a-085e12f03163@dunslane.net>
 <20250708212819.09.nmisch@google.com>
 <CAKYtNAr-6_CCz+tnJ0T2597Ar4iAZXkE1qPimsO9EY-Kn7LvzQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <CAKYtNAr-6_CCz+tnJ0T2597Ar4iAZXkE1qPimsO9EY-Kn7LvzQ@mail.gmail.com>
User-Agent: Mutt/2.2.12 (2023-09-09)
Archived-At: 
 <https://www.postgresql.org/message-id/20250716001957.c6.nmisch%40google.com>
Precedence: bulk

On Thu, Jul 10, 2025 at 12:21:03AM +0530, Mahendra Singh Thalor wrote:
> On Wed, 9 Jul 2025 at 02:58, Noah Misch <noah@leadboat.com> wrote:
> > On Fri, Apr 04, 2025 at 04:11:05PM -0400, Andrew Dunstan wrote:
> > > Thanks. I have pushed these now with a few further small tweaks.
> >
> > This drops all databases:
> >
> > pg_dumpall --clean -Fd -f /tmp/dump
> > pg_restore -d template1 --globals-only /tmp/dump
> >
> > That didn't match my expectations given this help text:
> >
> > $ pg_restore --help|grep global
> >   -g, --globals-only           restore only global objects, no databases
> 
> Databases are global objects so due to --clean command, we are putting
> drop commands in global.dat for all the databases. While restoring, we
> used the  "--globals-only" option so we are dropping all these
> databases by global.dat file.
> 
> Please let us know your expectations for this specific case.

Be consistent with "pg_dump".  A quick check suggests "pg_dump --clean"
affects plain format only.  For non-plain formats, only the pg_restore
argument governs the final commands:

$ rm -r /tmp/dump; pg_dump --clean -Fd -f /tmp/dump && pg_restore -f- /tmp/dump | grep DROP
$ rm -r /tmp/dump; pg_dump --clean -Fd -f /tmp/dump && pg_restore --clean -f- /tmp/dump | grep DROP
DROP TABLE public.example;

That said, you should audit code referencing the --clean flag and see if
there's more to it than that quick test suggests.  Note that aligning with
pg_dump will require changes for object types beyond databases.  "pg_restore
--clean" of a global dump should emit DROP TABLESPACE and DROP ROLE as
appropriate, regardless of whether the original pg_dumpall had --clean.

For my earlier example (pg_dumpall --clean; pg_restore --globals-only) I
expect the same outcome as plain-format "pg_dumpall --globals-only", which is
no databases dropped or created.  The help line says "no databases".  Plain
"pg_dumpall --globals-only" and even "pg_dumpall --globals-only --clean" do
not drop or create databases.

> > commit 1495eff wrote:
> > > --- a/src/bin/pg_dump/pg_dumpall.c
> > > +++ b/src/bin/pg_dump/pg_dumpall.c
> >
> > > @@ -1612,9 +1683,27 @@ dumpDatabases(PGconn *conn)
> > >                       continue;
> > >               }
> > >
> > > +             /*
> > > +              * If this is not a plain format dump, then append dboid and dbname to
> > > +              * the map.dat file.
> > > +              */
> > > +             if (archDumpFormat != archNull)
> > > +             {
> > > +                     if (archDumpFormat == archCustom)
> > > +                             snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".dmp", db_subdir, oid);
> > > +                     else if (archDumpFormat == archTar)
> > > +                             snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\".tar", db_subdir, oid);
> > > +                     else
> > > +                             snprintf(dbfilepath, MAXPGPATH, "\"%s\"/\"%s\"", db_subdir, oid);
> >
> > Use appendShellString() instead.  Plain mode already does that for the
> > "pg_dumpall -f" argument, which is part of db_subdir here.  We don't want
> > weird filename characters to work out differently for plain vs. non-plain
> > mode.  Also, it's easier to search for appendShellString() than to search for
> > open-coded shell quoting.
> 
> Yes, we can use appendShellString also. We are using snprintf in the
> pg_dump.c file also.
> Ex: snprintf(tagbuf, sizeof(tagbuf), "LARGE OBJECTS %u..%u",
>                      loinfo->looids[0], loinfo->looids[loinfo->numlos - 1]);

It's true snprintf() is not banned in these programs, but don't use it to do
the quoting for OS shell command lines or fragments thereof.  dbfilepath is a
fragment of an OS shell command line.  The LARGE OBJECTS string is not one of
those.  Hence, the LARGE OBJECTS scenario should keep using snprintf().

> If we want to use appendShellString, I can write a patch for these.
> Please let me know your opinion.

Use appendShellString() for shell quoting.  Don't attempt to use it for other
purposes.

> > > --- a/src/bin/pg_dump/pg_restore.c
> > > +++ b/src/bin/pg_dump/pg_restore.c
> >
> > > +/*
> > > + * read_one_statement
> > > + *
> > > + * This will start reading from passed file pointer using fgetc and read till
> > > + * semicolon(sql statement terminator for global.dat file)
> > > + *
> > > + * EOF is returned if end-of-file input is seen; time to shut down.
> >
> > What makes it okay to use this particular subset of SQL lexing?
> 
> To support complex syntax, we used this code from another file.

I'm hearing that you copied this code from somewhere.  Running
"git grep 'time to shut down'" suggests you copied it from
InteractiveBackend().  Is that right?  I do see other similarities between
read_one_statement() and InteractiveBackend().

Copying InteractiveBackend() provides negligible assurance that this is the
right subset of SQL lexing.  Only single-user mode uses InteractiveBackend().
Single-user mode survives mostly as a last resort for recovering from having
reached xidStopLimit, is rarely used, and only superusers write queries to it.

> > > +/*
> > > + * get_dbnames_list_to_restore
> > > + *
> > > + * This will mark for skipping any entries from dbname_oid_list that pattern match an
> > > + * entry in the db_exclude_patterns list.
> > > + *
> > > + * Returns the number of database to be restored.
> > > + *
> > > + */
> > > +static int
> > > +get_dbnames_list_to_restore(PGconn *conn,
> > > +                                                     SimpleOidStringList *dbname_oid_list,
> > > +                                                     SimpleStringList db_exclude_patterns)
> > > +{
> > > +     int                     count_db = 0;
> > > +     PQExpBuffer query;
> > > +     PGresult   *res;
> > > +
> > > +     query = createPQExpBuffer();
> > > +
> > > +     if (!conn)
> > > +             pg_log_info("considering PATTERN as NAME for --exclude-database option as no db connection while doing pg_restore.");
> >
> > When do we not have a connection here?  We'd need to document this behavior
> > variation if it stays, but I'd prefer if we can just rely on having a
> > connection.
> 
> Yes, we can document this behavior.

My review asked a question there.  I don't see an answer to that question.
Would you answer that question?