Re: deparsing utility commands

Stephen,

Thanks for the review. I have pushed these patches, squashed in one
commit, with the exception of the one that changed ALTER TABLE. You had
enough comments about that one that I decided to put it aside for now,
and get the other ones done. I will post a new series shortly.

I fixed (most of?) the issues you pointed out; some additional comments:

Stephen Frost wrote:
> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:

> > @@ -1004,6 +1004,10 @@ AddNewRelationType(const char *typeName,
> > * use_user_acl: TRUE if should look for user-defined default permissions;
> > * if FALSE, relacl is always set NULL
> > * allow_system_table_mods: TRUE to allow creation in system namespaces
> > + * is_internal: is this a system-generated catalog?
>
> Shouldn't adding the comment for is_internal be done independently?
> It's clearly missing in master and fixing that hasn't got anything to do
> with the other changes happening here.

That was pushed separately and backpatched to 9.3. Turns out it was my
own very old mistake.

> This patch did make me think that there's quite a few places which could
> use ObjectAddressSet() that aren't (even after this patch). I don't
> think we need to stress over that, but it might be nice to mark that
> down as a TODO for someone to go through all the places where we
> construct an ObjectAddress and update them to use the new macro.

Agreed. I fixed the ones in these patches, but of course there's a lot
of them in other places.

> > @@ -772,7 +775,7 @@ DefineOpFamily(CreateOpFamilyStmt *stmt)
> > * other commands called ALTER OPERATOR FAMILY exist, but go through
> > * different code paths.
> > */
> > -Oid
> > +void
> > AlterOpFamily(AlterOpFamilyStmt *stmt)
> > {
> > Oid amoid, /* our AM's oid */
> > @@ -826,8 +829,6 @@ AlterOpFamily(AlterOpFamilyStmt *stmt)
> > AlterOpFamilyAdd(stmt->opfamilyname, amoid, opfamilyoid,
> > maxOpNumber, maxProcNumber,
> > stmt->items);
> > -
> > - return opfamilyoid;
> > }
>
> This feels a bit funny to me..? Why not construct and return an
> ObjectAddress like the other Alter functions?

The reason for not changing AlterOpFamily is that it needs completely
separate support for event triggers -- a simple ObjectAddress is not
enough. If you had a look at the support for GrantStmt in the rest of
the series, you have an idea of what supporting this one needs: we need
to store additional information somewhere in the event trigger queue.
Most likely, this function will end up returning an ObjectAddress as
well, but I want to write the deparse code first to make sure the change
will make sense.

In the committed patch, I ended up not changing this from Oid to void;
that was just pointless churn.

> > --- a/src/backend/commands/extension.c
> > +++ b/src/backend/commands/extension.c
> > @@ -2402,7 +2402,7 @@ extension_config_remove(Oid extensionoid, Oid tableoid)
> > * Execute ALTER EXTENSION SET SCHEMA
> > */
> > ObjectAddress
> > -AlterExtensionNamespace(List *names, const char *newschema)
> > +AlterExtensionNamespace(List *names, const char *newschema, Oid *oldschema)
> > {
> > char *extensionName;
> > Oid extensionOid;
>
> Output parameter is not an ObjectAddress for this one? The other case
> where a schema was an output parameter, it was.. Feels a little odd.

What's going on here is that this function is not called directly by
ProcessUtilitySlow, but rather through ExecAlterObjectSchemaStmt(); it
seemed simpler to keep the OID return here, and only build the
ObjectAddress when needed. I did it that way because there's a path
through AlterExtensionNamespace itself that goes back through
AlterObjectNamespace_oid that requires the OID, and the address would
just be clutter most of the time. It seemed simpler. Not that much of
an issue anyhow, since this only affects four functions or so; it's
easily changed if necessary.

> Also, the function comment needs updating to reflect the new output
> parameter.

A large number of these functions were not documenting their APIs at
all, and most of them seemed self-explanatory. I updated the ones for
which it seemed worthwhile.

> Not sure if it's most helpful for me to continue to provide reviews or
> if it'd be useful for me to help with actually making some of these
> changes- please let me know your thoughts and I'll do my best to help.

Given the number of patches in the series, I think it's easiest for both
you and for me to give comments. I rebase this stuff pretty frequently
for submission.

--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services