Re: Patch for pg_dump (function dumps) - Mailing list pgsql-hackers

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. +