Re: Patch for pg_dump (function dumps)
От | Bruce Momjian |
---|---|
Тема | Re: Patch for pg_dump (function dumps) |
Дата | |
Msg-id | 200804030129.m331Ttf06460@momjian.us обсуждение исходный текст |
Ответ на | Re: Patch for pg_dump (function dumps) (Stephen Frost <sfrost@snowman.net>) |
Список | 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. +
В списке pgsql-hackers по дате отправления: