Re: Patch for pg_dump (function dumps)

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(at)gmail(dot)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(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +