Thread: [PATCH] pg_dump: Sort overloaded functions in deterministic order
I renamed the new element to DumpableObject from "proargs" to the more general name "sortkey".
This way this element can be used by any object types in the future,
which might require sorting by additional information than type, namespace and name.
Currently, it's only set for functions/aggregates though, its NULL for all other object types.
I felt less ugly to add a new element with a general name than one specific for functions.
I also moved the check to the last part of DOTypeNameCompare, just before sorting by OIDs as a last resort.
Feedback on the implementation is welcomed.
If this can be achieved without adding a new element to DumpableObject,
it is of course much better, but I couldn't find a way of doing that.
Attachment
New version, made a typo in last one.
Attachment
Joel Jacobson <joel@trustly.com> writes:
> New version, made a typo in last one.
I'm not particularly happy with the idea of adding a sortkey field to
DumpableObject as such, when most object types don't need it. That just
bloats the code and pg_dump's memory consumption. It would be better to
modify the already-existing object-type-specific special cases in
DOTypeNameCompare to take additional information into account as needed.
BTW, I see no reason to be adding extra calls of
pg_get_function_identity_arguments. What is wrong with the funcsig or
aggsig strings that the code already computes?
regards, tom lane
I agree, good suggestion, I just didn't know how to implement it without a new field. I'll make a new attempt to get it right.
On Thursday, July 5, 2012, Tom Lane wrote:
Joel Jacobson <joel@trustly.com> writes:
> New version, made a typo in last one.
I'm not particularly happy with the idea of adding a sortkey field to
DumpableObject as such, when most object types don't need it. That just
bloats the code and pg_dump's memory consumption. It would be better to
modify the already-existing object-type-specific special cases in
DOTypeNameCompare to take additional information into account as needed.
BTW, I see no reason to be adding extra calls of
pg_get_function_identity_arguments. What is wrong with the funcsig or
aggsig strings that the code already computes?
regards, tom lane
Joel Jacobson <joel@trustly.com> writes:
> I agree, good suggestion, I just didn't know how to implement it without a
> new field. I'll make a new attempt to get it right.
You may in fact need a new field --- I'm just saying it should be in the
object-type-specific struct, eg FuncInfo, not DumpableObject.
regards, tom lane
Roger that. I'm on it.<span></span><br /><br />On Thursday, July 5, 2012, Tom Lane wrote:<br /><blockquote class="gmail_quote"style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Joel Jacobson <<a href="javascript:;">joel@trustly.com</a>>writes:<br /> You may in fact need a new field --- I'm just saying it shouldbe in the<br /> object-type-specific struct, eg FuncInfo, not DumpableObject.<br /><br /> regards,tom lane<br /></blockquote>
On Thu, Jul 5, 2012 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
You may in fact need a new field --- I'm just saying it should be in theobject-type-specific struct, eg FuncInfo, not DumpableObject.
I suggest adding char *funcsig to FuncInfo, and moving the "funcsig = format_function_arguments(finfo, funciargs)" code from dumpFunc to getFuncs.
Because dumpFunc is called after sortDumpableObjectsByTypeName, setting funcsig in the FuncInfo struct in dumpFunc would't work, as it needs to be available when entering sortDumpableObjectsByTypeName.
What do you think?
Joel Jacobson wrote: > On Thu, Jul 5, 2012 at 10:33 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > You may in fact need a new field --- I'm just saying it should be in the > > object-type-specific struct, eg FuncInfo, not DumpableObject. > > > I suggest adding char *funcsig to FuncInfo, and moving the "funcsig = > format_function_arguments(finfo, funciargs)" code from dumpFunc to getFuncs. > > Because dumpFunc is called after sortDumpableObjectsByTypeName, setting > funcsig in the FuncInfo struct in dumpFunc would't work, as it needs to be > available when entering sortDumpableObjectsByTypeName. Uh, the patch you posted keeps the pg_get_function_identity_arguments call in dumpFunc, but there is now also a new one in getFuncs. Do we need to remove the second one? Here's an updated patch for your consideration. I was about to push this when I noticed the above. The only change here is that the extra code that tests for new remoteVersions in the second "else if" branch of getFuncs and getAggregates has been removed, since it cannot ever be reached. (I tested the new pg_dump with 8.2 and HEAD and also verified it passes pg_upgrade's "make check". I didn't test with other server versions.) -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Attachment
On Wed, Oct 17, 2012 at 5:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > (I tested the new pg_dump with 8.2 and HEAD and also verified it passes > pg_upgrade's "make check". I didn't test with other server versions.) I also tested against 8.3 and 8.4 since 8.4 is the version that introduced pg_get_function_identity_arguments. The included testcase fails on 8.3 and succeeds on 8.4 (pg_dump succeeds in both cases of course but it's only ordered deterministically in 8.4+).
On Wed, Oct 17, 2012 at 11:43 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Uh, the patch you posted keeps the pg_get_function_identity_arguments > call in dumpFunc, but there is now also a new one in getFuncs. Do we > need to remove the second one? It could be done, but unfortunately we cannot use the value computed in dumpFunc(), because getFuncs() is called before dumpFunc(). The patch currently only affects getFuncs(), it doesn't touch dumpFunc(). What could be done is to keep the changes in getFuncs(), and also change dumpFunc() to use the value computed in getFuncs(), but I think the gain is small in relation to the complexity of changing dumpFunc(), as we would still need to make the two other function calls in the SQL query in dumpFunc() to pg_get_function_arguments() and pg_get_function_result(). > Here's an updated patch for your consideration. I was about to push > this when I noticed the above. The only change here is that the extra > code that tests for new remoteVersions in the second "else if" branch of > getFuncs and getAggregates has been removed, since it cannot ever be > reached. Looks really good.
Joel Jacobson wrote: > On Wed, Oct 17, 2012 at 11:43 PM, Alvaro Herrera > <alvherre@2ndquadrant.com> wrote: > > Uh, the patch you posted keeps the pg_get_function_identity_arguments > > call in dumpFunc, but there is now also a new one in getFuncs. Do we > > need to remove the second one? > > It could be done, but unfortunately we cannot use the value computed > in dumpFunc(), > because getFuncs() is called before dumpFunc(). Right, I got that from the discussion. > What could be done is to keep the changes in getFuncs(), and also > change dumpFunc() > to use the value computed in getFuncs(), but I think the gain is small > in relation > to the complexity of changing dumpFunc(), as we would still need to > make the two other > function calls in the SQL query in dumpFunc() to pg_get_function_arguments() and > pg_get_function_result(). Changing pg_dump is complex enough whatever the change, yes. I have not touched this. > > Here's an updated patch for your consideration. I was about to push > > this when I noticed the above. The only change here is that the extra > > code that tests for new remoteVersions in the second "else if" branch of > > getFuncs and getAggregates has been removed, since it cannot ever be > > reached. > > Looks really good. Thanks, pushed it. -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services