Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
On Wed, Oct 17, 2012 at 11:43 PM, Alvaro Herrera alvhe...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Joel Jacobson wrote: On Wed, Oct 17, 2012 at 11:43 PM, Alvaro Herrera alvhe...@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 Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Joel Jacobson wrote:
On Thu, Jul 5, 2012 at 10:33 PM, Tom Lane t...@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 Herrerahttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training Services
*** a/src/bin/pg_dump/pg_dump.c
--- b/src/bin/pg_dump/pg_dump.c
***
*** 3534,3539 getAggregates(Archive *fout, int *numAggs)
--- 3534,3540
int i_proargtypes;
int i_rolname;
int i_aggacl;
+ int i_proiargs;
/* Make sure we are in proper schema */
selectSourceSchema(fout, pg_catalog);
***
*** 3543,3553 getAggregates(Archive *fout, int *numAggs)
* rationale behind the filtering logic.
*/
! if (fout-remoteVersion = 80200)
{
appendPQExpBuffer(query, SELECT tableoid, oid, proname AS aggname,
pronamespace AS aggnamespace,
pronargs, proargtypes,
(%s proowner) AS rolname,
proacl AS aggacl
FROM pg_proc p
--- 3544,3555
* rationale behind the filtering logic.
*/
! if (fout-remoteVersion = 80400)
{
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
***
*** 3565,3576 getAggregates(Archive *fout, int *numAggs)
--- 3567,3594
deptype = 'e'));
appendPQExpBuffer(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
***
*** 3585,3590 getAggregates(Archive *fout, int *numAggs)
--- 3603,3609
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
***
*** 3600,3605 getAggregates(Archive *fout, int *numAggs)
--- 3619,3625
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
***
*** 3623,3628 getAggregates(Archive *fout, int *numAggs)
--- 3643,3649
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++)
{
***
*** 3642,3647 getAggregates(Archive *fout, int *numAggs)
--- 3663,3669
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 =
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
On Wed, Oct 17, 2012 at 5:43 PM, Alvaro Herrera alvhe...@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+). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Makes pg_dump sort overloaded functions in deterministic order. The field proiargs has been added to FuncInfo and is set by getFuncs() and getAggregates() for all functions and aggregates. DOTypeNameCompare uses this field to break ties if the name and number of arguments are the same. This avoid having to default to OID sorting. This patch is independent from the ongoing discussion of the pg_dump --split option. Even if we can't agree on how to do the splitting of objects into files, it still makes sense to fix the sort order of overloaded functions. pg_dump_deterministic_order_v4.patch Description: Binary data pg_dump_deterministic_order.t Description: Troff document -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
On Thu, Jul 5, 2012 at 10:33 PM, Tom Lane t...@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. What do you think?
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
New version, made a typo in last one. pg_dump_deterministic_order_v3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Joel Jacobson j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
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 j...@trustly.com javascript:; 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
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Joel Jacobson j...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
Roger that. I'm on it. On Thursday, July 5, 2012, Tom Lane wrote: Joel Jacobson j...@trustly.com javascript:; writes: 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
[HACKERS] [PATCH] pg_dump: Sort overloaded functions in deterministic order
I have received positive feedback on the pg_dump --split option I suggested, but it depends on pg_dump dumping objects in a deterministic order. I'm committed to fixing this. The first problem I've spotted is overloaded functions. This patch adds a new element to DumpableObject: char *proargs This is set to the output from pg_get_function_identity_arguments(oid) for all functions, and set to NULL for all other objects. sortDumpableObjectsByTypeName calls DOTypeNameCompare, which in addition to sorting objects by type, namespace and name, now also sorts by the function identity arguments. This makes overloaded functions being dumped in the same order, regardless of which order they were created. Are there any other object types where the order isn't deterministic? pg_dump_deterministic_order.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] [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. pg_dump_deterministic_order_v2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
