Re: Patch for pg_dump (function dumps)

* Dany DeBontridder (dany118@gmail.com) wrote:
> I often need  in command line to get the code of function, so I make a
> patch for pg_dump, thanks this patch pg_dump is able to dump only one
> functions or all the functions.

First, a couple of general comments about the patch:

#1: You need to read the developer FAQ located here:   http://www.postgresql.org/docs/faqs.FAQ_DEV.htmlParticularly
question1.5, which discusses how a patch should besubmitted.
 
#2: The patch should be as readable as possible.  This includes notmaking gratuitous whitespace changes (which are in a
numberofplaces and just confuse things), comments like this:/* Now we can get the object ?? */also don't make for very
easyreading.
 
#3: The patch should be in contextual diff format, not unified diff.
#4: Re-use existing structure and minimize code duplicationWhile I can understand some desire to restructure pg_dump
codetohandle things as generalized objects, this patch doesn't actually goall the way through and do that.  Instead it
startsthat work, onlyadds support for functions, and then leaves the old methods andwhatnot the same.  Instead it
shouldeither be a large overhaul(probably not necessary for the specific functionality being lookedfor here) which is
completeand well tested (and removes the old, nolonger used code), or it should be integrated into the
existingstructure(which is what I would recommend here).Given that both the new approach and the old were left after
thispatch,there's some code duplication and really processduplication happening.
 
#5: Given the above, I would suggest making '-B' explicitly forfunctions and drop the 'function:' heading requirement.
#6: Passing an sql snippet to getFuncs to do the filtering strikes me asa really terrible approach.  Instead, the
approachused for schemasand tables is much cleaner and using it would make it be consistantwith those other types.
 
#7: Again, following with the existing approach, the schemas and tablesuse global variables to pass around what to
include/exclude. Unlessyou're going to rewrite the whole thing to not do that, you shouldfollow that example when
addingsupport for functions.  eg, getFuncsreally doesn't/shouldn't need to have its function definitionchanged.
 
#8: Functions *can* be mixed-case, I'm pretty sure, and pg_dump shoulddefinitely support that.  These kinds of issues
wouldhave beenhandled for you if you had used processSQLNamePattern as the otherfunctions do.  This would also remove
theneed for the pg_strcat,pg_free functions you've added, I believe.
 
#9: The vast majority of the code doesn't use 'pg_malloc' and so I wouldhesitate to add more things which use it, and
toadd more pg_Xfunctions which then *also* are rarely used.  If it makes sense tohave pg_malloc/pg_free (and I'm not
soldon that idea at all), thenit should be done consistantly, and probably seperately, from this.
 

This is probably enough.  My general feeling about this patch is that
it needs a rewrite and to be done using the existing structures and
following the same general processes we use for tables.  The resulting
code should be consistant and at least look like it was all written
towards a specific, defined structure.  That makes the code much more
maintainable and easier to pick up since you only have to understand one
structure which can be well documented rather than multiple not fully
thought out or documented structures.

As such, I would recommend rejecting this patch for this round and
waiting for a rewrite of it which can be reviewed during the next
commit-fest.
Thanks,
    Stephen