Re: deparsing utility commands - Mailing list pgsql-hackers

Stephen,

Stephen Frost wrote:

> * Alvaro Herrera (alvherre@2ndquadrant.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