Subject: Re: FOR EACH ROW triggers on partitioned tables
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
References: <20180123221027.2qenwwpvgplrrx3d@alvherre.pgsql>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>,
 Simon Riggs <simon@2ndquadrant.com>
From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Organization: 2ndQuadrant
Message-ID: <171cb95a-35ec-2ace-3add-a8d16279f0bf@2ndquadrant.com>
Date: Mon, 29 Jan 2018 15:30:12 -0500
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:52.0)
 Gecko/20100101 Thunderbird/52.5.2
MIME-Version: 1.0
In-Reply-To: <20180123221027.2qenwwpvgplrrx3d@alvherre.pgsql>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 1/23/18 17:10, Alvaro Herrera wrote:
> The main question is this: when running the trigger function, it is
> going to look as it is running in the context of the partition, not in
> the context of the parent partitioned table (TG_RELNAME etc).  That
> seems mildly ugly: some users may be expecting that the partitioning
> stuff is invisible to the rest of the system, so if you have triggers on
> a regular table and later on decide to partition that table, the
> behavior of triggers will change, which is maybe unexpected.  Maybe this
> is not really a problem, but I'm not sure and would like further
> opinions.

One could go either way on this, but I think reporting the actual table
partition is acceptable and preferable.  If you are writing a generic
trigger function, maybe to dump out all columns, you want to know the
physical table and its actual columns.  It's easy[citation needed] to
get the partition root for a given table, if the trigger code needs
that.  The other way around is not possible.

Some other comments are reading the patch:

It seems to generally follow the patterns of the partitioned indexes
patch, which is good.

I think WHEN clauses on partition triggers should be OK.  I don't see a
reason to disallow them.

Similarly, transition tables should be OK.  You only get the current
partition to look at, of course.

The function name CloneRowTriggersOnPartition() confused me.  A more
accurate phrasing might be CloneRowTriggersToPartition(), or maybe
reword altogether.

New CommandCounterIncrement() call in AttachPartitionEnsureIndexes()
should be explained.  Or maybe it belongs in ATExecAttachPartition()
between the calls to AttachPartitionEnsureIndexes() and
CloneRowTriggersOnPartition()?

Prohibition against constraint triggers is unclear.  The subsequent
foreign-key patches mess with that further.  It's not clear to me why
constraint triggers shouldn't be allowed like normal triggers.

Obvious missing things: documentation, pg_dump, psql updates

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