Re: deparsing utility commands - Mailing list pgsql-hackers
From | Stephen Frost |
---|---|
Subject | Re: deparsing utility commands |
Date | |
Msg-id | 20150218200624.GB6717@tamriel.snowman.net Whole thread Raw |
In response to | Re: deparsing utility commands (Alvaro Herrera <alvherre@2ndquadrant.com>) |
Responses |
Re: deparsing utility commands
(Alvaro Herrera <alvherre@2ndquadrant.com>)
Re: deparsing utility commands (Alvaro Herrera <alvherre@2ndquadrant.com>) Re: deparsing utility commands (Alvaro Herrera <alvherre@2ndquadrant.com>) |
List | pgsql-hackers |
Alvaro, * Alvaro Herrera (alvherre@2ndquadrant.com) wrote: > 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. 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
pgsql-hackers by date: