enable/disable broken for statement triggers on partitioned tables

From: Amit Langote <amitlangote09(at)gmail(dot)com>
To: PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Cc: Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>, Simon Riggs <simon(dot)riggs(at)enterprisedb(dot)com>
Subject: enable/disable broken for statement triggers on partitioned tables
Date: 2022-05-24 06:11:28
Message-ID: CA+HiwqG-cZT3XzGAnEgZQLoQbyfJApVwOTQaCaas1mhpf+4V5A@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

Simon reported $subject off-list.

For triggers on partitioned tables, various enable/disable trigger
variants recurse to also process partitions' triggers by way of
ATSimpleRecursion() done in the "prep" phase. While that way of
recursing is fine for row-level triggers which are cloned to
partitions, it isn't for statement-level triggers which are not
cloned, so you get an unexpected error as follows:

create table p (a int primary key) partition by list (a);
create table p1 partition of p for values in (1);
create function trigfun () returns trigger language plpgsql as $$
begin raise notice 'insert on p'; end; $$;
create trigger trig before insert on p for statement execute function trigfun();
alter table p disable trigger trig;
ERROR: trigger "trig" for table "p1" does not exist

The problem is that ATPrepCmd() is too soon to perform the recursion
in this case as it's not known at that stage if the trigger being
enabled/disabled is row-level or statement level, so it's better to
perform it during ATExecCmd(). Actually, that is how it used to be
done before bbb927b4db9b changed things to use ATSimpleRecursion() to
fix a related problem, which was that the ONLY specification was
ignored by the earlier implementation. The information of whether
ONLY is specified in a given command is only directly available in the
"prep" phase and must be remembered somehow if the recursion must be
handled in the "exec" phase. The way that's typically done that I see
in tablecmds.c is to have ATPrepCmd() change the AlterTableCmd.subtype
to a recursive variant of a given sub-command. For example,
AT_ValidateConstraint by AT_ValidateConstraintRecurse if ONLY is not
specified.

So, I think we should do something like the attached. A lot of
boilerplate is needed given that the various enable/disable trigger
variants are represented as separate sub-commands (AlterTableCmd
subtypes), which can perhaps be avoided by inventing a
EnableDisableTrigStmt sub-command node that stores (only?) the recurse
flag.

--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com

Attachment Content-Type Size
v1-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patch application/octet-stream 15.9 KB

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Nikolay Shaplov 2022-05-24 08:09:45 Re: Zstandard support for toast compression
Previous Message bucoo 2022-05-24 06:06:28 re: partition wise aggregate wrong rows cost