Обсуждение: BUG #17753: pg_dump --if-exists bug

The following bug has been logged on the website:

Bug reference:      17753
Logged by:          Justin Zhang
Email address:      justin@tonic.ai
PostgreSQL version: 15.1
Operating system:   MacOS 12.6
Description:

Running pg_dump with the options "--section=pre-data --no-owner
--no-privileges --no-subscriptions --no-publications --clean --if-exists
--file=/Users/ju5tinz/Dev/pg_pass_test/pre_data.sql" prints the warning: 
pg_dump: warning: could not find where to insert IF EXISTS in statement "--
*not* dropping schema, since initdb creates it
"

Seems like pg_dump is trying to add the IF EXISTS operator to a comment
because of the --if-exists option. Tested this in v14 and the warning does
not appear. Also, the sql file is generated successfully.

On Thu, 19 Jan 2023 at 22:31, PG Bug reporting form
<noreply@postgresql.org> wrote:
> Running pg_dump with the options "--section=pre-data --no-owner
> --no-privileges --no-subscriptions --no-publications --clean --if-exists
> --file=/Users/ju5tinz/Dev/pg_pass_test/pre_data.sql" prints the warning:
> pg_dump: warning: could not find where to insert IF EXISTS in statement "--
> *not* dropping schema, since initdb creates it
> "

Are you able to share the full warning message including the statement?

David

David Rowley <dgrowleyml@gmail.com> writes:
> On Thu, 19 Jan 2023 at 22:31, PG Bug reporting form
> <noreply@postgresql.org> wrote:
>> Running pg_dump with the options "--section=pre-data --no-owner
>> --no-privileges --no-subscriptions --no-publications --clean --if-exists
>> --file=/Users/ju5tinz/Dev/pg_pass_test/pre_data.sql" prints the warning:
>> pg_dump: warning: could not find where to insert IF EXISTS in statement "--
>> *not* dropping schema, since initdb creates it
>> "

> Are you able to share the full warning message including the statement?

Noting that that string is only associated with the public schema,
I tried dumping a DB in which the public schema had been dropped
--- no dice --- and then recreated --- and then it reproduces!
Apparently, a schema that's named "public" but isn't the original
one is confusing something somewhere, but I'm not immediately
seeing where.

            regards, tom lane

I wrote:
> Noting that that string is only associated with the public schema,
> I tried dumping a DB in which the public schema had been dropped
> --- no dice --- and then recreated --- and then it reproduces!
> Apparently, a schema that's named "public" but isn't the original
> one is confusing something somewhere, but I'm not immediately
> seeing where.

Ah, here's the difference: a schema named "public" will be marked
dumpable if its owner is not pg_database_owner:

    else if (strcmp(nsinfo->dobj.name, "public") == 0)
    {
        /*
         * The public schema is a strange beast that sits in a sort of
         * no-mans-land between being a system object and a user object.
         * CREATE SCHEMA would fail, so its DUMP_COMPONENT_DEFINITION is just
         * a comment and an indication of ownership.  If the owner is the
         * default, omit that superfluous DUMP_COMPONENT_DEFINITION.  Before
         * v15, the default owner was BOOTSTRAP_SUPERUSERID.
         */
        nsinfo->create = false;
        nsinfo->dobj.dump = DUMP_COMPONENT_ALL;
        if (nsinfo->nspowner == ROLE_PG_DATABASE_OWNER)
            nsinfo->dobj.dump &= ~DUMP_COMPONENT_DEFINITION;

Having done that, RestoreArchive's kluge to inject IF EXISTS
clauses fails, because (unlike other places in pg_backup_archiver.c)
it is unaware that the dropStmt might be just a comment.

I think we need to do the attached, more or less.  Also, although the
given case seems to be new, we'd probably better back-patch, because
I'm not convinced that there aren't other ways to reach this.  The
warning message is just cosmetic (the dump output is the same either
way), so possibly this has been seen in the field but not reported.

            regards, tom lane

diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 7f7a0f1ce7..ba5e6acbbb 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -538,9 +538,14 @@ RestoreArchive(Archive *AHX)
                  */
                 if (*te->dropStmt != '\0')
                 {
-                    if (!ropt->if_exists)
+                    if (!ropt->if_exists ||
+                        strncmp(te->dropStmt, "--", 2) == 0)
                     {
-                        /* No --if-exists?    Then just use the original */
+                        /*
+                         * Without --if-exists, or if it's just a comment (as
+                         * happens for the public schema), print the dropStmt
+                         * as-is.
+                         */
                         ahprintf(AH, "%s", te->dropStmt);
                     }
                     else

I wrote:
> I think we need to do the attached, more or less.  Also, although the
> given case seems to be new, we'd probably better back-patch, because
> I'm not convinced that there aren't other ways to reach this.

Ah --- we only need to back-patch to v15, because this business of
possibly having a comment in the dropStmt is new in a7a7be1f2.
That commit also added the existing exceptions for comments in
pg_backup_archiver.c ... but it missed the need to have special
cases for dropStmts too.

            regards, tom lane

On Thu, Jan 19, 2023 at 07:09:50PM -0500, Tom Lane wrote:
> I wrote:
> > I think we need to do the attached, more or less.  Also, although the
> > given case seems to be new, we'd probably better back-patch, because
> > I'm not convinced that there aren't other ways to reach this.
> 
> Ah --- we only need to back-patch to v15, because this business of
> possibly having a comment in the dropStmt is new in a7a7be1f2.
> That commit also added the existing exceptions for comments in
> pg_backup_archiver.c ... but it missed the need to have special
> cases for dropStmts too.

Your commit 74739d1 looks good.  Thanks.