Subject: Re: FOR EACH ROW triggers on partitioned tables
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 Thomas Munro <thomas.munro@enterprisedb.com>
References: <20180309204143.n4xwudjl5oawf55p@alvherre.pgsql>
Cc: Robert Haas <robertmhaas@gmail.com>,
 Pg Hackers <pgsql-hackers@postgresql.org>,
 Simon Riggs <simon@2ndquadrant.com>
From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Organization: 2ndQuadrant
Message-ID: <223101be-9f90-6965-87a5-a5d44dc66597@2ndquadrant.com>
Date: Mon, 12 Mar 2018 10:31:08 -0400
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0)
 Gecko/20100101 Thunderbird/52.6.0
MIME-Version: 1.0
In-Reply-To: <20180309204143.n4xwudjl5oawf55p@alvherre.pgsql>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 3/9/18 15:41, Alvaro Herrera wrote:
>> One thing I'd like to add before claiming this committable (backend-
>> side) is enabling constraint triggers.  AFAICT that requires a bit of
>> additional logic, but it shouldn't be too terrible.  This would allow
>> for deferrable unique constraints, for example.
> 
> v7 supports constraint triggers.  I added an example using a UNIQUE
> DEFERRABLE constraint, and another one using plain CREATE CONSTRAINT TRIGGER.
> It's neat to see that the WHEN clause is executed at the time of the
> operation, and the trigger action is deferred (or not) till COMMIT time.

I'm not sure why you have the CommandCounterIncrement() changes in
separate patches.

It looks like there are some test cases that are essentially duplicates,
e.g.,

+create trigger failed before insert or update or delete on parted_trig
+  for each row execute procedure trigger_nothing();

+create trigger trig_ins_before_1 before insert on parted_stmt_trig
+  for each row execute procedure trigger_notice();

Perhaps the latter is supposed to be testing statement triggers instead?

Some documentation updates are needed, at least in catalogs.sgml and
CREATE TRIGGER reference page.

The argument names of CreateTrigger() are slightly different in the .h
and .c files.

I'm wondering about deferrable unique constraint triggers.  In index.c,
the CreateTrigger() call doesn't pass any parent trigger OID.  How is
this meant to work?  I mean, it does work, it seems.  Some comments maybe.

In CloneRowTriggersToPartition(), for this piece

+               /*
+                * We only clone a) FOR EACH ROW triggers b) timed AFTER
events, c)
+                * that are not constraint triggers.
+                */
+               if (!TRIGGER_FOR_ROW(trigForm->tgtype) ||
+                       !TRIGGER_FOR_AFTER(trigForm->tgtype) ||
+                       OidIsValid(trigForm->tgconstraint))
+                       continue;

I would rather have some elog(ERROR)'s if it finds triggers it can't
support instead of silently skipping them.

What is the story with transition tables?  Why are they not supported?
I don't understand this comment in CreateTrigger():

+   /*
+    * Disallow use of transition tables.  If this partitioned table
+    * has any partitions, the error would occur below; but if it
+    * doesn't then we would only hit that code when the first CREATE
+    * TABLE ... PARTITION OF is executed, which is too late.  Check
+    * early to avoid the problem.
+    */

Earlier in the thread, others have indicated that transition tables
should work.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services