Re: Command Triggers, patch v11

On 8 March 2012 22:24, Dimitri Fontaine <dimitri(at)2ndquadrant(dot)fr> wrote:

We're getting there. :)

> Hi,
>
> Thom Brown <thom(at)linux(dot)com> writes:
>> The message returned by creating a command trigger after create index
>> is still problematic:
>
> Fixed.  I'm attaching an incremental patch here, the github branch is
> updated too.

Confirmed.

>> CREATE VIEW doesn't return schema:
>
> Fixed, and as an added bonus I fixed the CREATE SEQUENCE oddity about
> that too.

Yes, working now.

>> No specific triggers fire when altering a conversion:
>
> Couldn't reproduce, works here, added tests.

My apologies. It seems I neglected to set up a specific trigger for
it. It does indeed work.

>> No specific triggers fire when altering the properties of a function:
>
> Fixed.

Yes, tried every property available and working in every case now.

>> No specific triggers fire when altering a sequence:
>
> Couldn't reproduce, added tests.

Again, I wrongly assumed I had set up a command trigger for this. I
think it's because I based my specific triggers on what was listed on
the CREATE COMMAND TRIGGER documentation page, and those were only
recently added. This is working fine.

>> No specific triggers when altering a view:
>
> Same again.

Working correctly. (see above)

>> The object name shown in specific triggers when dropping aggregates
>> shows the schema name:
>> Same for collations:
>> Dropping functions shows the object name as the schema name:
>> Same with dropping operators:
>> ...and operator family:
>> … and the same for dropping text search
>> configuration/dictionary/parser/template.
>
> Fixed in the attached (all of those where located at exactly the same
> place, by the way, that's one fix).

CREATE OPERATOR CLASS now shows objectname as 'hash' instead of its
name where it's declared as "USING hash". This isn't a problem with
ALTER/DROP OPERATOR CLASS. Everything else above works as expected
now.

>> When dropping domains, the name of the domain includes the schema name:
>
> I'm using format_type_be(objectId) so that int4 is integer and int8
> bigint etc, but that's adding the schemaname. I didn't have time to look
> for another API that wouldn't add the schemaname nor to add one myself,
> will do that soon.

Okay, skipping test.

>> When creating a trigger on REINDEX, I get the following message:
>
> Fixed.

Could we change this to "REINDEX DATABASE triggers are not supported"?
This way it would be consistent with the "AFTER CREATE INDEX
CONCURRENTLY" warning.

>> VACUUM doesn't fire a specific command trigger:
>
> I though it was better this way, I changed my mind and completed the code.

Yes, working now.

>> REINDEX on a table seems to show no schema name but an object name for
>> specific triggers:
>
> Still on the TODO.

Skipped.

>> When REINDEXing an index rather than a table, the table's details are
>> shown in the trigger.  Is this expected?:
>
> Yeah well.  Will see about how much damage needs to be done in the
> current APIs, running out of steam for tonight's batch.

Skipped.

>> Documentation:
>
> Fixed.

ALTER CAST is still listed and needs removing, not just from the
documentation but every place it's used your code too. I can
currently create a trigger for it, but it's impossible for it to fire
since there's no such command.

All these corrections I mentioned previously still needs to be made:

“A command trigger's function must return void, the only it can aborts
the execution of the command is by raising an exception.”
should be:
“A command trigger's function must return void. It can then only
abort the execution of the command by raising an exception.”

Remove:
“For a constraint trigger, this is also the name to use when modifying
the trigger's behavior using SET CONSTRAINTS.”

“that's the case for VACUUM, CLUSTER CREATE INDEX CONCURRENTLY, and
REINDEX DATABASE.”
should be:
“that's the case for VACUUM, CLUSTER, CREATE INDEX CONCURRENTLY, and
REINDEX DATABASE.”

> Thom Brown <thom(at)linux(dot)com> writes:
>> I've also since found that if I issue a VACUUM, CLUSTER or REINDEX on
>> a read-only standby, the BEFORE ANY COMMAND trigger fires.  I don't
>> think any trigger should fire on a read-only standby.
>
> Well I'm not sold on that myself (think pl/untrusted that would reach
> out to the OS and do whatever is needed there).  You can even set the
> session_replication_role GUC to replica and only have the replica
> command triggers fired.

All other command triggers don't fire on read-only standbys, and the
inconsistency doesn't seem right. On the one hand all
CREATE/DROP/ALTER triggers aren't fired because of the "cannot execute
<command> in a read-only transaction" error message, but triggers do
occur before utility commands, which would otherwise display the same
message, and might not display it at all if the trigger causes an
error in its function call. So it seems like they should either all
fire, or none of them should. What are you thoughts?

--
Thom