Hi, On Fri, Jul 8, 2022 at 3:44 AM Dmitry Koval <d.ko...@postgrespro.ru> wrote: > I've looked through the code and everything looks good. > But there is one thing I doubt. > Patch changes result of test: > ---- > > create function trig_nothing() returns trigger language plpgsql > as $$ begin return null; end $$; > create table parent (a int) partition by list (a); > create table child1 partition of parent for values in (1); > > create trigger tg after insert on parent > for each row execute procedure trig_nothing(); > select tgrelid::regclass, tgname, tgenabled from pg_trigger > where tgrelid in ('parent'::regclass, 'child1'::regclass) > order by tgrelid::regclass::text; > alter table only parent enable always trigger tg; -- no recursion > select tgrelid::regclass, tgname, tgenabled from pg_trigger > where tgrelid in ('parent'::regclass, 'child1'::regclass) > order by tgrelid::regclass::text; > alter table parent enable always trigger tg; -- recursion > select tgrelid::regclass, tgname, tgenabled from pg_trigger > where tgrelid in ('parent'::regclass, 'child1'::regclass) > order by tgrelid::regclass::text; > > drop table parent, child1; > drop function trig_nothing(); > > ---- > Results of vanilla + patch: > ---- > CREATE FUNCTION > CREATE TABLE > CREATE TABLE > CREATE TRIGGER > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | O > parent | tg | O > (2 rows) > > ALTER TABLE > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | O > parent | tg | A > (2 rows) > > ALTER TABLE > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | O > parent | tg | A > (2 rows) > > DROP TABLE > DROP FUNCTION > > ---- > Results of vanilla: > ---- > CREATE FUNCTION > CREATE TABLE > CREATE TABLE > CREATE TRIGGER > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | O > parent | tg | O > (2 rows) > > ALTER TABLE > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | O > parent | tg | A > (2 rows) > > ALTER TABLE > tgrelid | tgname | tgenabled > ---------+--------+----------- > child1 | tg | A > parent | tg | A > (2 rows) > > DROP TABLE > DROP FUNCTION > ---- > The patch doesn't start recursion in case 'tgenabled' flag of parent > table is not changes. > Probably vanilla result is more correct.
Thanks for the review and this test case. I agree that the patch shouldn't have changed that behavior, so I've fixed the patch so that EnableDisableTrigger() recurses even if the parent trigger is unchanged. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
v2-0001-Fix-ENABLE-DISABLE-TRIGGER-to-handle-recursion-co.patch
Description: Binary data