Re: Event Triggers: adding information

Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:
> I gave this patch a look. I'm not done with it; I mostly gave the
> utility.c changes some love so that the end result is not as cluttered.

Thanks Álvaro!

> I did this by creating a couple of macros that call the Start() thing,
> then set the OID, then call the End() thing. This made pretty obvious
> the places that were missing to set the object OID; I changed the CREATE
> TABLE AS code to return it, for instance. The patch I attach applies on
> top of Dimitri's v6 patch. With these changes, it's much easier to
> review the big standard_ProcessUtility() switch and verify cases that
> are being handled correctly. (A few places cannot use the macros I
> defined because they use more complex arrangements. This is fine, I
> think, because they are few enough that can be verified manually.)

That sounds great. I'm not at ease with creating avdanved C macros and
I'm very happy you did it :)

> I also noticed that ALTER DEFAULT PRIVILEGES was trying to fire event
> triggers, even though we don't support GRANT and REVOKE; it seems to me
> that the three things out to be supported together. I'm told David
> Fetter would call this "DCL support", and we're not supporting DCL at
> this stage. So DEFAULT PRIVILEGES ought to be not supported.

Agreed. Do you want me to edit my patch to remove support for that
command and the other arrangement we are talking about, or do you want
to continue having at it?

> I also observed that the SECURITY LABEL commands does not fire event
> triggers; I think it should. But then maybe this can be rightly called
> DCL as well, so perhaps it's all right to leave it out.

I would agree with calling that a DCL.

> DROP OWNED is not firing event triggers. There is a comment therein
> that says "we don't fire them for shared objects", but this is wrong:
> this command is dropping local objects, not shared, so it seems
> necessary to me that triggers are fired.

I think the best way to address DROP OWNED is the same as to address
DROP CASCADE: have the inner code (doDeletion, I think) prepare another
DropStmt and give control back to ProcessUtility() with a context of
PROCESS_UTILITY_CASCADE.

The good news is that the unified DropStmt support allows us to think
about that development, even if it still is a non trivial refactoring.

I would like to hear that we can consider the current patch first then
refactor the drop cascade operations so that we automatically have full
support for DROP OWNED.

> I also noticed that some cases such as DROP <whatever> do not report the
> OIDs of all the objects that are being dropped, only one of them. This
> seems bogus to me; if you do "DROP TABLE foo,bar" then both tables
> should be reported.

When we have the DROP CASCADE refactoring, we could maybe implement drop
on a list of objects as getting back to ProcessUtility() also, with yet
another context (SUBCOMMAND or maybe GENERATED would do here).

The other way to address the information problem would be to expose it
as a relation (resultset, setof record, cursor, you name it) variable to
PLpgSQL.

Again, I hope we can decide on the approach now, commit what we have and
expand on it on the next commit fest.

>> Given the OID, we use the ObjectProperty[] structure to know which cache
>> or catalog to scan in order to get the name and namespace of the object.
>
> The changes to make ObjectPropertyType visible to code outside
> objectaddress.c look bogus to me. I think we should make this new code
> use the interfaces we already have.

We need to move the new fonction get_object_name_and_namespace() into
the objectaddress.c "module" then, and decide about returning yet
another structure (because we have two values to return) or to pass that
function a couple of char **. Which devil do you prefer?

Regards,
--
Dimitri Fontaine
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support