Обсуждение: 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

The author has received feedback so this has been saved for the next
commit-fest:
http://momjian.postgresql.org/cgi-bin/pgpatches_hold

---------------------------------------------------------------------------

Stephen Frost wrote:
-- Start of PGP signed section.
> * 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.html
>     Particularly question 1.5, which discusses how a patch should be
>     submitted.
> #2: The patch should be as readable as possible.  This includes not
>     making gratuitous whitespace changes (which are in a number of
>     places and just confuse things), comments like this:
>     /* Now we can get the object ?? */
>     also don't make for very easy reading.
> #3: The patch should be in contextual diff format, not unified diff.
> #4: Re-use existing structure and minimize code duplication
>     While I can understand some desire to restructure pg_dump code to
>     handle things as generalized objects, this patch doesn't actually go
>     all the way through and do that.  Instead it starts that work, only
>     adds support for functions, and then leaves the old methods and
>     whatnot the same.  Instead it should either be a large overhaul
>     (probably not necessary for the specific functionality being looked
>     for here) which is complete and well tested (and removes the old, no
>     longer used code), or it should be integrated into the existing
>     structure (which is what I would recommend here).
>     Given that both the new approach and the old were left after this
>     patch, there's some code duplication and really process
>     duplication happening.
> #5: Given the above, I would suggest making '-B' explicitly for
>     functions and drop the 'function:' heading requirement.
> #6: Passing an sql snippet to getFuncs to do the filtering strikes me as
>     a really terrible approach.  Instead, the approach used for schemas
>     and tables is much cleaner and using it would make it be consistant
>     with those other types.
> #7: Again, following with the existing approach, the schemas and tables
>     use global variables to pass around what to include/exclude.  Unless
>     you're going to rewrite the whole thing to not do that, you should
>     follow that example when adding support for functions.  eg, getFuncs
>     really doesn't/shouldn't need to have its function definition
>     changed.
> #8: Functions *can* be mixed-case, I'm pretty sure, and pg_dump should
>     definitely support that.  These kinds of issues would have been
>     handled for you if you had used processSQLNamePattern as the other
>     functions do.  This would also remove the need 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 would
>     hesitate to add more things which use it, and to add more pg_X
>     functions which then *also* are rarely used.  If it makes sense to
>     have pg_malloc/pg_free (and I'm not sold on that idea at all), then
>     it 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
-- End of PGP section, PGP failed!

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + If your life is a hard drive, Christ can be your backup. +