Subject: Re: BEFORE ROW triggers for partitioned tables
To: Alvaro Herrera <alvherre@2ndquadrant.com>,
 Pg Hackers <pgsql-hackers@lists.postgresql.org>
References: <20200227165158.GA2071@alvherre.pgsql>
From: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>
Organization: 2ndQuadrant
Message-ID: <2421844e-0798-5c2a-24b8-d061dafe2939@2ndquadrant.com>
Date: Tue, 10 Mar 2020 19:44:15 +0100
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:52.0)
 Gecko/20100101 Thunderbird/52.9.1
MIME-Version: 1.0
In-Reply-To: <20200227165158.GA2071@alvherre.pgsql>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2020-02-27 17:51, Alvaro Herrera wrote:
> Enabling BEFORE triggers FOR EACH ROW in partitioned tables is very easy
> -- just remove the check against them.  With that, they work fine.

This looks good to me in principle.  It's a useful thing to support.

> 1. Just let it be.  If the user does something silly, it's their problem
> if they get an ugly error message.
> 
> 2. If the partition keys are changed, raise an error.  The trigger is
> allowed to change everything but those columns.  Then there's no
> conflict, and it allows desirable use cases.
> 
> 3. Allow the partition keys to change, as long as the tuple ends up in
> the same partition.  This is the same as (1) except the error message is
> nicer.
> 
> The attached patch implements (2).

That seems alright to me.

> * The new function I added, ReportTriggerPartkeyChange(), contains one
> serious bug (namely: it doesn't map attribute numbers properly if
> partitions are differently defined).   Also, it has a performance issue,
> namely that we do heap_getattr() potentially repeatedly -- maybe it'd be
> better to "slotify" the tuple prior to doing the checks.

The attribute ordering issue obviously needs to be addressed, but the 
performance issue is probably not so important.  How many levels of 
partitioning are we expecting?

> Another
> possible controversial point is that its location in commands/trigger.c
> isn't great.  (Frankly, I don't understand why the executor trigger
> firing functions are in commands/ at all.)

yeah ...

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