From: Tom Lane <tgl@sss.pgh.pa.us>
To: Imseih (AWS), Sami <simseih@amazon.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
Date: Fri, 02 Jun 2023 08:16:16 -0400
Message-ID: <49272.1685708176@sss.pgh.pa.us> (raw)
In-Reply-To: <2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com>
References: <2C1933AB-C2F8-499B-9D18-4AC1882256A0@amazon.com>
"Imseih (AWS), Sami" <simseih@amazon.com> writes:
> With the attached repro, pg_restore fails with
> pg_restore: error: could not execute query: ERROR: constraint "a_pkey" for table "a" does not exist
> Command was: CREATE FUNCTION public.a_f(c1_in text, c2 integer DEFAULT 60) RETURNS void
Hmph. The other thing worth noticing is that pg_dump prints
a warning:
pg_dump: warning: could not resolve dependency loop among these items:
or with -v:
pg_dump: warning: could not resolve dependency loop among these items:
pg_dump: FUNCTION a_f (ID 218 OID 40664)
pg_dump: CONSTRAINT a_pkey (ID 4131 OID 40663)
pg_dump: POST-DATA BOUNDARY (ID 4281)
pg_dump: TABLE DATA a (ID 4278 OID 40657)
pg_dump: PRE-DATA BOUNDARY (ID 4280)
So it's lacking a rule to tell it what to do in this case, and the
default is the wrong way around. I think we need to fix it in
about the same way as the equivalent case for matviews, which
leads to the attached barely-tested patch.
BTW, now that I see a case the default printout here seems
completely ridiculous. I think we need to do
pg_log_warning("could not resolve dependency loop among these items:");
for (i = 0; i < nLoop; i++)
{
char buf[1024];
describeDumpableObject(loop[i], buf, sizeof(buf));
- pg_log_info(" %s", buf);
+ pg_log_warning(" %s", buf);
}
but I didn't actually change that in the attached.
regards, tom lane
Attachments:dump-loop-fix.patch (text/x-patch, 3.4K) downloaddiff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.cindex 3af97a6039..689a3acff5 100644--- a/src/bin/pg_dump/pg_dump.c+++ b/src/bin/pg_dump/pg_dump.c@@ -6085,6 +6085,7 @@ getAggregates(Archive *fout, int *numAggs)
agginfo[i].aggfn.argtypes,
agginfo[i].aggfn.nargs);
}
+ agginfo[i].aggfn.postponed_def = false; /* might get set during sort */
/* Decide whether we want to dump it */
selectDumpableObject(&(agginfo[i].aggfn.dobj), fout);
@@ -6283,6 +6284,7 @@ getFuncs(Archive *fout, int *numFuncs)
parseOidArray(PQgetvalue(res, i, i_proargtypes),
finfo[i].argtypes, finfo[i].nargs);
}
+ finfo[i].postponed_def = false; /* might get set during sort */
/* Decide whether we want to dump it */
selectDumpableObject(&(finfo[i].dobj), fout);
@@ -12168,7 +12170,8 @@ dumpFunc(Archive *fout, const FuncInfo *finfo)
.namespace = finfo->dobj.namespace->dobj.name,
.owner = finfo->rolname,
.description = keyword,
- .section = SECTION_PRE_DATA,+ .section = finfo->postponed_def ?+ SECTION_POST_DATA : SECTION_PRE_DATA,
.createStmt = q->data,
.dropStmt = delqry->data));
diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.hindex ed6ce41ad7..bc8f2ec36d 100644--- a/src/bin/pg_dump/pg_dump.h+++ b/src/bin/pg_dump/pg_dump.h@@ -227,6 +227,7 @@ typedef struct _funcInfo
int nargs;
Oid *argtypes;
Oid prorettype;
+ bool postponed_def; /* function must be postponed into post-data */
} FuncInfo;
/* AggInfo is a superset of FuncInfo */
diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.cindex 745578d855..759be027c8 100644--- a/src/bin/pg_dump/pg_dump_sort.c+++ b/src/bin/pg_dump/pg_dump_sort.c@@ -868,6 +868,28 @@ repairMatViewBoundaryMultiLoop(DumpableObject *boundaryobj,
}
}
+/*+ * If a function is involved in a multi-object loop, we can't currently fix+ * that by splitting it into two DumpableObjects. As a stopgap, we try to fix+ * it by dropping the constraint that the function be dumped in the pre-data+ * section. This is sufficient to handle cases where a function depends on+ * some unique index, as can happen if it has a GROUP BY for example.+ */+static void+repairFunctionBoundaryMultiLoop(DumpableObject *boundaryobj,+ DumpableObject *nextobj)+{+ /* remove boundary's dependency on object after it in loop */+ removeObjectDependency(boundaryobj, nextobj->dumpId);+ /* if that object is a function, mark it as postponed into post-data */+ if (nextobj->objType == DO_FUNC)+ {+ FuncInfo *nextinfo = (FuncInfo *) nextobj;++ nextinfo->postponed_def = true;+ }+}+
/*
* Because we make tables depend on their CHECK constraints, while there
* will be an automatic dependency in the other direction, we need to break
@@ -1062,6 +1084,28 @@ repairDependencyLoop(DumpableObject **loop,
}
}
+ /* Indirect loop involving function and data boundary */+ if (nLoop > 2)+ {+ for (i = 0; i < nLoop; i++)+ {+ if (loop[i]->objType == DO_FUNC)+ {+ for (j = 0; j < nLoop; j++)+ {+ if (loop[j]->objType == DO_PRE_DATA_BOUNDARY)+ {+ DumpableObject *nextobj;++ nextobj = (j < nLoop - 1) ? loop[j + 1] : loop[0];+ repairFunctionBoundaryMultiLoop(loop[j], nextobj);+ return;+ }+ }+ }+ }+ }+
/* Table and CHECK constraint */
if (nLoop == 2 &&
loop[0]->objType == DO_TABLE &&
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, simseih@amazon.com, pgsql-hackers@lists.postgresql.org
Subject: Re: [BUG] pg_dump does not properly deal with BEGIN ATOMIC function
In-Reply-To: <49272.1685708176@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox