Re: Patch for pg_dump (function dumps)

* 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

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