Re: Command Triggers, patch v11 - Mailing list pgsql-hackers

On 9 March 2012 21:38, Dimitri Fontaine <dimitri@2ndquadrant.fr> wrote:
> Hi,
>
> Please find attached v15 of the patch, addressing all known issues apart
> from the trigger function argument passing style. Expect a new patch
> with that taken care of early next week.
>
>  (The github branch too, should you be using that)
>
> Thom Brown <thom@linux.com> writes:
>> 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.
>
> Ah yes that needed a special case, it's properly handled now, and
> tested.

Confirmed.

>>>> When dropping domains, the name of the domain includes the schema name:
>
> Fixed.

Confirmed.

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

Confirmed, thanks.

>>>> REINDEX on a table seems to show no schema name but an object name for
>>>> specific triggers:
>
> Was a typo, fixed.

Confirmed

>>>> When REINDEXing an index rather than a table, the table's details are
>>>> shown in the trigger.  Is this expected?:
>
> Fixed.

Confirmed.

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

Confirmed that it's removed from the code.  Just needs removing from
the docs too now. (doc/src/sgml/ref/create_command_trigger.sgml)

>> All these corrections I mentioned previously still needs to be made:
>
> That's about the docs, I edited them accordingly to your comments.

Confirmed.

>>> Thom Brown <thom@linux.com> writes:
>> 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?
>
> The others trigger don't fire because an ERROR case is detected before
> they have a chance to run, much like on a primary in some ERROR cases.

Yes, I've since discovered that I shouldn't have been treating them
equally due to the different type of error those sets of commands
generate.  That's fine then.

> Thom Brown <thom@linux.com> writes:
>> It was late last night and I forgot to get around to testing pg_dump,
>> which isn't working correctly:
>
> Fixed.

Confirmed.

>> Also I notice that CREATE/ALTER/DROP COMMAND TRIGGER appears just
>> before CREATE/ALTER/DROP TRIGGER in the documentation.  This breaks
>> the alphabetical order and I wasn't expecting to find it there when
>> scanning down the page.  Could we move them into an alphabetic
>> position?
>
> I don't see that problem in the source files, could you be more specific?

I can't see the problem in the source either, but when viewing
postgresql/doc/src/sgml/html/reference.html in my browser, CREATE
COMMAND TRIGGER appears between CREATE TRIGGER and CREATE TYPE.  Not
sure why though.

Unless you're cleverly distracted me away from some issue that's yet
to be resolved, that appears to be nearly all my concerns addressed.
All that appears to remain is the trigger-variable stuff which you
said shall arrive early next week, and also the that odd doc issue I
mentioned in the paragraph above (although that could just be
something weird happening when I build it).  Sounds like I have the
weekend off. :)

Thanks Dimitri.

--
Thom