Обсуждение: BUG #16794: BEFORE UPDATE FOR EACH ROW triggers on partitioned tables can break tuple moving UPDATEs

The following bug has been logged on the website:

Bug reference:      16794
Logged by:          Philipp Menke
Email address:      pg@pmenke.de
PostgreSQL version: 13.1
Operating system:   Linux
Description:

Hi there,

i was testing the PG13 enhancement that should allow BEFORE ROW triggers on
partitioned tables, as long as they don't move the tuple to a different
partition (original thread:
https://postgr.es/m/20200227165158.GA2071@alvherre.pgsql). The actual
restriction on "not to move the tuple to a different partition" seems to be
a bit stronger though, as the trigger fails, even though not itself, but the
overarching UPDATE command, did move the tuple. Maybe this is best shown by
an example:

```
CREATE TABLE parted (
    part_key INT,
    changed_at TIMESTAMPTZ DEFAULT now()
) PARTITION BY RANGE(part_key);

CREATE TABLE parted_p0_9 PARTITION OF parted FOR VALUES FROM (0) TO (9);
CREATE TABLE parted_p10_19 PARTITION OF parted FOR VALUES FROM (10) TO
(19);

CREATE FUNCTION parted_audit_trig() RETURNS TRIGGER LANGUAGE plpgsql AS $$
BEGIN
    NEW.changed_at = now();
    RETURN NEW;
END;
$$;

CREATE TRIGGER a01_audit_trig BEFORE UPDATE ON parted FOR EACH ROW EXECUTE
PROCEDURE parted_audit_trig();

INSERT INTO parted(part_key) VALUES (1);

UPDATE parted SET part_key = 11 WHERE part_key = 1;
```

The final UPDATE statement fails with:
```
[0A000] ERROR: moving row to another partition during a BEFORE trigger is
not supported
Detail: Before executing trigger "a01_audit_trig", the row was to be in
partition "public.parted_p0_9".
```

At least according to the documentation
(https://www.postgresql.org/docs/13/ddl-partitioning.html 5.11.2.3.
Limitations) i would have expected that the UPDATE succeeds and moves the
tuple to parted_p10_19.

Interestingly the error seems to only occur if the trigger function actually
assigns a value to any field in NEW - even if it is the same value (as in
`NEW.changed_at = NEW.changed_at;`). If the trigger function does nothing /
performs checks etc. but doesn't assign any field in NEW, the statement
completes successfully.

Thanks and Kind Regards,
Philipp

On 2020-Dec-28, PG Bug reporting form wrote:

> i was testing the PG13 enhancement that should allow BEFORE ROW triggers on
> partitioned tables, as long as they don't move the tuple to a different
> partition (original thread:
> https://postgr.es/m/20200227165158.GA2071@alvherre.pgsql). The actual
> restriction on "not to move the tuple to a different partition" seems to be
> a bit stronger though, as the trigger fails, even though not itself, but the
> overarching UPDATE command, did move the tuple.

Hi Philipp, thanks for reporting this issue.  Yeah, that should
definitely work.  I'll have a look at this in a couple of days and try
my best to have a fix for the February minors.

Regards

-- 
Álvaro Herrera       Valdivia, Chile

On 2021-Jan-23, Alvaro Herrera wrote:

> On 2020-Dec-28, PG Bug reporting form wrote:
> 
> > i was testing the PG13 enhancement that should allow BEFORE ROW triggers on
> > partitioned tables, as long as they don't move the tuple to a different
> > partition (original thread:
> > https://postgr.es/m/20200227165158.GA2071@alvherre.pgsql). The actual
> > restriction on "not to move the tuple to a different partition" seems to be
> > a bit stronger though, as the trigger fails, even though not itself, but the
> > overarching UPDATE command, did move the tuple.
> 
> Hi Philipp, thanks for reporting this issue.  Yeah, that should
> definitely work.  I'll have a look at this in a couple of days and try
> my best to have a fix for the February minors.

Looked at this today, and it's not nearly as easy to fix as I hoped.
The misbehavior in corner cases seems weird enough that we should keep
the restriction ... but the way the restriction is implemented currently
is completely broken.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Si no sabes adonde vas, es muy probable que acabes en otra parte.

On 2021-Jan-26, Alvaro Herrera wrote:

> Looked at this today, and it's not nearly as easy to fix as I hoped.
> The misbehavior in corner cases seems weird enough that we should keep
> the restriction ... but the way the restriction is implemented currently
> is completely broken.

Actually, I was misled by the fact that I had both INSERT triggers and
UPDATE triggers, and they were both changing the partition keys.  What I
now think we should do is just remove the restrictions for UPDATE, and
let the trigger do whatever; and keep what we have for INSERT, because
it is useful.  Things work out okay.  As in the attached patch.  A quick
write-up:

In the UPDATE case, this is the timeline:

u0. The user-specified changes are carried out in the tuple.
u1. BEFORE UPDATE FOR EACH ROW triggers run, *in the original partition*.
    We don't know yet if the tuple is okay in this partition or not.
u2. The partition constraint (of the original partition) is tested.
u3. If the partition constraint returns OK, we're done.
... since the partition constraint failed, we have to route the tuple:
u4. a DELETE is executed in the original partition. Appropriate triggers run.
u5. ExecFindPartition determines the target partition
u6. an INSERT is executed in the new partition. Appropriate triggers run.

The originally claimed reason not to allow the trigger from moving the
row to another partition was that if we allowed that, then we would not
run the correct triggers. But that's wrong: because the UPDATE at step
u1 runs triggers in the original partition, not in the new one (which
would be determined only at u5) then that holds true for the original
update too, not just the changes in the before-row triggers.  So if the
triggers change the partkey columns ... it doesn't have any semantic
difference from the user changing the partkey columns.  We don't care.


As for INSERT -- it does not know how to handle rerouting.  So we're
only producing a better-quality error message by having the
same-partition check.  If we did not have the check, the only difference
is that ExecPartitionCheck would fail a bit later, with a mysterious
error that the tuple doesn't meet the partition constraint.

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
Tom: There seems to be something broken here.
Teodor: I'm in sackcloth and ashes...  Fixed.
        http://archives.postgresql.org/message-id/482D1632.8010507@sigaev.ru

I added some more tests and pushed.

Thanks for reporting!

-- 
Álvaro Herrera                            39°49'30"S 73°17'W
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)