Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jul 5, 2018 at 6:44 AM, Robert Haas wrote: > > Well, as far as I know, it's up to me which parts of your emails I want to > quote in my reply. I did read this part. It did not change my opinion. My > fundamental objection to your proposal is that I think it is too wordy. I > think users will generally know whether they are using partitioning or > inheritance, and if they don't it's not hard to figure out. I don't think > quoting only part of what you wrote made the quote misleading, but it did > allow me to express my opinion. I would usually believe that people have read complete email before replying. But when the reply raises a question without quoting a portion of my mail which I think has the answer, I am confused. There's no straight forward method to avoid that confusion given it's written and turn based communication. But I try to get clarity on that confusion. > I understand that you don't agree, which is > fine, but I stand by my position. > I am fine with disagreement, now that I know why there's disagreement. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, Jul 3, 2018 at 12:41 AM Ashutosh Bapat < ashutosh.ba...@enterprisedb.com> wrote: > > rhaas=# drop table foo; > > ERROR: table "foo" does not exist > > HINT: Try dropping a table with a different name that does exist, or > > first create this table before trying to drop it. > > Again a wrong example and wrong comparison. I think I was clear about > the problem when I wrote I don't think this is a question of "right" vs. "wrong". You are certainly entitled to your opinion, but I believe that I am entitled to mine, too. -- > When there was only a single way, i.e table > inheritance, to "inherit" things users could probably guess that. But > now there are multiple ways to inherit things, we have to help user a > bit more. The user might figure out that ti's a partition of a table, > so the constraint is inherited from the partitioned table. But it will > help if we give a hint about from where the constraint was inherited > like the error message itself reads like "can not drop constraint > "p_a_check" on relation "p1" inherited from "partitioned table's name" > OR a hint "you may drop constraint "p_a_check" on the partitioned > table "partitioned table's name". > -- > > For some reason you have chosen to remove this from the email and > commented on previous part of it. Well, as far as I know, it's up to me which parts of your emails I want to quote in my reply. I did read this part. It did not change my opinion. My fundamental objection to your proposal is that I think it is too wordy. I think users will generally know whether they are using partitioning or inheritance, and if they don't it's not hard to figure out. I don't think quoting only part of what you wrote made the quote misleading, but it did allow me to express my opinion. I understand that you don't agree, which is fine, but I stand by my position. ...Robert -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, Jul 3, 2018 at 8:19 AM, Robert Haas wrote: > On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat > wrote: >> This constraint was added to the partitioned table and inherited from >> there. If user wants to drop that constraint for some reason, this >> error message doesn't help. The error message tells why he can't drop >> it, but doesn't tell, directly or indirectly, the user what he should >> do in order to drop it. > > That doesn't really sound like an actual problem to me. If the error > is that the constraint is inherited, that suggests that the solution > is to find the place from which it got inherited and drop it there. > And that's in fact what you have to do. What's the problem? I mean, > we could add a hint, but it's possible to make yourself sound dumb by > giving hints that are basically obvious implications from the error > message. Nobody wants this sort of thing: > > rhaas=# drop table foo; > ERROR: table "foo" does not exist > HINT: Try dropping a table with a different name that does exist, or > first create this table before trying to drop it. Again a wrong example and wrong comparison. I think I was clear about the problem when I wrote -- When there was only a single way, i.e table inheritance, to "inherit" things users could probably guess that. But now there are multiple ways to inherit things, we have to help user a bit more. The user might figure out that ti's a partition of a table, so the constraint is inherited from the partitioned table. But it will help if we give a hint about from where the constraint was inherited like the error message itself reads like "can not drop constraint "p_a_check" on relation "p1" inherited from "partitioned table's name" OR a hint "you may drop constraint "p_a_check" on the partitioned table "partitioned table's name". -- For some reason you have chosen to remove this from the email and commented on previous part of it. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/07/03 11:49, Robert Haas wrote: > On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat > wrote: >> This constraint was added to the partitioned table and inherited from >> there. If user wants to drop that constraint for some reason, this >> error message doesn't help. The error message tells why he can't drop >> it, but doesn't tell, directly or indirectly, the user what he should >> do in order to drop it. > > That doesn't really sound like an actual problem to me. If the error > is that the constraint is inherited, that suggests that the solution > is to find the place from which it got inherited and drop it there. > And that's in fact what you have to do. What's the problem? I mean, > we could add a hint, but it's possible to make yourself sound dumb by > giving hints that are basically obvious implications from the error > message. Nobody wants this sort of thing: > > rhaas=# drop table foo; > ERROR: table "foo" does not exist > HINT: Try dropping a table with a different name that does exist, or > first create this table before trying to drop it. > > A hint here wouldn't be as silly as that, but I think it is > unnecessary. I doubt there's likely to be much confusion here. I have to agree with that. "cannot drop inherited ..." conveys enough for a user to find out more. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat wrote: > This constraint was added to the partitioned table and inherited from > there. If user wants to drop that constraint for some reason, this > error message doesn't help. The error message tells why he can't drop > it, but doesn't tell, directly or indirectly, the user what he should > do in order to drop it. That doesn't really sound like an actual problem to me. If the error is that the constraint is inherited, that suggests that the solution is to find the place from which it got inherited and drop it there. And that's in fact what you have to do. What's the problem? I mean, we could add a hint, but it's possible to make yourself sound dumb by giving hints that are basically obvious implications from the error message. Nobody wants this sort of thing: rhaas=# drop table foo; ERROR: table "foo" does not exist HINT: Try dropping a table with a different name that does exist, or first create this table before trying to drop it. A hint here wouldn't be as silly as that, but I think it is unnecessary. I doubt there's likely to be much confusion here. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/07/02 14:46, Ashutosh Bapat wrote: > This constraint was added to the partitioned table and inherited from > there. If user wants to drop that constraint for some reason, this > error message doesn't help. The error message tells why he can't drop > it, but doesn't tell, directly or indirectly, the user what he should > do in order to drop it. When there was only a single way, i.e table > inheritance, to "inherit" things users could probably guess that. But > now there are multiple ways to inherit things, we have to help user a > bit more. The user might figure out that ti's a partition of a table, > so the constraint is inherited from the partitioned table. But it will > help if we give a hint about from where the constraint was inherited > like the error message itself reads like "can not drop constraint > "p_a_check" on relation "p1" inherited from "partitioned table's name" > OR a hint "you may drop constraint "p_a_check" on the partitioned > table "partitioned table's name". It might even suffice to say > "partition p1" instead "relation p1" so that the user gets a clue. It might be a good idea to mention if the table is a partition in the HINT message. ERROR message can remain the same. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Fri, Jun 29, 2018 at 6:35 PM, Tomas Vondra wrote: > > > On 06/29/2018 02:30 PM, Robert Haas wrote: >> >> On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote >> wrote: >>> >>> By the way, picking up on the word "inherited" in the error message shown >>> above, I wonder if you decided against using similar terminology >>> intentionally. >> >> >> Good question. >> >>> I guess the thinking there is that the terminology being >>> used extensively with columns and constraints ("inherited column/check >>> constraint cannot be dropped", etc.) is just a legacy of partitioning >>> sharing implementation with inheritance. >> >> >> It seems to me that we can talk about things being inherited by >> partitions even if we're calling the feature partitioning, rather than >> inheritance. Maybe that's confusing, but then again, maybe it's not >> that confusing. >> > > my 2c: I don't think it's confusing. Inheritance is a generic concept, not > attached exclusively to the "table inheritance". If you look at docs from > other databases, you'll see "partition inherits" all the time. I generally agree with this. But I think we need to think more in the context of the particular example Amit gave. -- quoting Amit's example, Table "public.p1" Column | Type | Collation | Nullable | Default +-+---+--+- a | integer | | | b | integer | | | Partition of: p FOR VALUES IN (1) Check constraints: "p1_b_check" CHECK (b >= 0) "p_a_check" CHECK (a >= 0) alter table p1 drop constraint p_a_check; ERROR: cannot drop inherited constraint "p_a_check" of relation "p1" --unquote This constraint was added to the partitioned table and inherited from there. If user wants to drop that constraint for some reason, this error message doesn't help. The error message tells why he can't drop it, but doesn't tell, directly or indirectly, the user what he should do in order to drop it. When there was only a single way, i.e table inheritance, to "inherit" things users could probably guess that. But now there are multiple ways to inherit things, we have to help user a bit more. The user might figure out that ti's a partition of a table, so the constraint is inherited from the partitioned table. But it will help if we give a hint about from where the constraint was inherited like the error message itself reads like "can not drop constraint "p_a_check" on relation "p1" inherited from "partitioned table's name" OR a hint "you may drop constraint "p_a_check" on the partitioned table "partitioned table's name". It might even suffice to say "partition p1" instead "relation p1" so that the user gets a clue. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 06/29/2018 02:30 PM, Robert Haas wrote: On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote wrote: By the way, picking up on the word "inherited" in the error message shown above, I wonder if you decided against using similar terminology intentionally. Good question. I guess the thinking there is that the terminology being used extensively with columns and constraints ("inherited column/check constraint cannot be dropped", etc.) is just a legacy of partitioning sharing implementation with inheritance. It seems to me that we can talk about things being inherited by partitions even if we're calling the feature partitioning, rather than inheritance. Maybe that's confusing, but then again, maybe it's not that confusing. my 2c: I don't think it's confusing. Inheritance is a generic concept, not attached exclusively to the "table inheritance". If you look at docs from other databases, you'll see "partition inherits" all the time. regards -- Tomas Vondra http://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote wrote: > By the way, picking up on the word "inherited" in the error message shown > above, I wonder if you decided against using similar terminology > intentionally. Good question. > I guess the thinking there is that the terminology being > used extensively with columns and constraints ("inherited column/check > constraint cannot be dropped", etc.) is just a legacy of partitioning > sharing implementation with inheritance. It seems to me that we can talk about things being inherited by partitions even if we're calling the feature partitioning, rather than inheritance. Maybe that's confusing, but then again, maybe it's not that confusing. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/28 7:58, Alvaro Herrera wrote: > On 2018-Jun-18, Robert Treat wrote: >> So +1 for thinking we do need to worry about it. I'm not exactly sure >> how we want to expose that info; with \d+ we list various "Partition >> X:" sections, perhaps adding one for "Partition triggers:" would be >> enough, although I am inclined to think it needs exposure at the \d >> level. One other thing to consider is firing order of said triggers... >> if all parent level triggers fire before child level triggers then the >> above separation is straightforward, but if the execution order is, as >> I suspect, mixed, then it becomes more complicated. > > The firing order is alphabetical across the whole set, i.e. parent/child > triggers are interleaved. See the regression test output at the bottom > of this email. > > I looked into adding a "Partition triggers" section because it seemed a > nice idea, but looking at the code I realized it's a bit tough because > we already split triggers in "categories": normal triggers, disabled > triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c > line 2795). Since the trigger being in a partition is orthogonal to > that classification, we would end up with eight groups. Not sure that's > great. > > The attached patch (catversion bump not included -- beware of server > crash if you run it without initdb'ing) keeps the categories the same. Thanks for creating the patch. > So with my previous example setup, you can see the duplicate triggers in > psql: > > alvherre=# \d child >Table "public.child" > Column │ Type │ Collation │ Nullable │ Default > ┼─┼───┼──┼─ > a │ integer │ │ │ > Partition of: parent FOR VALUES FROM (0) TO (100) > Triggers: > trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() > trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() > > and as soon as you try to drop the second one, you'll learn the truth > about it: > > alvherre=# drop trigger trig_p on child; > ERROR: cannot drop trigger trig_p on table child because trigger trig_p on > table parent requires it > SUGERENCIA: You can drop trigger trig_p on table parent instead. > Time: 1,443 ms > > I say this is sufficient. Yeah. We do same with constraints for example: create table p (a int check (a >= 0), b int) partition by list (a); create table p1 partition of p (b check (b >= 0)) for values in (1); \d p1 Table "public.p1" Column | Type | Collation | Nullable | Default +-+---+--+- a | integer | | | b | integer | | | Partition of: p FOR VALUES IN (1) Check constraints: "p1_b_check" CHECK (b >= 0) "p_a_check" CHECK (a >= 0) alter table p1 drop constraint p_a_check; ERROR: cannot drop inherited constraint "p_a_check" of relation "p1" More specifically, inherited triggers are not listed separately. By the way, picking up on the word "inherited" in the error message shown above, I wonder if you decided against using similar terminology intentionally. I guess the thinking there is that the terminology being used extensively with columns and constraints ("inherited column/check constraint cannot be dropped", etc.) is just a legacy of partitioning sharing implementation with inheritance. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-18, Robert Treat wrote: > One of the things I was thinking about while reading this thread is > that the scenario of creating "duplicate" triggers on a table meaning > two triggers doing the same thing is entirely possible now but we > don't really do anything to prevent it, which is Ok. I've never seen > much merit in the argument "people should test" (it assumes a certain > infallibility that just isn't true) but we've also generally been > pretty good about exposing what is going on so people can debug this > type of unexpected behavior. > > So +1 for thinking we do need to worry about it. I'm not exactly sure > how we want to expose that info; with \d+ we list various "Partition > X:" sections, perhaps adding one for "Partition triggers:" would be > enough, although I am inclined to think it needs exposure at the \d > level. One other thing to consider is firing order of said triggers... > if all parent level triggers fire before child level triggers then the > above separation is straightforward, but if the execution order is, as > I suspect, mixed, then it becomes more complicated. The firing order is alphabetical across the whole set, i.e. parent/child triggers are interleaved. See the regression test output at the bottom of this email. I looked into adding a "Partition triggers" section because it seemed a nice idea, but looking at the code I realized it's a bit tough because we already split triggers in "categories": normal triggers, disabled triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c line 2795). Since the trigger being in a partition is orthogonal to that classification, we would end up with eight groups. Not sure that's great. The attached patch (catversion bump not included -- beware of server crash if you run it without initdb'ing) keeps the categories the same. So with my previous example setup, you can see the duplicate triggers in psql: alvherre=# \d child Table "public.child" Column │ Type │ Collation │ Nullable │ Default ┼─┼───┼──┼─ a │ integer │ │ │ Partition of: parent FOR VALUES FROM (0) TO (100) Triggers: trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() and as soon as you try to drop the second one, you'll learn the truth about it: alvherre=# drop trigger trig_p on child; ERROR: cannot drop trigger trig_p on table child because trigger trig_p on table parent requires it SUGERENCIA: You can drop trigger trig_p on table parent instead. Time: 1,443 ms I say this is sufficient. -- Verify that triggers fire in alphabetical order create table parted_trig (a int) partition by range (a); create table parted_trig_1 partition of parted_trig for values from (0) to (1000) partition by range (a); create table parted_trig_1_1 partition of parted_trig_1 for values from (0) to (100); create table parted_trig_2 partition of parted_trig for values from (1000) to (2000); create trigger zzz after insert on parted_trig for each row execute procedure trigger_notice(); create trigger mmm after insert on parted_trig_1_1 for each row execute procedure trigger_notice(); create trigger aaa after insert on parted_trig_1 for each row execute procedure trigger_notice(); create trigger bbb after insert on parted_trig for each row execute procedure trigger_notice(); create trigger qqq after insert on parted_trig_1_1 for each row execute procedure trigger_notice(); insert into parted_trig values (50), (1500); NOTICE: trigger aaa on parted_trig_1_1 AFTER INSERT for ROW NOTICE: trigger bbb on parted_trig_1_1 AFTER INSERT for ROW NOTICE: trigger mmm on parted_trig_1_1 AFTER INSERT for ROW NOTICE: trigger qqq on parted_trig_1_1 AFTER INSERT for ROW NOTICE: trigger zzz on parted_trig_1_1 AFTER INSERT for ROW NOTICE: trigger bbb on parted_trig_2 AFTER INSERT for ROW NOTICE: trigger zzz on parted_trig_2 AFTER INSERT for ROW -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services diff --git a/src/backend/commands/trigger.c b/src/backend/commands/trigger.c index 57519fe8d6..9542856d6a 100644 --- a/src/backend/commands/trigger.c +++ b/src/backend/commands/trigger.c @@ -815,11 +815,6 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, /* * Build the new pg_trigger tuple. -* -* When we're creating a trigger in a partition, we mark it as internal, -* even though we don't do the isInternal magic in this function. This -* makes the triggers in partitions identical to the ones in the -* partitioned tables, except that they are marked internal. */ memset(nulls, false, sizeof(nulls)); @@ -829,7 +824,8 @@ CreateTrigger(CreateTrigStmt *stmt, const char *queryString, values[Anum_pg_trigger_tgfoid - 1] = Object
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-19, Amit Langote wrote: > In CreateTrigger(), 86f575948c7 did this. > > -values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal); > +values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal || > in_partition); > > I'm not sure why it had to be done, but undoing this change doesn't seem > to break any regression tests (neither those added by 86f575948c7 nor of > the partitioned table foreign key commit). Did we really intend to change > the meaning of tginternal like this here? I'm pretty sure pg_dump breaks if you turn that flag off for triggers in partitions, because pg_dump is going to emit commands to create those triggers, and those fail. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 18, 2018 at 10:29 PM, Alvaro Herrera wrote: > On 2018-Jun-18, Ashutosh Bapat wrote: > >> That's a wrong comparison. Inheritance based partitioning works even >> after declarative partitioning feature is added. So, users can >> continue using inheritance based partitioning if they don't want to >> move to declarative partitioning. That's not true here. A user creates >> a BEFORE ROW trigger on each partition that mimics partitioned table >> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on >> partitioned table will cascade the trigger down to each partition. If >> that fails to recognize that there is already an equivalent trigger, a >> partition table will get two triggers doing the same thing silently >> without any warning or notice. That's fine if the trigger changes the >> salaries to $50K but not OK if each of those increases salary by 10%. >> The database has limited ability to recognize what a trigger is doing. > > I agree with Robert that nobody in their right minds would be caught by > this problem by adding new triggers without thinking about it and > without testing them. If you add a partitioned-table-level trigger to > raise salaries by 10% but there was already one in the partition level > that does the same thing, you'll readily notice that salaries have been > increased by 21% instead. > > So like Robert I'm inclined to not change the wording in the > documentation. > > What does worry me a little bit now, reading this discussion, is whether > we've made the triggers in partitions visible enough. We'll have this > problem once we implement BEFORE ROW triggers as proposed, and I think > we already have this problem now. Let's suppose a user creates a > duplicated after trigger: > > create table parent (a int) partition by range (a); > create table child partition of parent for values from (0) to (100); > create function noise() returns trigger language plpgsql as $$ begin raise > notice 'nyaa'; return null; end; $$; > create trigger trig_p after insert on parent for each row execute procedure > noise(); > create trigger trig_c after insert on child for each row execute procedure > noise(); > > Now let's try it: > > alvherre=# insert into child values (1); > NOTICE: nyaa > NOTICE: nyaa > INSERT 0 1 > > OK, so where does that one come from? > > alvherre=# \d child > Tabla «public.child» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition of: parent FOR VALUES FROM (0) TO (100) > Triggers: > trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() > > Hmm, there's only one trigger here, why does it appear twice? To find > out, you have to know where to look: > > alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger; > tgname │ tgrelid │ tgisinternal > ┼─┼── > trig_p │ parent │ f > trig_p │ child │ t > trig_c │ child │ f > (3 filas) > > So there is a trigger in table child, but it's hidden because > tgisinternal. Of course, you can see it if you look at the parent's > definition: > > alvherre=# \d parent >Tabla «public.parent» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition key: RANGE (a) > Triggers: > trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise() > Number of partitions: 1 (Use \d+ to list them.) > > I think it'd be useful to have a list of triggers that have been > inherited from ancestors, or maybe simply a list of internal triggers That's a very good description of the problem people will face. Thanks for elaborating it this way. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, Jun 19, 2018 at 3:51 AM, Robert Treat wrote: > > So +1 for thinking we do need to worry about it. I'm not exactly sure > how we want to expose that info; with \d+ we list various "Partition > X:" sections, perhaps adding one for "Partition triggers:" would be > enough, although I am inclined to think it needs exposure at the \d > level. Something like this or what Alvaro proposed will be helpful. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
Hi. On 2018/06/19 1:59, Alvaro Herrera wrote: > What does worry me a little bit now, reading this discussion, is whether > we've made the triggers in partitions visible enough. We'll have this > problem once we implement BEFORE ROW triggers as proposed, and I think > we already have this problem now. Let's suppose a user creates a > duplicated after trigger: > > create table parent (a int) partition by range (a); > create table child partition of parent for values from (0) to (100); > create function noise() returns trigger language plpgsql as $$ begin raise > notice 'nyaa'; return null; end; $$; > create trigger trig_p after insert on parent for each row execute procedure > noise(); > create trigger trig_c after insert on child for each row execute procedure > noise(); > > Now let's try it: > > alvherre=# insert into child values (1); > NOTICE: nyaa > NOTICE: nyaa > INSERT 0 1 > > OK, so where does that one come from? > > alvherre=# \d child > Tabla «public.child» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition of: parent FOR VALUES FROM (0) TO (100) > Triggers: > trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() > > Hmm, there's only one trigger here, why does it appear twice? To find > out, you have to know where to look: > > alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger; > tgname │ tgrelid │ tgisinternal > ┼─┼── > trig_p │ parent │ f > trig_p │ child │ t > trig_c │ child │ f > (3 filas) > > So there is a trigger in table child, but it's hidden because > tgisinternal. Of course, you can see it if you look at the parent's > definition: > > alvherre=# \d parent >Tabla «public.parent» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition key: RANGE (a) > Triggers: > trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise() > Number of partitions: 1 (Use \d+ to list them.) > > I think it'd be useful to have a list of triggers that have been > inherited from ancestors, or maybe simply a list of internal triggers In CreateTrigger(), 86f575948c7 did this. -values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal); +values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal || in_partition); I'm not sure why it had to be done, but undoing this change doesn't seem to break any regression tests (neither those added by 86f575948c7 nor of the partitioned table foreign key commit). Did we really intend to change the meaning of tginternal like this here? Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 18, 2018 at 12:59 PM, Alvaro Herrera wrote: > On 2018-Jun-18, Ashutosh Bapat wrote: > >> That's a wrong comparison. Inheritance based partitioning works even >> after declarative partitioning feature is added. So, users can >> continue using inheritance based partitioning if they don't want to >> move to declarative partitioning. That's not true here. A user creates >> a BEFORE ROW trigger on each partition that mimics partitioned table >> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on >> partitioned table will cascade the trigger down to each partition. If >> that fails to recognize that there is already an equivalent trigger, a >> partition table will get two triggers doing the same thing silently >> without any warning or notice. That's fine if the trigger changes the >> salaries to $50K but not OK if each of those increases salary by 10%. >> The database has limited ability to recognize what a trigger is doing. > > I agree with Robert that nobody in their right minds would be caught by > this problem by adding new triggers without thinking about it and > without testing them. If you add a partitioned-table-level trigger to > raise salaries by 10% but there was already one in the partition level > that does the same thing, you'll readily notice that salaries have been > increased by 21% instead. > > So like Robert I'm inclined to not change the wording in the > documentation. > > What does worry me a little bit now, reading this discussion, is whether > we've made the triggers in partitions visible enough. We'll have this > problem once we implement BEFORE ROW triggers as proposed, and I think > we already have this problem now. Let's suppose a user creates a > duplicated after trigger: > > create table parent (a int) partition by range (a); > create table child partition of parent for values from (0) to (100); > create function noise() returns trigger language plpgsql as $$ begin raise > notice 'nyaa'; return null; end; $$; > create trigger trig_p after insert on parent for each row execute procedure > noise(); > create trigger trig_c after insert on child for each row execute procedure > noise(); > > Now let's try it: > > alvherre=# insert into child values (1); > NOTICE: nyaa > NOTICE: nyaa > INSERT 0 1 > > OK, so where does that one come from? > > alvherre=# \d child > Tabla «public.child» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition of: parent FOR VALUES FROM (0) TO (100) > Triggers: > trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() > > Hmm, there's only one trigger here, why does it appear twice? To find > out, you have to know where to look: > > alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger; > tgname │ tgrelid │ tgisinternal > ┼─┼── > trig_p │ parent │ f > trig_p │ child │ t > trig_c │ child │ f > (3 filas) > > So there is a trigger in table child, but it's hidden because > tgisinternal. Of course, you can see it if you look at the parent's > definition: > > alvherre=# \d parent >Tabla «public.parent» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition key: RANGE (a) > Triggers: > trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise() > Number of partitions: 1 (Use \d+ to list them.) > > I think it'd be useful to have a list of triggers that have been > inherited from ancestors, or maybe simply a list of internal triggers > > Or maybe this is not something to worry about? > One of the things I was thinking about while reading this thread is that the scenario of creating "duplicate" triggers on a table meaning two triggers doing the same thing is entirely possible now but we don't really do anything to prevent it, which is Ok. I've never seen much merit in the argument "people should test" (it assumes a certain infallibility that just isn't true) but we've also generally been pretty good about exposing what is going on so people can debug this type of unexpected behavior. So +1 for thinking we do need to worry about it. I'm not exactly sure how we want to expose that info; with \d+ we list various "Partition X:" sections, perhaps adding one for "Partition triggers:" would be enough, although I am inclined to think it needs exposure at the \d level. One other thing to consider is firing order of said triggers... if all parent level triggers fire before child level triggers then the above separation is straightforward, but if the execution order is, as I suspect, mixed, then it becomes more complicated. Robert Treat http://xzilla.net
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 18, 2018 at 9:59 AM, Alvaro Herrera wrote: > > alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger; > tgname │ tgrelid │ tgisinternal > ┼─┼── > trig_p │ parent │ f > trig_p │ child │ t > trig_c │ child │ f > (3 filas) > > So there is a trigger in table child, but it's hidden because > tgisinternal. Of course, you can see it if you look at the parent's > definition: > > alvherre=# \d parent >Tabla «public.parent» > Columna │ Tipo │ Collation │ Nullable │ Default > ─┼─┼───┼──┼─ > a │ integer │ │ │ > Partition key: RANGE (a) > Triggers: > trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise() > Number of partitions: 1 (Use \d+ to list them.) > > I think it'd be useful to have a list of triggers that have been > inherited from ancestors, or maybe simply a list of internal triggers > > Or maybe this is not something to worry about? For the main internal trigger, foreign key, we don't show the trigger on the relevant table but we do indicate its effect by showing the presence of the foreign key. We likewise need to show the effect of the inherited trigger on the child table. When viewing the output the display order of invocation should be retained (is it that way now?) as a primary goal - with the directed or inherited nature presentation dependent upon that. i.e., I would like to see "Parent Triggers:" and "Triggers:" sections if possible but if trig_c is going to be invoked before trig_p that won't work and having a single "Triggers:" section with "(parent)" somewhere in the trigger info printout would be preferred. David J.
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-18, Ashutosh Bapat wrote: > That's a wrong comparison. Inheritance based partitioning works even > after declarative partitioning feature is added. So, users can > continue using inheritance based partitioning if they don't want to > move to declarative partitioning. That's not true here. A user creates > a BEFORE ROW trigger on each partition that mimics partitioned table > level BEFORE ROW trigger. The proposed BEFORE ROW trigger on > partitioned table will cascade the trigger down to each partition. If > that fails to recognize that there is already an equivalent trigger, a > partition table will get two triggers doing the same thing silently > without any warning or notice. That's fine if the trigger changes the > salaries to $50K but not OK if each of those increases salary by 10%. > The database has limited ability to recognize what a trigger is doing. I agree with Robert that nobody in their right minds would be caught by this problem by adding new triggers without thinking about it and without testing them. If you add a partitioned-table-level trigger to raise salaries by 10% but there was already one in the partition level that does the same thing, you'll readily notice that salaries have been increased by 21% instead. So like Robert I'm inclined to not change the wording in the documentation. What does worry me a little bit now, reading this discussion, is whether we've made the triggers in partitions visible enough. We'll have this problem once we implement BEFORE ROW triggers as proposed, and I think we already have this problem now. Let's suppose a user creates a duplicated after trigger: create table parent (a int) partition by range (a); create table child partition of parent for values from (0) to (100); create function noise() returns trigger language plpgsql as $$ begin raise notice 'nyaa'; return null; end; $$; create trigger trig_p after insert on parent for each row execute procedure noise(); create trigger trig_c after insert on child for each row execute procedure noise(); Now let's try it: alvherre=# insert into child values (1); NOTICE: nyaa NOTICE: nyaa INSERT 0 1 OK, so where does that one come from? alvherre=# \d child Tabla «public.child» Columna │ Tipo │ Collation │ Nullable │ Default ─┼─┼───┼──┼─ a │ integer │ │ │ Partition of: parent FOR VALUES FROM (0) TO (100) Triggers: trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise() Hmm, there's only one trigger here, why does it appear twice? To find out, you have to know where to look: alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger; tgname │ tgrelid │ tgisinternal ┼─┼── trig_p │ parent │ f trig_p │ child │ t trig_c │ child │ f (3 filas) So there is a trigger in table child, but it's hidden because tgisinternal. Of course, you can see it if you look at the parent's definition: alvherre=# \d parent Tabla «public.parent» Columna │ Tipo │ Collation │ Nullable │ Default ─┼─┼───┼──┼─ a │ integer │ │ │ Partition key: RANGE (a) Triggers: trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise() Number of partitions: 1 (Use \d+ to list them.) I think it'd be useful to have a list of triggers that have been inherited from ancestors, or maybe simply a list of internal triggers Or maybe this is not something to worry about? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 18, 2018 at 1:20 AM, Ashutosh Bapat wrote: > That's a wrong comparison. Inheritance based partitioning works even > after declarative partitioning feature is added. So, users can > continue using inheritance based partitioning if they don't want to > move to declarative partitioning. That's not true here. A user creates > a BEFORE ROW trigger on each partition that mimics partitioned table > level BEFORE ROW trigger. The proposed BEFORE ROW trigger on > partitioned table will cascade the trigger down to each partition. If > that fails to recognize that there is already an equivalent trigger, a > partition table will get two triggers doing the same thing silently > without any warning or notice. That's fine if the trigger changes the > salaries to $50K but not OK if each of those increases salary by 10%. > The database has limited ability to recognize what a trigger is doing. I don't even know what to say about that. You are correct that if a user creates a new trigger on a partitioned table and doesn't check what happens to any existing triggers that they already have, bad things might happen. But that seems like a pretty stupid thing to do. If you make *any* kind of critical change to your production database without testing it, bad things may happen. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Sat, Jun 16, 2018 at 9:29 AM, Robert Haas wrote: > > By that logic, we should not have suggested that anyone use table > inheritance, because they would have to change their configuration > when we implemented table partitioning. Indeed, switching from table > inheritance to table partitioning is much more intrusive than dropping > triggers on individual partitions and adding one on the partitioned > table. The latter only requires DDL on existing objects, but the > former requires creating entirely new objects and moving all of your > data. That's a wrong comparison. Inheritance based partitioning works even after declarative partitioning feature is added. So, users can continue using inheritance based partitioning if they don't want to move to declarative partitioning. That's not true here. A user creates a BEFORE ROW trigger on each partition that mimics partitioned table level BEFORE ROW trigger. The proposed BEFORE ROW trigger on partitioned table will cascade the trigger down to each partition. If that fails to recognize that there is already an equivalent trigger, a partition table will get two triggers doing the same thing silently without any warning or notice. That's fine if the trigger changes the salaries to $50K but not OK if each of those increases salary by 10%. The database has limited ability to recognize what a trigger is doing. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jun 14, 2018 at 9:45 AM, Ashutosh Bapat wrote: >> It sounds like you want to try to hide from users the fact that they >> can create triggers on the individual partitions. > > No. I never said that in my mails (see [1], [2]) I object to the > explicit suggestion that they can/should create BEFORE ROW triggers on > partitions since those are not supported on the partitioned table. > >> I think that's the >> wrong approach. First of all, it's not going to work, because people >> are going to find out about it and do it anyway. Secondly, the >> documentation's job is to tell you about the system's capabilities, >> not conceal them from you. > > Neither is documentation's job to "suggest" something that can lead to > problems in future. Well, perhaps what this boils down to is that I don't agree that it can lead to problems in the future. If the user creates a trigger on each partition, whether they are all the same or some are different from others, they can stick with that configuration in future releases, too. I don't think we're going to remove the ability to have separate triggers on each partition; we're just going to add, as an additional possibility, the ability to have a trigger on the partitioned table that cascades to the individual partitions. It's true that if people want to get the benefit of that feature, they'll have to change the configuration, but so what? That's true of many new features, and I don't think it's right to characterize that as a problem. By that logic, we should not have suggested that anyone use table inheritance, because they would have to change their configuration when we implemented table partitioning. Indeed, switching from table inheritance to table partitioning is much more intrusive than dropping triggers on individual partitions and adding one on the partitioned table. The latter only requires DDL on existing objects, but the former requires creating entirely new objects and moving all of your data. I think that the existing wording is fine and there's really no need to change anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/14 22:45, Ashutosh Bapat wrote: > On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas wrote: >> It sounds like you want to try to hide from users the fact that they >> can create triggers on the individual partitions. > > No. I never said that in my mails (see [1], [2]) I object to the > explicit suggestion that they can/should create BEFORE ROW triggers on > partitions since those are not supported on the partitioned table. I understand Ashutosh's worry as follows: A workaround for the limitation that BR triggers cannot be defined on partitioned tables yet is to define that *same* trigger on all partitions, which works from the beginning (PG 10), so that gets the job done. But... some users may however fail to ensure that the trigger's definition is exactly alike for each partition, especially they are likely to make each trigger call a different function, although each of those functions may have the same body of code. When in some future release, one is able to define the trigger on the partitioned table and does so, the trigger will end up being created again on each partition, because the existing trigger on each partition (after upgrading) would be different due to a different function being called in each. I proposed that we reword the sentence that describes this particular limitation to warn users about that. See if the attached patch is any good for that. Thanks, Amit diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 0258391154..781536c44b 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -3349,8 +3349,10 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 - BEFORE ROW triggers, if necessary, must be defined - on individual partitions, not the partitioned table. + Creating BEFORE ROW triggers on partitioned tables + is not supported. A workaround is to create such a trigger on individual + partitions, but make sure that its definition is identical across all + partitions, including the function that's called from the trigger.
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
> On Jun 14, 2018, at 9:19 AM, Robert Haas wrote: > > anyone who wants a BEFORE trigger has a good reason > for wanting it. I have used before triggers to enforce the immutability of a column. i.e. if (new.member_key != old.member_key) then raise exception 'Unable to change member_key, column is immutable'; end if ;
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas wrote: > On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat > wrote: >> On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera >> wrote: >> >>> - BEFORE row triggers are not supported >> >> I think this is fine. The existing wording suggests that the user >> creates the triggers on the partitioned table, and that will be >> supported always, which can lead to problems. With this description, >> if the user finds out that BEFORE triggers are supported on partitions >> and creates those, and we implement something to support those on >> partitioned table, s/he will have to change the implementation >> accordingly. > > It sounds like you want to try to hide from users the fact that they > can create triggers on the individual partitions. No. I never said that in my mails (see [1], [2]) I object to the explicit suggestion that they can/should create BEFORE ROW triggers on partitions since those are not supported on the partitioned table. > I think that's the > wrong approach. First of all, it's not going to work, because people > are going to find out about it and do it anyway. Secondly, the > documentation's job is to tell you about the system's capabilities, > not conceal them from you. Neither is documentation's job to "suggest" something that can lead to problems in future. [1] https://www.postgresql.org/message-id/cafjfprfzcvnul0l3_qafqr5xfn-eynbmow4gf-2moo7gnaz...@mail.gmail.com [2] https://www.postgresql.org/message-id/CAFjFpRfzCVnuL0L3_qAFqr5xFN-EynbMoW4gf-2moO7gNazADA%40mail.gmail.com -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat wrote: > On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera > wrote: > >> - BEFORE row triggers are not supported > > I think this is fine. The existing wording suggests that the user > creates the triggers on the partitioned table, and that will be > supported always, which can lead to problems. With this description, > if the user finds out that BEFORE triggers are supported on partitions > and creates those, and we implement something to support those on > partitioned table, s/he will have to change the implementation > accordingly. It sounds like you want to try to hide from users the fact that they can create triggers on the individual partitions. I think that's the wrong approach. First of all, it's not going to work, because people are going to find out about it and do it anyway. Secondly, the documentation's job is to tell you about the system's capabilities, not conceal them from you. Third, any speculation about what upgrade problems people might have in the future is just that: speculation. As the saying goes, it's tough to make predictions, especially about the future. To put that another way, if we really think nobody should do this, we should prohibit it, not leave it out of the documentation. But I think prohibiting it would be a terrible idea: it would break upgrades from existing releases and inconvenience users to no good purpose. Very few, if any, users say, well, I don't really need a trigger on this table, but PostgreSQL is really quite a bit faster than I need it to be, so I think I'll add one anyway just to slow things down. We should assume that anyone who wants a BEFORE trigger has a good reason for wanting it. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 4, 2018 at 5:40 PM, Tom Lane wrote: >> I think, in general, that we should try to pick semantics that make a >> partitioned table behave like an unpartitioned table, provided that >> all triggers are defined on the partitioned table itself. > > Well, then we lose the property Alvaro wanted, namely that if an > application chooses to insert directly into a partition, that's > just an optimization that changes no behavior (as long as it picked > the right partition). Maybe this can be dodged by propagating > parent trigger definitions to the children, but it's going to be > complicated I'm afraid. Isn't this basically what I already proposed in http://postgr.es/m/CA+TgmoYQD1xSM7=xry6rv2a-w43gkpcth76f3nsp5o2sgwe...@mail.gmail.com ? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jun 14, 2018 at 1:54 PM, Amit Langote wrote: > On 2018/06/12 22:22, Ashutosh Bapat wrote: >> -- create triggers, user may create different trigger functions one >> for each partition, unless s/he understands that the tables can share >> trigger functions >> create function trig_t1p1() returns trigger as $$ begin return new; >> end;$$ language plpgsql; >> create trigger trig_t1p1_1 before update on t1p1 execute procedure >> trig_t1p1(); >> create trigger trig_t1p1_2 before update on t1p1 execute procedure >> trig_t1p1(); >> >> create function trig_t1p2() returns trigger as $$ begin return new; >> end;$$ language plpgsql; >> create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2(); >> >> When this schema gets upgraded to v12 or whatever version supports >> BEFORE ROW triggers on a partitioned table and the user tries to >> create a trigger on a partitioned table >> 1. somehow the code has to detect that there are already triggers on >> the partitions so no need to create new triggers on the partitions. I >> don't see how this can be done, unless the user is smart enough to use >> the same trigger function for all partitions. >> >> 2. user has to drop those triggers and trigger functions and create >> trigger on the partitioned table. >> >> May be I am underestimating users but I am sure there will be some >> users who will end up with scenario. > > Hmm, if a user *knowingly* makes triggers on different partitions call > different functions, then they wouldn't want to use the feature to create > the trigger on partitioned tables, because that feature implies same > trigger definition for all partitions. How many users would do that *knowingly*? > > Maybe, just as a reminder to users, we could mention in the docs that it > is in fact possible to call the same function from different triggers > (that is, triggers defined on different partitions) and that's what they > should do if they intend to later use the feature to define that same > trigger on the partitioned table. +1 for something like this, but wording is important. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/12 22:22, Ashutosh Bapat wrote: > -- create triggers, user may create different trigger functions one > for each partition, unless s/he understands that the tables can share > trigger functions > create function trig_t1p1() returns trigger as $$ begin return new; > end;$$ language plpgsql; > create trigger trig_t1p1_1 before update on t1p1 execute procedure > trig_t1p1(); > create trigger trig_t1p1_2 before update on t1p1 execute procedure > trig_t1p1(); > > create function trig_t1p2() returns trigger as $$ begin return new; > end;$$ language plpgsql; > create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2(); > > When this schema gets upgraded to v12 or whatever version supports > BEFORE ROW triggers on a partitioned table and the user tries to > create a trigger on a partitioned table > 1. somehow the code has to detect that there are already triggers on > the partitions so no need to create new triggers on the partitions. I > don't see how this can be done, unless the user is smart enough to use > the same trigger function for all partitions. > > 2. user has to drop those triggers and trigger functions and create > trigger on the partitioned table. > > May be I am underestimating users but I am sure there will be some > users who will end up with scenario. Hmm, if a user *knowingly* makes triggers on different partitions call different functions, then they wouldn't want to use the feature to create the trigger on partitioned tables, because that feature implies same trigger definition for all partitions. Maybe, just as a reminder to users, we could mention in the docs that it is in fact possible to call the same function from different triggers (that is, triggers defined on different partitions) and that's what they should do if they intend to later use the feature to define that same trigger on the partitioned table. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Sat, Jun 9, 2018 at 3:48 AM, Alvaro Herrera wrote: > On 2018-Jun-08, Alvaro Herrera wrote: > >> Actually, the column order doesn't matter for a trigger function, >> because these don't refer to columns by number but by name. So unless >> users write trigger functions in C and use hardcoded column numbers >> (extremely unlikely), I think this is not an issue. In other words, in >> the interesting cases the trigger functions are the same for all >> partitions -- therefore upgrading from separate per-partition triggers >> to one master trigger in the partitioned table is not going to be that >> difficult, ISTM. > > One last thing. This wording has been there since pg10; the only thing > that changed is that it now says "BEFORE ROW triggers" instead of "Row > triggers". I don't think leaving it there one more year is going to get > us too much grief, TBH. I am worried about following scenario -- create partitioned table create table t1 (a int, b int) partition by range(a); -- create some tables which will be attached to the partitioned table in future create table t1p1 (like t1); create table t1p2 (like t1); -- those tables have indexes create index t1p1_a on t1p1(a); create index t1p2_a on t1p2(a); -- attach partitions alter table t1 attach partition t1p1 for values from (0) to (100); alter table t1 attach partition t1p2 for values from (100) to (200); -- create index on partitioned table, it doesn't create new indexes on t1p1 and t1p2 since there are already indexes on a create index t1_a on t1(a); -- create triggers, user may create different trigger functions one for each partition, unless s/he understands that the tables can share trigger functions create function trig_t1p1() returns trigger as $$ begin return new; end;$$ language plpgsql; create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1(); create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1(); create function trig_t1p2() returns trigger as $$ begin return new; end;$$ language plpgsql; create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2(); When this schema gets upgraded to v12 or whatever version supports BEFORE ROW triggers on a partitioned table and the user tries to create a trigger on a partitioned table 1. somehow the code has to detect that there are already triggers on the partitions so no need to create new triggers on the partitions. I don't see how this can be done, unless the user is smart enough to use the same trigger function for all partitions. 2. user has to drop those triggers and trigger functions and create trigger on the partitioned table. May be I am underestimating users but I am sure there will be some users who will end up with scenario. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-08, Alvaro Herrera wrote: > Actually, the column order doesn't matter for a trigger function, > because these don't refer to columns by number but by name. So unless > users write trigger functions in C and use hardcoded column numbers > (extremely unlikely), I think this is not an issue. In other words, in > the interesting cases the trigger functions are the same for all > partitions -- therefore upgrading from separate per-partition triggers > to one master trigger in the partitioned table is not going to be that > difficult, ISTM. One last thing. This wording has been there since pg10; the only thing that changed is that it now says "BEFORE ROW triggers" instead of "Row triggers". I don't think leaving it there one more year is going to get us too much grief, TBH. I'm going to leave this as an open item for one or two more weeks and see if there's more support for Ashutosh's PoV; but if not, my proposal is to close it without changing anything further. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-07, Ashutosh Bapat wrote: > On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote > wrote: > > I don't understand why you think it's too troublesome to let the users > > know that there is some way to use BR triggers with partitioning. We > > didn't do that for indexes, for example, before PG 11 introduced the > > ability to create them on partitioned tables. > > By looking at the index keys it's easy to decide whether the two > indexes are same. When we add an index on a partitioned table in v11, > we skip creating an index on the partition if there exists an index > similar to the one being created. So, a user can have indexes on > partitions created in v10, upgrade to v11 and create an index on the > partitioned table. Nothing changes. But that's not true for a trigger. > It's not easy to check whether two triggers are same or not unless the > underlying function is same. User may or may not be using same trigger > function for all the partitions, which is more true, if the column > order differs between partitions. So, if the user has created triggers > on the partitions in v10, upgrades to v11, s/he may have to drop them > all and recreate the trigger on the partitioned table. Actually, the column order doesn't matter for a trigger function, because these don't refer to columns by number but by name. So unless users write trigger functions in C and use hardcoded column numbers (extremely unlikely), I think this is not an issue. In other words, in the interesting cases the trigger functions are the same for all partitions -- therefore upgrading from separate per-partition triggers to one master trigger in the partitioned table is not going to be that difficult, ISTM. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote wrote: > On 2018/06/07 14:17, Ashutosh Bapat wrote: >>> that is, users can find out about that feature by themselves by >>> trying it out? >> >> I didn't understand that part. >> >> Probably we just say that BEFORE ROW triggers are not supported on a >> partitioned table. It's good enough not to suggest it ourselves. If >> users find out that they can create triggers on partitions and use it >> that way, they may or may not have to change their implementation >> later when we start supporting those. But then we didn't suggest that >> they do it that way. > > I don't understand why you think it's too troublesome to let the users > know that there is some way to use BR triggers with partitioning. We > didn't do that for indexes, for example, before PG 11 introduced the > ability to create them on partitioned tables. By looking at the index keys it's easy to decide whether the two indexes are same. When we add an index on a partitioned table in v11, we skip creating an index on the partition if there exists an index similar to the one being created. So, a user can have indexes on partitions created in v10, upgrade to v11 and create an index on the partitioned table. Nothing changes. But that's not true for a trigger. It's not easy to check whether two triggers are same or not unless the underlying function is same. User may or may not be using same trigger function for all the partitions, which is more true, if the column order differs between partitions. So, if the user has created triggers on the partitions in v10, upgrades to v11, s/he may have to drop them all and recreate the trigger on the partitioned table. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/07 14:17, Ashutosh Bapat wrote: >> that is, users can find out about that feature by themselves by >> trying it out? > > I didn't understand that part. > > Probably we just say that BEFORE ROW triggers are not supported on a > partitioned table. It's good enough not to suggest it ourselves. If > users find out that they can create triggers on partitions and use it > that way, they may or may not have to change their implementation > later when we start supporting those. But then we didn't suggest that > they do it that way. I don't understand why you think it's too troublesome to let the users know that there is some way to use BR triggers with partitioning. We didn't do that for indexes, for example, before PG 11 introduced the ability to create them on partitioned tables. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Thu, Jun 7, 2018 at 7:51 AM, Amit Langote wrote: > On 2018/06/06 20:51, Ashutosh Bapat wrote: >> The existing wording suggests that the user >> creates the triggers on the partitioned table, and that will be >> supported always, which can lead to problems. > > Do you mean the following wording > > "BEFORE ROW triggers, if necessary, must be defined on individual > partitions, not the partitioned table." > > suggests that user *can* create triggers on the partitioned table? I > guess I can see how one may read it that way. How about we make it say > something like: > > "BEFORE ROW triggers are not supported on partitioned tables; a user can > still define them, if necessary, on individual partitions though." Whichever way said, I am reading it like "We don't support BEFORE ROW triggers on partitioned tables, but you can have them by creating those on partitions". That is going to be a trouble for us. > >> With this description, >> if the user finds out that BEFORE triggers are supported on partitions >> and creates those, and we implement something to support those on >> partitioned table, s/he will have to change the implementation >> accordingly. > > So you mean that the existing wording *encourages* users to use the > feature to create BR triggers on individual partitions whereas it > shouldn't, right. > that is, users can find out about that feature by themselves by > trying it out? I didn't understand that part. Probably we just say that BEFORE ROW triggers are not supported on a partitioned table. It's good enough not to suggest it ourselves. If users find out that they can create triggers on partitions and use it that way, they may or may not have to change their implementation later when we start supporting those. But then we didn't suggest that they do it that way. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/06 20:51, Ashutosh Bapat wrote: > The existing wording suggests that the user > creates the triggers on the partitioned table, and that will be > supported always, which can lead to problems. Do you mean the following wording "BEFORE ROW triggers, if necessary, must be defined on individual partitions, not the partitioned table." suggests that user *can* create triggers on the partitioned table? I guess I can see how one may read it that way. How about we make it say something like: "BEFORE ROW triggers are not supported on partitioned tables; a user can still define them, if necessary, on individual partitions though." > With this description, > if the user finds out that BEFORE triggers are supported on partitions > and creates those, and we implement something to support those on > partitioned table, s/he will have to change the implementation > accordingly. So you mean that the existing wording *encourages* users to use the feature to create BR triggers on individual partitions whereas it shouldn't, that is, users can find out about that feature by themselves by trying it out? I think the revised wording I proposed above isn't too encouraging. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera wrote: > - BEFORE row triggers are not supported I think this is fine. The existing wording suggests that the user creates the triggers on the partitioned table, and that will be supported always, which can lead to problems. With this description, if the user finds out that BEFORE triggers are supported on partitions and creates those, and we implement something to support those on partitioned table, s/he will have to change the implementation accordingly. > > The following caveat applies to partitioned tables: > - When an UPDATE causes a row to move ..." > -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/06 2:08, Alvaro Herrera wrote: > On 2018-Jun-05, Amit Langote wrote: > >> On 2018/06/05 16:41, Ashutosh Bapat wrote: >>> On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote >>> wrote: On 2018/06/05 1:25, Alvaro Herrera wrote: > In the meantime, my inclination is to fix the documentation to point out > that AFTER triggers are allowed but BEFORE triggers are not. Wasn't that already fixed by bcded2609ade6? > > Ah, yes, so it's already OK. > >>> Thought correct that suggestion is problematic for upgrades. Probably >>> users will create triggers using same function on all the partitions. >>> After the upgrade when we start supporting BEFORE TRIGGER on >>> partitioned table, the user will have to drop these triggers and >>> create one trigger on the partitioned table to have the trigger to be >>> applicable to the new partitions created. >> >> Hmm yes, maybe there is scope for rewording then. > > Reading that subsection in its entirety, I'm not sure how to do better > -- it's all about restrictions that currently exist and we think we > could do better in the future, with the exception of the one about an > UPDATE/DELETE not seeing the updated version after a row migrating to > another partition. One idea would be to split it into its own list of > issues; something like: > > "The following limitations apply, and might be lifted in the future: > - no way to create exclusion constraint > - foreign keys cannot refer to partitioned tables > - BEFORE row triggers are not supported > > The following caveat applies to partitioned tables: > - When an UPDATE causes a row to move ..." > > In other words, make a distinction of things we expect to change soon > (which might be too optimistic), vs. others we don't expect to change. > > Or we could leave it alone; any behavior change would be called out in > the future version's release notes anyway. This is what I propose. That works for me. :) Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-05, Amit Langote wrote: > On 2018/06/05 16:41, Ashutosh Bapat wrote: > > On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote > > wrote: > >> On 2018/06/05 1:25, Alvaro Herrera wrote: > >>> In the meantime, my inclination is to fix the documentation to point out > >>> that AFTER triggers are allowed but BEFORE triggers are not. > >> > >> Wasn't that already fixed by bcded2609ade6? Ah, yes, so it's already OK. > > Thought correct that suggestion is problematic for upgrades. Probably > > users will create triggers using same function on all the partitions. > > After the upgrade when we start supporting BEFORE TRIGGER on > > partitioned table, the user will have to drop these triggers and > > create one trigger on the partitioned table to have the trigger to be > > applicable to the new partitions created. > > Hmm yes, maybe there is scope for rewording then. Reading that subsection in its entirety, I'm not sure how to do better -- it's all about restrictions that currently exist and we think we could do better in the future, with the exception of the one about an UPDATE/DELETE not seeing the updated version after a row migrating to another partition. One idea would be to split it into its own list of issues; something like: "The following limitations apply, and might be lifted in the future: - no way to create exclusion constraint - foreign keys cannot refer to partitioned tables - BEFORE row triggers are not supported The following caveat applies to partitioned tables: - When an UPDATE causes a row to move ..." In other words, make a distinction of things we expect to change soon (which might be too optimistic), vs. others we don't expect to change. Or we could leave it alone; any behavior change would be called out in the future version's release notes anyway. This is what I propose. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/05 16:41, Ashutosh Bapat wrote: > On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote > wrote: >> On 2018/06/05 1:25, Alvaro Herrera wrote: >>> In the meantime, my inclination is to fix the documentation to point out >>> that AFTER triggers are allowed but BEFORE triggers are not. >> >> Wasn't that already fixed by bcded2609ade6? >> >> We don't say anything about AFTER triggers per se, but the following under >> the limitations of partitioned tables: >> >> BEFORE ROW triggers, if necessary, must be defined on individual >> partitions, not the partitioned table. > > Thought correct that suggestion is problematic for upgrades. Probably > users will create triggers using same function on all the partitions. > After the upgrade when we start supporting BEFORE TRIGGER on > partitioned table, the user will have to drop these triggers and > create one trigger on the partitioned table to have the trigger to be > applicable to the new partitions created. Hmm yes, maybe there is scope for rewording then. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/06/05 1:25, Alvaro Herrera wrote: > In the meantime, my inclination is to fix the documentation to point out > that AFTER triggers are allowed but BEFORE triggers are not. Wasn't that already fixed by bcded2609ade6? We don't say anything about AFTER triggers per se, but the following under the limitations of partitioned tables: BEFORE ROW triggers, if necessary, must be defined on individual partitions, not the partitioned table. Thanks, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote wrote: > On 2018/06/05 1:25, Alvaro Herrera wrote: >> In the meantime, my inclination is to fix the documentation to point out >> that AFTER triggers are allowed but BEFORE triggers are not. > > Wasn't that already fixed by bcded2609ade6? > > We don't say anything about AFTER triggers per se, but the following under > the limitations of partitioned tables: > > BEFORE ROW triggers, if necessary, must be defined on individual > partitions, not the partitioned table. Thought correct that suggestion is problematic for upgrades. Probably users will create triggers using same function on all the partitions. After the upgrade when we start supporting BEFORE TRIGGER on partitioned table, the user will have to drop these triggers and create one trigger on the partitioned table to have the trigger to be applicable to the new partitions created. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 4, 2018 at 2:40 PM, Tom Lane wrote: > > I think, in general, that we should try to pick semantics that make a > > partitioned table behave like an unpartitioned table, provided that > > all triggers are defined on the partitioned table itself. > > Well, then we lose the property Alvaro wanted, namely that if an > application chooses to insert directly into a partition, that's > just an optimization that changes no behavior (as long as it picked > the right partition). Maybe this can be dodged by propagating > parent trigger definitions to the children, but it's going to be > complicated I'm afraid. > Can we give the user the option - adding a before trigger to the partitioned table forces one to forgo the ability to directly insert into the partitions? David J.
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
Robert Haas writes: > On Mon, Jun 4, 2018 at 4:50 PM, Tom Lane wrote: >> Perhaps, but I'm having a hard time wrapping my mind around what the >> semantics ought to be. If a trigger on partition A changes the keys >> so that the row shouldn't have gone into A at all, what then? That >> trigger should never have fired, eh? > Causality is for wimps. :-) Heh. > I think, in general, that we should try to pick semantics that make a > partitioned table behave like an unpartitioned table, provided that > all triggers are defined on the partitioned table itself. Well, then we lose the property Alvaro wanted, namely that if an application chooses to insert directly into a partition, that's just an optimization that changes no behavior (as long as it picked the right partition). Maybe this can be dodged by propagating parent trigger definitions to the children, but it's going to be complicated I'm afraid. regards, tom lane
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 6/4/18 16:50, Tom Lane wrote: > Perhaps, but I'm having a hard time wrapping my mind around what the > semantics ought to be. If a trigger on partition A changes the keys > so that the row shouldn't have gone into A at all, what then? That > trigger should never have fired, eh? The insert will result in an error and everything is rolled back, so you do kind of get the "should never have" behavior. It seems we ultimately have to answer the question whether we want BEFORE triggers executed before or after tuple routing. Both behaviors might be useful, so perhaps we'll need two kinds of triggers. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 4, 2018 at 4:50 PM, Tom Lane wrote: > Robert Haas writes: >> On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane wrote: >>> Could we solve it by saying that triggers on partitioned tables aren't >>> allowed to change the partitioning values? (Or at least, not allowed >>> to change them in a way that changes the target partition.) > >> That seems like a somewhat-unfortunate restriction. > > Perhaps, but I'm having a hard time wrapping my mind around what the > semantics ought to be. If a trigger on partition A changes the keys > so that the row shouldn't have gone into A at all, what then? That > trigger should never have fired, eh? Causality is for wimps. :-) I agree that it's weird if you think about a partition, but it's a lot less strange if you think about a partitioned table. If we're running the trigger before tuple routing has been done, then we ought to be able to change the results of tuple routing. I think, in general, that we should try to pick semantics that make a partitioned table behave like an unpartitioned table, provided that all triggers are defined on the partitioned table itself. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
Robert Haas writes: > On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane wrote: >> Could we solve it by saying that triggers on partitioned tables aren't >> allowed to change the partitioning values? (Or at least, not allowed >> to change them in a way that changes the target partition.) > That seems like a somewhat-unfortunate restriction. Perhaps, but I'm having a hard time wrapping my mind around what the semantics ought to be. If a trigger on partition A changes the keys so that the row shouldn't have gone into A at all, what then? That trigger should never have fired, eh? regards, tom lane
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane wrote: > Could we solve it by saying that triggers on partitioned tables aren't > allowed to change the partitioning values? (Or at least, not allowed > to change them in a way that changes the target partition.) That seems like a somewhat-unfortunate restriction. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-04, Tom Lane wrote: > Alvaro Herrera writes: > > On 2018-Jun-04, Tom Lane wrote: > >> ... why doesn't the same problem apply to AFTER triggers that are attached > >> to the inheritance parent? > > > With a BEFORE trigger, running the trigger might change the target > > partition, which has the potential for all kinds of trouble. > > Got it. That seems like not just an implementation restriction, but > a pretty fundamental issue. > > Could we solve it by saying that triggers on partitioned tables aren't > allowed to change the partitioning values? (Or at least, not allowed > to change them in a way that changes the target partition.) Yes, that would be one way forward. In fact, IIUC what happens today if you remove the restriction (as Amit tested and reported upthread[1]) is that this already causes an error, because tuple routing is not re-done after BEFORE triggers are run. I was thinking it would be good to leave the option open (i.e. forbid BEFORE triggers altogether) so that in the future we could implement that case too, and avoid a backwards-incompatible behavior change. Thinking about it again, this may not be a problem: if you write a trigger that works (it doesn't change the target partition) then when forward-porting it to a version that does allow the target partition to change, your trigger doesn't change behavior. So maybe it's okay to remove the restriction. I'll test more tomorrow. [1] https://postgr.es/m/1824bda1-0c47-abc4-8b97-e37414c52...@lab.ntt.co.jp -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
Alvaro Herrera writes: > On 2018-Jun-04, Tom Lane wrote: >> ... why doesn't the same problem apply to AFTER triggers that are attached >> to the inheritance parent? > With a BEFORE trigger, running the trigger might change the target > partition, which has the potential for all kinds of trouble. Got it. That seems like not just an implementation restriction, but a pretty fundamental issue. Could we solve it by saying that triggers on partitioned tables aren't allowed to change the partitioning values? (Or at least, not allowed to change them in a way that changes the target partition.) regards, tom lane
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-Jun-04, Tom Lane wrote: > Alvaro Herrera writes: > > This kind of inconsistency is what I wanted to avoid. One of the > > guiding principles here was that a partitioned table behaves just like a > > regular table does; in particular, inserting directly into a partition > > is an application-level optimization that must behave exactly like it > > would if the insert had gone into its parent table (unless the user > > explicitly makes it not so). If we make an insertion into the partition > > *not* fire the trigger that would have been fired by inserting into the > > partitioned table, that's a bug. I couldn't see a way to have the > > BEFORE trigger handle that nicely, so I decided to punt on that feature. > > Reasonable, but ... > > > In the meantime, my inclination is to fix the documentation to point out > > that AFTER triggers are allowed but BEFORE triggers are not. > > ... why doesn't the same problem apply to AFTER triggers that are attached > to the inheritance parent? The trigger is not executed as attached to the parent; it's only executed as attached to the partition itself. So you create it in the parent, and clone so that all partitions have it; when you run the command, only the cloned trigger is run, so in effect the trigger runs exactly once. Because the trigger runs *after* the target partition has been selected, and the trigger cannot change that outcome (ie. it cannot move the row to another partition) this works fine. With a BEFORE trigger, running the trigger might change the target partition, which has the potential for all kinds of trouble. Also, there's room to say that the before trigger should run before partition routing is even invoked (hence the idea of running the triggers in the partitioned table rather than the partition). But in that case we must not run the trigger again when executing the insertion in the chosen partition; and we must do *something* if the user executes the command targetting the partition rather than the parent table. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
Alvaro Herrera writes: > This kind of inconsistency is what I wanted to avoid. One of the > guiding principles here was that a partitioned table behaves just like a > regular table does; in particular, inserting directly into a partition > is an application-level optimization that must behave exactly like it > would if the insert had gone into its parent table (unless the user > explicitly makes it not so). If we make an insertion into the partition > *not* fire the trigger that would have been fired by inserting into the > partitioned table, that's a bug. I couldn't see a way to have the > BEFORE trigger handle that nicely, so I decided to punt on that feature. Reasonable, but ... > In the meantime, my inclination is to fix the documentation to point out > that AFTER triggers are allowed but BEFORE triggers are not. ... why doesn't the same problem apply to AFTER triggers that are attached to the inheritance parent? regards, tom lane
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-May-03, Robert Haas wrote: > On Wed, May 2, 2018 at 9:17 AM, Ashutosh Bapat > wrote: > > +1 for that. We should associate before row triggers with the > > partitioned table and not with the partitions. Those should be > > executed before tuple routing (for that partition level) kicks in. But > > then that would be asymetric with AFTER row trigger behaviour. From > > this POV, pushing AFTER row triggers to the individual partitions > > doesn't look good. > > The asymmetry doesn't seem horrible to me on its own merits, but it > would lead to some odd behavior: suppose you defined a BEFORE ROW > trigger and an AFTER ROW trigger on the parent, and then inserted one > row into the parent table and a second row directly into a partition. > It seems like the BEFORE ROW trigger would fire only for the parent > insert, but the AFTER ROW trigger would fire in both cases. This kind of inconsistency is what I wanted to avoid. One of the guiding principles here was that a partitioned table behaves just like a regular table does; in particular, inserting directly into a partition is an application-level optimization that must behave exactly like it would if the insert had gone into its parent table (unless the user explicitly makes it not so). If we make an insertion into the partition *not* fire the trigger that would have been fired by inserting into the partitioned table, that's a bug. I couldn't see a way to have the BEFORE trigger handle that nicely, so I decided to punt on that feature. So I stand by my original decision to disallow BEFORE triggers on partitioned tables altogether -- we can add that as a new feature later, keeping in mind the above ... namely to implement Robert's idea: > What seems like a better idea is to have the BEFORE ROW trigger get > cloned to each partition just as we do with AFTER ROW triggers, but > arrange things so that if it already got fired for the parent table, > it doesn't get fired again after tuple routing. This would be a bit > tricky to get correct in cases where there are multiple levels of > partitioning involved, but it seems doable. In the meantime, my inclination is to fix the documentation to point out that AFTER triggers are allowed but BEFORE triggers are not. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018-May-03, Robert Haas wrote: > The asymmetry doesn't seem horrible to me on its own merits, but it > would lead to some odd behavior: suppose you defined a BEFORE ROW > trigger and an AFTER ROW trigger on the parent, and then inserted one > row into the parent table and a second row directly into a partition. > It seems like the BEFORE ROW trigger would fire only for the parent > insert, but the AFTER ROW trigger would fire in both cases. > > What seems like a better idea is to have the BEFORE ROW trigger get > cloned to each partition just as we do with AFTER ROW triggers, but > arrange things so that if it already got fired for the parent table, > it doesn't get fired again after tuple routing. This would be a bit > tricky to get correct in cases where there are multiple levels of > partitioning involved, but it seems doable. Hmm. I'm adding this to the open items list. I'll study this next week. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Wed, May 2, 2018 at 9:17 AM, Ashutosh Bapat wrote: > On Wed, May 2, 2018 at 11:56 AM, Amit Langote > wrote: >> But one could very well argue that BEFORE ROW triggers on the >> parent should run before performing the tuple routing and not be cloned to >> individual partitions, in which case the result would not have been the >> same. Perhaps that's the motivation behind leaving this to be considered >> later. > > +1 for that. We should associate before row triggers with the > partitioned table and not with the partitions. Those should be > executed before tuple routing (for that partition level) kicks in. But > then that would be asymetric with AFTER row trigger behaviour. From > this POV, pushing AFTER row triggers to the individual partitions > doesn't look good. The asymmetry doesn't seem horrible to me on its own merits, but it would lead to some odd behavior: suppose you defined a BEFORE ROW trigger and an AFTER ROW trigger on the parent, and then inserted one row into the parent table and a second row directly into a partition. It seems like the BEFORE ROW trigger would fire only for the parent insert, but the AFTER ROW trigger would fire in both cases. What seems like a better idea is to have the BEFORE ROW trigger get cloned to each partition just as we do with AFTER ROW triggers, but arrange things so that if it already got fired for the parent table, it doesn't get fired again after tuple routing. This would be a bit tricky to get correct in cases where there are multiple levels of partitioning involved, but it seems doable. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 5/2/18 02:26, Amit Langote wrote: > You're right. I think that's what you were also saying on the other > thread, in reply to which I directed you to this thread. I very clearly > missed that BEFORE ROW triggers are still disallowed. fixed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Wed, May 2, 2018 at 11:56 AM, Amit Langote wrote: > But one could very well argue that BEFORE ROW triggers on the > parent should run before performing the tuple routing and not be cloned to > individual partitions, in which case the result would not have been the > same. Perhaps that's the motivation behind leaving this to be considered > later. +1 for that. We should associate before row triggers with the partitioned table and not with the partitions. Those should be executed before tuple routing (for that partition level) kicks in. But then that would be asymetric with AFTER row trigger behaviour. From this POV, pushing AFTER row triggers to the individual partitions doesn't look good. Other question that comes to my mind is what happens when rows are inserted into a partition directly. Do we execute BEFORE trigger on the partitioned table? -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/05/02 14:17, Ashutosh Bapat wrote: > On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut > wrote: >> On 4/26/18 05:03, Amit Langote wrote: >>> On 2018/04/26 13:01, David Rowley wrote: The attached small patch removes the mention that partitioned tables cannot have foreign keys defined on them. This has been supported since 3de241db >>> >>> I noticed also that the item regarding row triggers might be obsolete as >>> of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take >>> care of that. >> >> committed both > > AFAIK we still don't support BEFORE ROW triggers, so removing that > entry altogether is misleading. You're right. I think that's what you were also saying on the other thread, in reply to which I directed you to this thread. I very clearly missed that BEFORE ROW triggers are still disallowed. create trigger br_insert_trig before insert on p for each row execute procedure br_insert_trigfunc(); ERROR: "p" is a partitioned table DETAIL: Partitioned tables cannot have BEFORE / FOR EACH ROW triggers. Here is a patch that adds back a line to state this particular limitation. Sorry about the earlier misleading patch. Fwiw, I am a bit surprised to see the error, but that's irrelevant now. I see that 86f575948c7 added the following comment in CreateTrigger() above the check that causes above error. /* * BEFORE triggers FOR EACH ROW are forbidden, because they would * allow the user to direct the row to another partition, which * isn't implemented in the executor. */ But if that's the only reason, I think it might be too restrictive. Allowing them would not lead to something wrong happening, afaics. To illustrate, I temporarily removed the check to allow BR triggers to be created on the parent and thus being cloned to each partition: create table p (a int) partition by list (a); create table p1 partition of p for values in (1); create or replace function br_insert_trigfunc() returns trigger as $$ begin new.a := new.a + 1; return new; end $$ language plpgsql; create trigger br_insert_trig before insert on p for each row execute procedure br_insert_trigfunc(); insert into p values (1, 'a') returning *; ERROR: new row for relation "p1" violates partition constraint DETAIL: Failing row contains (2, a). Since the same error would occur if the trigger were instead defined directly on the partition (which *is* allowed), maybe users could live with this. But one could very well argue that BEFORE ROW triggers on the parent should run before performing the tuple routing and not be cloned to individual partitions, in which case the result would not have been the same. Perhaps that's the motivation behind leaving this to be considered later. Thanks, Amit diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 0c8eb48a24..0b1948f069 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -3340,6 +3340,12 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 version. + + + BEFORE ROW triggers, if necessary, must be defined + on individual partitions, not the partitioned table. + +
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut wrote: > On 4/26/18 05:03, Amit Langote wrote: >> On 2018/04/26 13:01, David Rowley wrote: >>> The attached small patch removes the mention that partitioned tables >>> cannot have foreign keys defined on them. >>> >>> This has been supported since 3de241db >> >> I noticed also that the item regarding row triggers might be obsolete as >> of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take >> care of that. > > committed both AFAIK we still don't support BEFORE ROW triggers, so removing that entry altogether is misleading. -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/05/01 20:50, Peter Eisentraut wrote: > On 4/26/18 05:03, Amit Langote wrote: >> On 2018/04/26 13:01, David Rowley wrote: >>> The attached small patch removes the mention that partitioned tables >>> cannot have foreign keys defined on them. >>> >>> This has been supported since 3de241db >> >> I noticed also that the item regarding row triggers might be obsolete as >> of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take >> care of that. > > committed both Thank you. Regards, Amit
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 1 May 2018 at 23:50, Peter Eisentraut wrote: > committed both Thanks! -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 4/26/18 05:03, Amit Langote wrote: > On 2018/04/26 13:01, David Rowley wrote: >> The attached small patch removes the mention that partitioned tables >> cannot have foreign keys defined on them. >> >> This has been supported since 3de241db > > I noticed also that the item regarding row triggers might be obsolete as > of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take > care of that. committed both -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 26 April 2018 at 21:03, Amit Langote wrote: > I noticed also that the item regarding row triggers might be obsolete as > of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take > care of that. Thanks. I walked right past that one. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: Remove mention in docs that foreign keys on partitioned tables are not supported
On 2018/04/26 13:01, David Rowley wrote: > The attached small patch removes the mention that partitioned tables > cannot have foreign keys defined on them. > > This has been supported since 3de241db I noticed also that the item regarding row triggers might be obsolete as of 86f575948c7, thanks again to Alvaro! So, I updated your patch to take care of that. Thanks, Amit diff --git a/doc/src/sgml/ddl.sgml b/doc/src/sgml/ddl.sgml index 89735b4804..c2e4ec9ab9 100644 --- a/doc/src/sgml/ddl.sgml +++ b/doc/src/sgml/ddl.sgml @@ -3315,8 +3315,7 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 While primary keys are supported on partitioned tables, foreign - keys referencing partitioned tables are not supported, nor are foreign - key references from a partitioned table to some other table. + keys referencing partitioned tables are not supported. @@ -3340,13 +3339,6 @@ ALTER TABLE measurement ATTACH PARTITION measurement_y2008m02 version. - - - - Row triggers, if necessary, must be defined on individual partitions, - not the partitioned table. - -