Re: Patch for pg_dump (function dumps)

[Stephen Frost <sfrost(at)snowman(dot)net>]

* 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

Attachment: signature.asc
Description: Digital signature