Thread: Rethinking pg_dump's function sorting code
In bug #12832 Marko Tiikkaja points out that commit 7b583b20b1c95acb621c71251150beef958bb603 created a rather unnecessary dump failure hazard, since it applies pg_get_function_identity_arguments() to every function in the database, even those that won't get dumped. I think we should fix this by getting rid of pg_dump's use of that function altogether. A low-tech way to sort functions of identical names would be to compare argument type OIDs, as in the attached simple patch. If people feel it's important to avoid depending on numerical OID order, we could instead look up type names locally and compare them as in the attached less-simple patch. (Both patches neglect reverting the data collection aspects of the prior commit, since that's mechanical; the only interesting part is what we'll do to sort.) Neither patch will exactly preserve the sort behavior of the current code, but I don't think that's important. Comments? regards, tom lane diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 4b9bba0..816c9f0 100644 *** a/src/bin/pg_dump/pg_dump_sort.c --- b/src/bin/pg_dump/pg_dump_sort.c *************** DOTypeNameCompare(const void *p1, const *** 291,303 **** { FuncInfo *fobj1 = *(FuncInfo *const *) p1; FuncInfo *fobj2 = *(FuncInfo *const *) p2; cmpval = fobj1->nargs - fobj2->nargs; if (cmpval != 0) return cmpval; ! cmpval = strcmp(fobj1->proiargs, fobj2->proiargs); ! if (cmpval != 0) ! return cmpval; } else if (obj1->objType == DO_OPERATOR) { --- 291,307 ---- { FuncInfo *fobj1 = *(FuncInfo *const *) p1; FuncInfo *fobj2 = *(FuncInfo *const *) p2; + int i; cmpval = fobj1->nargs - fobj2->nargs; if (cmpval != 0) return cmpval; ! for (i = 0; i < fobj1->nargs; i++) ! { ! cmpval = oidcmp(fobj1->argtypes[i], fobj2->argtypes[i]); ! if (cmpval != 0) ! return cmpval; ! } } else if (obj1->objType == DO_OPERATOR) { diff --git a/src/bin/pg_dump/pg_dump_sort.c b/src/bin/pg_dump/pg_dump_sort.c index 4b9bba0..de9407c 100644 *** a/src/bin/pg_dump/pg_dump_sort.c --- b/src/bin/pg_dump/pg_dump_sort.c *************** DOTypeNameCompare(const void *p1, const *** 291,303 **** { FuncInfo *fobj1 = *(FuncInfo *const *) p1; FuncInfo *fobj2 = *(FuncInfo *const *) p2; cmpval = fobj1->nargs - fobj2->nargs; if (cmpval != 0) return cmpval; ! cmpval = strcmp(fobj1->proiargs, fobj2->proiargs); ! if (cmpval != 0) ! return cmpval; } else if (obj1->objType == DO_OPERATOR) { --- 291,313 ---- { FuncInfo *fobj1 = *(FuncInfo *const *) p1; FuncInfo *fobj2 = *(FuncInfo *const *) p2; + int i; cmpval = fobj1->nargs - fobj2->nargs; 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) ! { ! cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name); ! if (cmpval != 0) ! return cmpval; ! } ! } } else if (obj1->objType == DO_OPERATOR) {
On 2015-03-06 01:28, Tom Lane wrote: > In bug #12832 Marko Tiikkaja points out that commit > 7b583b20b1c95acb621c71251150beef958bb603 created a rather unnecessary > dump failure hazard, since it applies pg_get_function_identity_arguments() > to every function in the database, even those that won't get dumped. > I think we should fix this by getting rid of pg_dump's use of that > function altogether. A low-tech way to sort functions of identical names > would be to compare argument type OIDs, as in the attached simple patch. > If people feel it's important to avoid depending on numerical OID order, > we could instead look up type names locally and compare them as in the > attached less-simple patch. (Both patches neglect reverting the data > collection aspects of the prior commit, since that's mechanical; the only > interesting part is what we'll do to sort.) > > Neither patch will exactly preserve the sort behavior of the current > code, but I don't think that's important. > > Comments? I have my own cow in this ditch, but I would much prefer the sort to be done based on the type name. That way the order is still consistent between two databases where the objects were created in a different order. .m
On Thu, Mar 05, 2015 at 07:28:33PM -0500, Tom Lane wrote: > In bug #12832 Marko Tiikkaja points out that commit > 7b583b20b1c95acb621c71251150beef958bb603 created a rather unnecessary > dump failure hazard, since it applies pg_get_function_identity_arguments() > to every function in the database, even those that won't get dumped. > I think we should fix this by getting rid of pg_dump's use of that > function altogether. A low-tech way to sort functions of identical names > would be to compare argument type OIDs, as in the attached simple patch. > If people feel it's important to avoid depending on numerical OID order, > we could instead look up type names locally and compare them as in the > attached less-simple patch. Comparing argument type names sounds fine. Comparing argument type OID does not offer enough to justify the loss of cross-cluster sort equivalence. > Neither patch will exactly preserve the sort behavior of the current > code, but I don't think that's important. Agreed. > --- 291,313 ---- > { > FuncInfo *fobj1 = *(FuncInfo *const *) p1; > FuncInfo *fobj2 = *(FuncInfo *const *) p2; > + int i; > > cmpval = fobj1->nargs - fobj2->nargs; > 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) > ! { > ! cmpval = strcmp(argtype1->dobj.name, argtype2->dobj.name); > ! if (cmpval != 0) > ! return cmpval; > ! } > ! } So as to stably compare f(nsp0.t) to f(nsp1.t), this should also compare the dobj.namespace name.
Noah Misch <noah@leadboat.com> writes: > Comparing argument type names sounds fine. Comparing argument type OID does > not offer enough to justify the loss of cross-cluster sort equivalence. Fair enough. > So as to stably compare f(nsp0.t) to f(nsp1.t), this should also compare the > dobj.namespace name. Ah, good point. Will fix. regards, tom lane