Alvaro, * Alvaro Herrera (alvhe...@2ndquadrant.com) wrote: > Stephen Frost wrote: > > * Alvaro Herrera (alvhe...@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.
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
signature.asc
Description: Digital signature
