A row-level trigger on a partitioned table is not created on a sub-partition created later
Hello,
According to the documentation
<https://www.postgresql.org/docs/11/sql-createtrigger.html>, 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.
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. :(
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
Attachments:
dont-mark-cloned-triggers-internal.patchtext/plain; charset=US-ASCII; name=dont-mark-cloned-triggers-internal.patchDownload+10-15
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
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
Attachments:
dont-mark-cloned-triggers-internal_v2.patchtext/plain; charset=US-ASCII; name=dont-mark-cloned-triggers-internal_v2.patchDownload+42-21
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
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
Attachments:
trigtest.patchtext/x-diff; charset=us-asciiDownload+27-11
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
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