Skip site navigation (1) Skip section navigation (2)

Re: Patch for pg_dump (function dumps)

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Stephen Frost <sfrost(at)snowman(dot)net>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: Patch for pg_dump (function dumps)
Date: 2008-04-03 01:29:55
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
The author has received feedback so this has been saved for the next


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:
> 	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>

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

In response to

pgsql-hackers by date

Next:From: Zoltan BoszormenyiDate: 2008-04-03 05:52:25
Previous:From: D'Arcy J.M. CainDate: 2008-04-03 00:55:52
Subject: Re: modules

Privacy Policy | About PostgreSQL
Copyright © 1996-2017 The PostgreSQL Global Development Group