Re: deparsing utility commands

Stephen,

Stephen Frost wrote:

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

> > Patch 0002 I think is good to go as well, AFAICT (have the various
> > RENAME commands return the OID and attnum of affected objects).
>
> It's not a huge complaint, but it feels a bit awkward to me that
> ExecRenameStmt is now returning one item and using an out variable for
> the other when the two really go together (Oid and Object Sub ID, that
> is). Further, the comment above ExecRenameStmt should make it clear
> that it's safe to pass NULL into objsubid if you don't care about it.
>
> The same probably goes for the COMMENT bits.

Hmm, while I agree that it's a relatively minor point, it seems a fair
one. I think we could handle this by returning ObjectAddress rather
than Oid in ExecRenameStmt() and CommentObject(); then you have all the
bits you need in a single place. Furthermore, the function in another
patch EventTriggerStashCommand() instead of getting separately (ObjType,
objectId, objectSubId) could take a single argument of type
ObjectAddress.

Now, we probably don't want to hack *all* the utility commands to return
ObjectAddress instead of OID, because it many cases that's just not
going to be convenient (not to speak of the code churn); so I think for
most objtypes the ProcessUtilitySlow stanza would look like this:

case T_AlterTSConfigurationStmt:
objectId = AlterTSConfiguration((AlterTSConfigurationStmt *) parsetree);
objectType = OBJECT_TSCONFIGURATION;
break;

For ExecRenameStmt and CommentObject (and probably other cases such as
security labels) the stanza in ProcessUtilitySlow would be simpler:

case T_CommentStmt:
address = CommentObject((CommentStmt *) parsetree);
break;

and at the bottom of the loop we would transform the objid/type into
address for the cases that need it:

if (!commandStashed)
{
if (objectId != InvalidOid)
{
address.classId = get_objtype_catalog_oid(objectType);
address.objectId = objectId;
address.objectSubId = 0;
}
EventTriggerStashCommand(address, secondaryOid, parsetree);
}

> > On 0006 (which is about having ALTER TABLE return the OID/attnum of the
> > affected object on each subcommand), I have a problem about the ALTER
> > TABLE ALTER COLUMN SET DATA TYPE USING subcommand. The problem with
> > that is that in order to fully deparse that subcommand we need to
> > deparse the expression of the USING clause. But in the parse node we
> > only have info about the untransformed expression, so it is not possible
> > to pass it through ruleutils directly; it needs to be run by
> > transformExpr() first.
>
> I agree- I'm pretty sure we definitely don't want to run through
> transformExpr again in the deparse code. I'm not a huge fan of adding a
> Node* output parameter, but I havn't got any other great ideas about how
> to address that.

Yeah, my thoughts exactly :-(

> > > I think the GRANT/REVOKE, COMMENT ON and SECURITY LABEL changes are good
> > > independently as well, but there previously have been raised some
> > > concerns about shared objects. I think the answer in the patches which
> > > is to raise events when affecting database local objects makes sense,
> > > but others might disagree.
> >
> > Yes, I will push these unless somebody objects soon, as they seem
> > perfectly reasonable to me. The only troubling thing is that there is
> > no regression test for this kind of thing in event triggers (i.e. verify
> > which command tags get support and which don't), which seems odd to me.
> > Not these patches's fault, though, so I'm not considering adding any ATM.
>
> Ugh. I dislike that when we say an event trigger will fire before
> 'GRANT' what we really mean is "GRANT when it's operating against a
> local object". At the minimum we absolutely need to be very clear in
> the documentation about that limitation. Perhaps there is something
> already which reflects that, but I don't see anything in the patch
> being added about that.

Hmm, good point, will give this some thought. I'm thinking perhaps we
can add a table of which object types are supported for generic commands
such as GRANT, COMMENT and SECURITY LABEL.

> Still looking at the rest of the patches.

Great, thanks -- and thanks for the review so far.

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