From e3bfe6d84d4919433d8323cfb8194ca60d99f2c4 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Fri, 6 Mar 2015 13:27:46 -0500 Subject: [PATCH] Rethink function argument sorting in pg_dump. Commit 7b583b20b1c95acb621c71251150beef958bb603 created an unnecessary dump failure hazard by applying pg_get_function_identity_arguments() to every function in the database, even those that won't get dumped. This could result in snapshot-related problems if concurrent sessions are, for example, creating and dropping temporary functions, as noted by Marko Tiikkaja in bug #12832. While this is by no means pg_dump's only such issue with concurrent DDL, it's unfortunate that we added a new failure mode for cases that used to work, and even more so that the failure was created for basically cosmetic reasons (ie, to sort overloaded functions more deterministically). To fix, revert that patch and instead sort function arguments using information that pg_dump has available anyway, namely the names of the argument types. This will produce a slightly different sort ordering for overloaded functions than the previous coding; but applying strcmp directly to the output of pg_get_function_identity_arguments really was a bit odd anyway. The sorting will still be name-based and hence independent of possibly-installation-specific OID assignments. A small additional benefit is that sorting now works regardless of server version. Back-patch to 9.3, where the previous commit appeared. --- src/bin/pg_dump/pg_dump.c | 47 ++-------------------------------- src/bin/pg_dump/pg_dump.h | 1 - src/bin/pg_dump/pg_dump_sort.c | 23 ++++++++++++++--- 3 files changed, 22 insertions(+), 49 deletions(-) diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c index 41a0107491..fdfb4317f4 100644 --- a/src/bin/pg_dump/pg_dump.c +++ b/src/bin/pg_dump/pg_dump.c @@ -4199,7 +4199,6 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) int i_proargtypes; int i_rolname; int i_aggacl; - int i_proiargs; /* Make sure we are in proper schema */ selectSourceSchema(fout, "pg_catalog"); @@ -4209,12 +4208,11 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) * rationale behind the filtering logic. */ - if (fout->remoteVersion >= 80400) + if (fout->remoteVersion >= 80200) { appendPQExpBuffer(query, "SELECT tableoid, oid, proname AS aggname, " "pronamespace AS aggnamespace, " "pronargs, proargtypes, " - "pg_catalog.pg_get_function_identity_arguments(oid) AS proiargs," "(%s proowner) AS rolname, " "proacl AS aggacl " "FROM pg_proc p " @@ -4232,28 +4230,12 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) "deptype = 'e')"); appendPQExpBufferChar(query, ')'); } - else if (fout->remoteVersion >= 80200) - { - appendPQExpBuffer(query, "SELECT tableoid, oid, proname AS aggname, " - "pronamespace AS aggnamespace, " - "pronargs, proargtypes, " - "NULL::text AS proiargs," - "(%s proowner) AS rolname, " - "proacl AS aggacl " - "FROM pg_proc p " - "WHERE proisagg AND (" - "pronamespace != " - "(SELECT oid FROM pg_namespace " - "WHERE nspname = 'pg_catalog'))", - username_subquery); - } else if (fout->remoteVersion >= 70300) { appendPQExpBuffer(query, "SELECT tableoid, oid, proname AS aggname, " "pronamespace AS aggnamespace, " "CASE WHEN proargtypes[0] = 'pg_catalog.\"any\"'::pg_catalog.regtype THEN 0 ELSE 1 END AS pronargs, " "proargtypes, " - "NULL::text AS proiargs, " "(%s proowner) AS rolname, " "proacl AS aggacl " "FROM pg_proc " @@ -4268,7 +4250,6 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) "0::oid AS aggnamespace, " "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, " "aggbasetype AS proargtypes, " - "NULL::text AS proiargs, " "(%s aggowner) AS rolname, " "'{=X}' AS aggacl " "FROM pg_aggregate " @@ -4284,7 +4265,6 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) "0::oid AS aggnamespace, " "CASE WHEN aggbasetype = 0 THEN 0 ELSE 1 END AS pronargs, " "aggbasetype AS proargtypes, " - "NULL::text AS proiargs, " "(%s aggowner) AS rolname, " "'{=X}' AS aggacl " "FROM pg_aggregate " @@ -4308,7 +4288,6 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) i_proargtypes = PQfnumber(res, "proargtypes"); i_rolname = PQfnumber(res, "rolname"); i_aggacl = PQfnumber(res, "aggacl"); - i_proiargs = PQfnumber(res, "proiargs"); for (i = 0; i < ntups; i++) { @@ -4328,7 +4307,6 @@ getAggregates(Archive *fout, DumpOptions *dopt, int *numAggs) agginfo[i].aggfn.lang = InvalidOid; /* not currently interesting */ agginfo[i].aggfn.prorettype = InvalidOid; /* not saved */ agginfo[i].aggfn.proacl = pg_strdup(PQgetvalue(res, i, i_aggacl)); - agginfo[i].aggfn.proiargs = pg_strdup(PQgetvalue(res, i, i_proiargs)); agginfo[i].aggfn.nargs = atoi(PQgetvalue(res, i, i_pronargs)); if (agginfo[i].aggfn.nargs == 0) agginfo[i].aggfn.argtypes = NULL; @@ -4380,7 +4358,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) int i_proargtypes; int i_prorettype; int i_proacl; - int i_proiargs; /* Make sure we are in proper schema */ selectSourceSchema(fout, "pg_catalog"); @@ -4401,13 +4378,12 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) * doesn't have; otherwise we might not get creation ordering correct. */ - if (fout->remoteVersion >= 80400) + if (fout->remoteVersion >= 70300) { appendPQExpBuffer(query, "SELECT tableoid, oid, proname, prolang, " "pronargs, proargtypes, prorettype, proacl, " "pronamespace, " - "pg_catalog.pg_get_function_identity_arguments(oid) AS proiargs," "(%s proowner) AS rolname " "FROM pg_proc p " "WHERE NOT proisagg AND (" @@ -4429,21 +4405,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) "deptype = 'e')"); appendPQExpBufferChar(query, ')'); } - else if (fout->remoteVersion >= 70300) - { - appendPQExpBuffer(query, - "SELECT tableoid, oid, proname, prolang, " - "pronargs, proargtypes, prorettype, proacl, " - "pronamespace, " - "NULL::text AS proiargs," - "(%s proowner) AS rolname " - "FROM pg_proc p " - "WHERE NOT proisagg AND (" - "pronamespace != " - "(SELECT oid FROM pg_namespace " - "WHERE nspname = 'pg_catalog'))", - username_subquery); - } else if (fout->remoteVersion >= 70100) { appendPQExpBuffer(query, @@ -4451,7 +4412,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) "pronargs, proargtypes, prorettype, " "'{=X}' AS proacl, " "0::oid AS pronamespace, " - "NULL::text AS proiargs," "(%s proowner) AS rolname " "FROM pg_proc " "WHERE pg_proc.oid > '%u'::oid", @@ -4468,7 +4428,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) "pronargs, proargtypes, prorettype, " "'{=X}' AS proacl, " "0::oid AS pronamespace, " - "NULL::text AS proiargs," "(%s proowner) AS rolname " "FROM pg_proc " "where pg_proc.oid > '%u'::oid", @@ -4494,7 +4453,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) i_proargtypes = PQfnumber(res, "proargtypes"); i_prorettype = PQfnumber(res, "prorettype"); i_proacl = PQfnumber(res, "proacl"); - i_proiargs = PQfnumber(res, "proiargs"); for (i = 0; i < ntups; i++) { @@ -4510,7 +4468,6 @@ getFuncs(Archive *fout, DumpOptions *dopt, int *numFuncs) finfo[i].rolname = pg_strdup(PQgetvalue(res, i, i_rolname)); finfo[i].lang = atooid(PQgetvalue(res, i, i_prolang)); finfo[i].prorettype = atooid(PQgetvalue(res, i, i_prorettype)); - finfo[i].proiargs = pg_strdup(PQgetvalue(res, i, i_proiargs)); finfo[i].proacl = pg_strdup(PQgetvalue(res, i, i_proacl)); finfo[i].nargs = atoi(PQgetvalue(res, i, i_pronargs)); if (finfo[i].nargs == 0) diff --git a/src/bin/pg_dump/pg_dump.h b/src/bin/pg_dump/pg_dump.h index f42c42d5e6..a9d3c1016b 100644 --- a/src/bin/pg_dump/pg_dump.h +++ b/src/bin/pg_dump/pg_dump.h @@ -149,7 +149,6 @@ typedef struct _funcInfo Oid *argtypes; Oid prorettype; char *proacl; - char *proiargs; } 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.c index 4b9bba0723..c5ed593e02 100644 --- a/src/bin/pg_dump/pg_dump_sort.c +++ b/src/bin/pg_dump/pg_dump_sort.c @@ -291,13 +291,30 @@ DOTypeNameCompare(const void *p1, const void *p2) { FuncInfo *fobj1 = *(FuncInfo *const *) p1; FuncInfo *fobj2 = *(FuncInfo *const *) p2; + int i; cmpval = fobj1->nargs - fobj2->nargs; if (cmpval != 0) return cmpval; - cmpval = strcmp(fobj1->proiargs, fobj2->proiargs); - if (cmpval != 0) - return cmpval; + for (i = 0; i < fobj1->nargs; i++) + { + TypeInfo *argtype1 = findTypeByOid(fobj1->argtypes[i]); + TypeInfo *argtype2 = findTypeByOid(fobj2->argtypes[i]); + + if (argtype1 && argtype2) + { + if (argtype1->dobj.namespace && argtype2->dobj.namespace) + { + cmpval = strcmp(argtype1->dobj.namespace->dobj.name, + argtype2->dobj.namespace->dobj.name); + if (cmpval != 0) + return cmpval; + } + cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name); + if (cmpval != 0) + return cmpval; + } + } } else if (obj1->objType == DO_OPERATOR) { -- 2.40.0