Re: refactoring comment.c

On Fri, Aug 6, 2010 at 11:36 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Robert Haas's message of vie ago 06 11:02:58 -0400 2010:
>
>> Any comments?  (ha ha ha...)
>
> Interesting idea.  The patch looks fine on a quick once-over.

Thanks for taking a look.

> Two small
> things: this comment
>
> +    /*
> +     * Databases, tablespaces, and roles are cluster-wide objects, so any
> +     * comments on those objects are record in the shared pg_shdescription
> +     * catalog.  Comments on all other objects are recorded in pg_description.
> +     */
>
> says "record" where it should say "recorded".

Thanks, good eye.

> Also, it strikes me that perhaps the ObjectAddress struct definition
> should be moved to the new header file; seems a more natural place for
> it (but then, it seems like a lot of files will need to include the new
> header, so perhaps that should be left to a subsequent patch).

I thought about that, but erred on the side of being conservative and
didn't move it. I like the idea, though.

> Thirdly, is it just me or just could replace a lot of code in DropFoo
> functions with this new auxiliary code?  Seems it's just missing
> "missing_ok" support ...

I am not sure how much code it would save you at the level of the
individual DropFoo() functions; I'd have to look through them more
carefully. But now that you mention it, what about getting rid of all
of the individual parse nodes for drop statements? Right now we have:

DropTableSpaceStmt
DropFdwStmt
DropForeignServerStmt
DropUserMappingStmt
DropPLangStmt
DropRoleStmt
DropStmt (table, sequence, view, index, domain, conversion, schema,
text search {parser, dictionary, template, configuration}
DropPropertyStmt (rules and triggers)
DropdbStmt (capitalized differently, just for fun)
DropCastStmt
RemoveFuncStmt (so you can't find it by grepping for Drop!)
RemoveOpClassStmt
RemoveOpFamilyStmt

It seems like we could probably unify all of these into a single
DropStmt, following the same pattern as CommentStmt, although I think
perhaps that should be a follow-on patch rather than doing it as part
of this one. GRANT also has some code to translate object names to
OIDs, which I thought might be able to use this machinery as well,
though I haven't really checked whether it makes sense.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company