Thread: A row-level trigger on a partitioned table is not created on asub-partition created later
A row-level trigger on a partitioned table is not created on asub-partition created later
From
Petr Fedorov
Date:
Hello,
According to the documentation, creating a row-level trigger on a partitioned table will cause identical triggers to be created in all its existing partitions; and any partitions created or attached later will contain an identical trigger, too.
It does not happen for a sub-partition - partition of partition - when it is created/attached later. Steps to reproduce (11.6, Centos 7):
create table level1 (id1 integer not null, id2 integer not null, id3 integer not null) partition by list (id2);
create table level2 partition of level1 for values in (1) partition by list (id3);
create table level3 partition of level2 for values in (1);
create or replace function trigger_func() returns trigger language 'plpgsql' as $body$ begin raise exception 'fired'; return null; end $body$;
create trigger test_trigger after insert on level1 for each row execute procedure trigger_func();
insert into level1 values (1,1,1); -- fails as expected due to test_trigger();
drop table level1;
drop function trigger_func();
create table level1 (id1 integer not null, id2 integer not null, id3 integer not null) partition by list (id2);
create or replace function trigger_func() returns trigger language 'plpgsql' as $body$ begin raise exception 'fired'; return null; end $body$;
create trigger test_trigger after insert on level1 for each row execute procedure trigger_func();
create table level2 partition of level1 for values in (1) partition by list (id3);
create table level3 partition of level2 for values in (1);
insert into level1 values (1,1,1); -- row inserted
psql \d+ shows that level3 does not have row level trigger while level2 and level1 have.
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Amit Langote
Date:
Hello, On Thu, Dec 5, 2019 at 4:24 PM Petr Fedorov <petr.fedorov@phystech.edu> wrote: > create table level1 (id1 integer not null, id2 integer not null, id3 integer not null) partition by list (id2); > create or replace function trigger_func() returns trigger language 'plpgsql' as $body$ begin raise exception 'fired'; returnnull; end $body$; > create trigger test_trigger after insert on level1 for each row execute procedure trigger_func(); > > create table level2 partition of level1 for values in (1) partition by list (id3); > create table level3 partition of level2 for values in (1); > > insert into level1 values (1,1,1); -- row inserted > > psql \d+ shows that level3 does not have row level trigger while level2 and level1 have. That is a bug. :( Alvaro, isn't marking triggers cloned to partitions "internal" unnecessary? Because the cloned trigger on partition (level2 in above example) is marked internal, CloneRowTriggersToPartition() skips it when called on a sub-partition (level3 in above example). Attached patch to fix that passes make check, although a bit surprised that it does. Thanks, Amit
Attachment
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Alvaro Herrera
Date:
On 2019-Dec-18, Amit Langote wrote: > Hello, > > On Thu, Dec 5, 2019 at 4:24 PM Petr Fedorov <petr.fedorov@phystech.edu> wrote: > > create table level1 (id1 integer not null, id2 integer not null, id3 integer not null) partition by list (id2); > > create or replace function trigger_func() returns trigger language 'plpgsql' as $body$ begin raise exception 'fired';return null; end $body$; > > create trigger test_trigger after insert on level1 for each row execute procedure trigger_func(); > > > > create table level2 partition of level1 for values in (1) partition by list (id3); > > create table level3 partition of level2 for values in (1); > > > > insert into level1 values (1,1,1); -- row inserted > > > > psql \d+ shows that level3 does not have row level trigger while level2 and level1 have. > > That is a bug. :( Ouch. > Alvaro, isn't marking triggers cloned to partitions "internal" > unnecessary? Because the cloned trigger on partition (level2 in above > example) is marked internal, CloneRowTriggersToPartition() skips it > when called on a sub-partition (level3 in above example). > > Attached patch to fix that passes make check, although a bit surprised > that it does. IIRC that change would break pg_dump. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Amit Langote
Date:
On Wed, Dec 18, 2019 at 6:56 PM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Dec-18, Amit Langote wrote: > > Alvaro, isn't marking triggers cloned to partitions "internal" > > unnecessary? Because the cloned trigger on partition (level2 in above > > example) is marked internal, CloneRowTriggersToPartition() skips it > > when called on a sub-partition (level3 in above example). > > > > Attached patch to fix that passes make check, although a bit surprised > > that it does. > > IIRC that change would break pg_dump. Indeed it does, but looks like partition triggers are not tested that extensively in pg_dump's suite. Attached updated patch with pg_dump hacks seems to do the trick for me. What do you think? Thanks, Amit
Attachment
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Alvaro Herrera
Date:
On 2019-Dec-26, Amit Langote wrote: > > Indeed it does, but looks like partition triggers are not tested that > extensively in pg_dump's suite. Hmm, you're right. I added a test to the three branches that would be broken by your first patch. I hope the buildfarm likes it. Working on getting your fix pushed now. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Alvaro Herrera
Date:
On 2019-Dec-27, Alvaro Herrera wrote: > Working on getting your fix pushed now. Ran out of time for today, but I added this test that reproduces the issue -- fails without your patch, and works with it. One thing I just realized is that in next minors' release notes we should publish a recipe to fix catalogs for existing databases, unless we go for a fix that doesn't require changing the catalogs -- I don't know what that would be, though, but maybe it would not be totally insane to clone even internal triggers in the cases that matter. (I wonder if a better solution for the master branch would involve a new pg_trigger column that indicates the parent trigger for a cloned trigger. That would avoid pg_dump's new subquery. Also: would it be better to use pg_partition_ancestors instead of the sub-subquery? ... though that doesn't work in pg11 ...) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Alvaro Herrera
Date:
On 2019-Dec-27, Alvaro Herrera wrote: > One thing I just realized is that in next minors' release notes we > should publish a recipe to fix catalogs for existing databases, unless > we go for a fix that doesn't require changing the catalogs -- I don't > know what that would be, though, but maybe it would not be totally > insane to clone even internal triggers in the cases that matter. So, the more I thought about this, the more it seemed that marking those triggers as not internal was the wrong thing to do. So I instead made it clone the internal triggers that seem to matter. This fixes the originally reported problem, and passes the test case I propose. Also, pg_dump continues to work unchanged, and existing database don't need tweaking (excepting those that might already be missing triggers). It would be better to fix master afterwards, by adding a pg_trigger.trgparentid column, which seems like it would be a better answer going forward. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
Re: A row-level trigger on a partitioned table is not created on asub-partition created later
From
Amit Langote
Date:
Sorry, away from work past few days. On Wed, Jan 1, 2020 at 4:48 AM Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > On 2019-Dec-27, Alvaro Herrera wrote: > > One thing I just realized is that in next minors' release notes we > > should publish a recipe to fix catalogs for existing databases, unless > > we go for a fix that doesn't require changing the catalogs -- I don't > > know what that would be, though, but maybe it would not be totally > > insane to clone even internal triggers in the cases that matter. On second thought, I agree that not having to have to fix catalog state for existing users would be good. > So, the more I thought about this, the more it seemed that marking those > triggers as not internal was the wrong thing to do. So I instead made > it clone the internal triggers that seem to matter. This fixes the > originally reported problem, and passes the test case I propose. Also,, away > pg_dump continues to work unchanged, and existing database don't need > tweaking (excepting those that might already be missing triggers). 0002 seems fine as a solution although some comments: +/* + * doesTriggerDependOnAnotherTrigger + * Checks pg_depend for what the name says + * + * This is an ugly hack to cope with a catalog deficiency. + * Keep away from children. Do not stare with naked eyes. Do not propagate. + */ +static bool +doesTriggerDependOnAnotherTrigger(Oid trigger_oid) Instead of making this too generic sounding, would it be better to name this to make clear what hack this is part of, say, isPartitionTrigger? > It would be better to fix master afterwards, by adding a > pg_trigger.trgparentid column, which seems like it would be a better > answer going forward. Agreed. Thanks, Amit