Re: deparsing utility commands

Alvaro,

* Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> 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.

Agreed, that thought occured to me as well while I was looking through
the other deparse patches and I agree that it makes sense.

> 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);
> }

That'd be fine with me, though for my 2c, I wouldn't object to changing
them all to return ObjectAddress either. I agree that it'd cause a fair
bit of code churn to do so, but there's a fair bit of code churn
happening here anyway (looking at what 0008 does to ProcessUtilitySlow
anyway).

> > > 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.

That sounds like a good idea.

Thanks!

Stephen