Обсуждение: FOR EACH ROW triggers on partitioned tables
This patch enables FOR EACH ROW triggers on partitioned tables. As presented, this patch is sufficient to discuss the semantics that we want for triggers on partitioned tables, which is the most pressing question here ISTM. However, this is incomplete: it doesn't create triggers when you do ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF. I'm using this as a basis on which to try foreign keys for partitioned tables. Getting this to committable status requires adding those features. This is essentially the same patch I posted as 0003 in https://postgr.es/m/20171220194937.pldcecyx7yrwmgkg@alvherre.pgsql -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 12/29/17 17:53, Alvaro Herrera wrote: > This patch enables FOR EACH ROW triggers on partitioned tables. > > As presented, this patch is sufficient to discuss the semantics that we > want for triggers on partitioned tables, which is the most pressing > question here ISTM. This seems pretty straightforward. What semantics questions do you have? > However, this is incomplete: it doesn't create triggers when you do > ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF. I'm using > this as a basis on which to try foreign keys for partitioned tables. > Getting this to committable status requires adding those features. Yeah that, and also perhaps preventing the removal of triggers from partitions if they are supposed to be on the whole partition hierarchy. And then make pg_dump do the right things. That's all mostly legwork, I think. Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on partitioned tables? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3 January 2018 at 03:12, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 12/29/17 17:53, Alvaro Herrera wrote: >> This patch enables FOR EACH ROW triggers on partitioned tables. >> >> As presented, this patch is sufficient to discuss the semantics that we >> want for triggers on partitioned tables, which is the most pressing >> question here ISTM. > > This seems pretty straightforward. What semantics questions do you have? I see the patch imposes these restrictions * AFTER TRIGGERS only * No transition tables * No WHEN clause All of which might be removed/extended at some later date So that's all good... there's not much here, so easy to commit soon. >> However, this is incomplete: it doesn't create triggers when you do >> ALTER TABLE ATTACH PARTITION or by CREATE TABLE PARTITION OF. I'm using >> this as a basis on which to try foreign keys for partitioned tables. >> Getting this to committable status requires adding those features. > > Yeah that, and also perhaps preventing the removal of triggers from > partitions if they are supposed to be on the whole partition hierarchy. +1 > And then make pg_dump do the right things. That's all mostly legwork, I > think. > > Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on > partitioned tables? Not sure I care about that, since it just breaks FKs and other things, but we can add it later. -- Simon Riggs http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > On 12/29/17 17:53, Alvaro Herrera wrote: > > This patch enables FOR EACH ROW triggers on partitioned tables. > > > > As presented, this patch is sufficient to discuss the semantics that we > > want for triggers on partitioned tables, which is the most pressing > > question here ISTM. > > This seems pretty straightforward. What semantics questions do you have? The main question is this: when running the trigger function, it is going to look as it is running in the context of the partition, not in the context of the parent partitioned table (TG_RELNAME etc). That seems mildly ugly: some users may be expecting that the partitioning stuff is invisible to the rest of the system, so if you have triggers on a regular table and later on decide to partition that table, the behavior of triggers will change, which is maybe unexpected. Maybe this is not really a problem, but I'm not sure and would like further opinions. Anyway, the attached v2 has the following changes 1. ALTER TABLE ATTACH PARTITION and CREATE TABLE PARTITION OF now clone any triggers from the main table, as if the trigger had been created with the partitions in place. 2. dependencies work correctly: dropping the trigger on a partition is disallowed; dropping the table removes the trigger. This is pretty much the same behavior we have for indexes in partitions; I've reused the new dependency type. While existing pg_dump tests pass, I have not verified that it does anything remotely sensible. > Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on > partitioned tables? Haven't done this yet, either. I like Simon's suggestion of outright disallowing this. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Tue, Jan 23, 2018 at 5:10 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > The main question is this: when running the trigger function, it is > going to look as it is running in the context of the partition, not in > the context of the parent partitioned table (TG_RELNAME etc). That > seems mildly ugly: some users may be expecting that the partitioning > stuff is invisible to the rest of the system, so if you have triggers on > a regular table and later on decide to partition that table, the > behavior of triggers will change, which is maybe unexpected. Maybe this > is not really a problem, but I'm not sure and would like further > opinions. It doesn't seem either great or horrible. Also, what about logical replication? Amit just raised this issue for the UPDATE row movement patch, and it seems like the issues are similar here. If somebody's counting on the same kinds of per-row triggers to fire during logical replication as we do during the original operation, they will be disappointed. >> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on >> partitioned tables? > > Haven't done this yet, either. I like Simon's suggestion of outright > disallowing this. Why not just make it work? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On 1/23/18 17:10, Alvaro Herrera wrote: > The main question is this: when running the trigger function, it is > going to look as it is running in the context of the partition, not in > the context of the parent partitioned table (TG_RELNAME etc). That > seems mildly ugly: some users may be expecting that the partitioning > stuff is invisible to the rest of the system, so if you have triggers on > a regular table and later on decide to partition that table, the > behavior of triggers will change, which is maybe unexpected. Maybe this > is not really a problem, but I'm not sure and would like further > opinions. One could go either way on this, but I think reporting the actual table partition is acceptable and preferable. If you are writing a generic trigger function, maybe to dump out all columns, you want to know the physical table and its actual columns. It's easy[citation needed] to get the partition root for a given table, if the trigger code needs that. The other way around is not possible. Some other comments are reading the patch: It seems to generally follow the patterns of the partitioned indexes patch, which is good. I think WHEN clauses on partition triggers should be OK. I don't see a reason to disallow them. Similarly, transition tables should be OK. You only get the current partition to look at, of course. The function name CloneRowTriggersOnPartition() confused me. A more accurate phrasing might be CloneRowTriggersToPartition(), or maybe reword altogether. New CommandCounterIncrement() call in AttachPartitionEnsureIndexes() should be explained. Or maybe it belongs in ATExecAttachPartition() between the calls to AttachPartitionEnsureIndexes() and CloneRowTriggersOnPartition()? Prohibition against constraint triggers is unclear. The subsequent foreign-key patches mess with that further. It's not clear to me why constraint triggers shouldn't be allowed like normal triggers. Obvious missing things: documentation, pg_dump, psql updates -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/01/30 5:30, Peter Eisentraut wrote: > On 1/23/18 17:10, Alvaro Herrera wrote: >> The main question is this: when running the trigger function, it is >> going to look as it is running in the context of the partition, not in >> the context of the parent partitioned table (TG_RELNAME etc). That >> seems mildly ugly: some users may be expecting that the partitioning >> stuff is invisible to the rest of the system, so if you have triggers on >> a regular table and later on decide to partition that table, the >> behavior of triggers will change, which is maybe unexpected. Maybe this >> is not really a problem, but I'm not sure and would like further >> opinions. > > One could go either way on this, but I think reporting the actual table > partition is acceptable and preferable. +1 > If you are writing a generic > trigger function, maybe to dump out all columns, you want to know the > physical table and its actual columns. It's easy[citation needed] to > get the partition root for a given table, if the trigger code needs > that. The other way around is not possible. I guess you mean the root where a trigger originated, that is, ancestor table on which an inherited trigger was originally defined. It is possible for a trigger to be defined on an intermediate parent and not the topmost root in a partition tree. I see that the only parent-child relationship for triggers created recursively is recorded in the form of a dependency. I wonder why not a flag in, say, pg_trigger to indicate that a trigger may have been created recursively. With the committed for inherited indexes, I can see that inheritance is explicitly recorded in pg_inherits because indexes are relations, so it's possible in the indexes' case to get the parent in which a given inherited index originated. > Similarly, transition tables should be OK. You only get the current > partition to look at, of course. +1 > The function name CloneRowTriggersOnPartition() confused me. A more > accurate phrasing might be CloneRowTriggersToPartition(), or maybe > reword altogether. CloneRowTriggers*For*Partition()? Thanks, Amit
On 1/30/18 04:49, Amit Langote wrote: >> If you are writing a generic >> trigger function, maybe to dump out all columns, you want to know the >> physical table and its actual columns. It's easy[citation needed] to >> get the partition root for a given table, if the trigger code needs >> that. The other way around is not possible. > > I guess you mean the root where a trigger originated, that is, ancestor > table on which an inherited trigger was originally defined. It is > possible for a trigger to be defined on an intermediate parent and not the > topmost root in a partition tree. OK, so maybe not so "easy". But this muddies the situation even further. You could be updating table A, which causes an update in intermediate partition B, which causes an update in leaf partition C, which fires a trigger that was logically defined on B and has a local child on C. Under this proposal, the trigger will see TG_RELNAME = C. You could make arguments that the trigger should also somehow know about B (where the trigger was defined) and A (what the user actually targeted in their statement). I'm not sure how useful these would be. But if you want to cover everything, you'll need three values. I think the patch can go ahead as proposed, and the other things could be future separate additions. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/01/31 9:44, Peter Eisentraut wrote: > On 1/30/18 04:49, Amit Langote wrote: >>> If you are writing a generic >>> trigger function, maybe to dump out all columns, you want to know the >>> physical table and its actual columns. It's easy[citation needed] to >>> get the partition root for a given table, if the trigger code needs >>> that. The other way around is not possible. >> >> I guess you mean the root where a trigger originated, that is, ancestor >> table on which an inherited trigger was originally defined. It is >> possible for a trigger to be defined on an intermediate parent and not the >> topmost root in a partition tree. > > OK, so maybe not so "easy". > > But this muddies the situation even further. You could be updating > table A, which causes an update in intermediate partition B, which > causes an update in leaf partition C, which fires a trigger that was > logically defined on B and has a local child on C. Under this proposal, > the trigger will see TG_RELNAME = C. You could make arguments that the > trigger should also somehow know about B (where the trigger was defined) > and A (what the user actually targeted in their statement). I'm not > sure how useful these would be. But if you want to cover everything, > you'll need three values. > > I think the patch can go ahead as proposed, and the other things could > be future separate additions. Yeah, I see no problem with going ahead with the patch as it for now. Thanks, Amit
Moved to next commit fest. There is some work to be done, but there appears to be a straight path to success. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Robert Haas wrote:
> On Tue, Jan 23, 2018 at 5:10 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> >> Also, does ALTER TABLE ... ENABLE/DISABLE TRIGGER do the right things on
> >> partitioned tables?
> >
> > Haven't done this yet, either. I like Simon's suggestion of outright
> > disallowing this.
>
> Why not just make it work?
I haven't had as much time to work on this as I wished, so progress has
been a bit slow. That is to say, this version is almost identical to
the one I last posted. I added a test for enable/disable trigger, which
currently fails because the code to support it is not implemented. I
added a report of the trigger name to the relevant test, for improved
visibility of what is happening. (I intend to push that one, since it's
a trivial improvement.)
Now one way to fix that would be to do as Amit suggests elsewhere, ie.,
to add a link to parent trigger from child trigger, so we can search for
children whenever the parent is disabled. We'd also need a new index on
that column so that the searches are fast, and perhaps a boolean flag
("trghaschildren") to indicate that searches must be done.
(We could add an array of children OID instead, but designwise that
seems much worse.)
Another option is to rethink this feature from the ground up: instead of
cloning catalog rows for each children, maybe we should have the trigger
lookup code, when running DML on the child relation (the partition),
obtain trigger entries not only for the child relation itself but also
for its parents recursively -- so triggers defined in the parent are
fired for the partitions, too. I'm not sure what implications this has
for constraint triggers.
The behavior should be the same, except that you cannot modify the
trigger (firing conditions, etc) on the partition individually -- it
works at the level of the whole partitioned table instead.
For foreign key triggers to work properly, I think I'd propose that this
occurs only for non-internal triggers. For internal triggers,
particularly FK triggers, we continue with the current approach in that
patch which is to create trigger clones.
This seems more promising to me.
Thoughts?
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2018/02/15 6:26, Alvaro Herrera wrote: > Another option is to rethink this feature from the ground up: instead of > cloning catalog rows for each children, maybe we should have the trigger > lookup code, when running DML on the child relation (the partition), > obtain trigger entries not only for the child relation itself but also > for its parents recursively -- so triggers defined in the parent are > fired for the partitions, too. I'm not sure what implications this has > for constraint triggers. > > The behavior should be the same, except that you cannot modify the > trigger (firing conditions, etc) on the partition individually -- it > works at the level of the whole partitioned table instead. Do you mean to fire these triggers only if the parent table (not a child table/partition) is addressed in the DML, right? If the table directly addressed in the DML is a partition whose parent has a row-level trigger, then that trigger should not get fired I suppose. Thanks, Amit
Amit Langote wrote: > On 2018/02/15 6:26, Alvaro Herrera wrote: > > Another option is to rethink this feature from the ground up: instead of > > cloning catalog rows for each children, maybe we should have the trigger > > lookup code, when running DML on the child relation (the partition), > > obtain trigger entries not only for the child relation itself but also > > for its parents recursively -- so triggers defined in the parent are > > fired for the partitions, too. I'm not sure what implications this has > > for constraint triggers. > > > > The behavior should be the same, except that you cannot modify the > > trigger (firing conditions, etc) on the partition individually -- it > > works at the level of the whole partitioned table instead. > > Do you mean to fire these triggers only if the parent table (not a child > table/partition) is addressed in the DML, right? If the table directly > addressed in the DML is a partition whose parent has a row-level trigger, > then that trigger should not get fired I suppose. No, I think that would be strange and cause data inconsistencies. Inserting directly into the partition is seen as a performance optimization (compared to inserted into the partitioned table), so we don't get to skip firing the triggers defined on the parent because the behavior would become different. In other words, the performance optimization breaks the database. Example: suppose the trigger is used to maintain an audit record trail. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 2018/02/16 6:55, Alvaro Herrera wrote: > Amit Langote wrote: >> On 2018/02/15 6:26, Alvaro Herrera wrote: >>> Another option is to rethink this feature from the ground up: instead of >>> cloning catalog rows for each children, maybe we should have the trigger >>> lookup code, when running DML on the child relation (the partition), >>> obtain trigger entries not only for the child relation itself but also >>> for its parents recursively -- so triggers defined in the parent are >>> fired for the partitions, too. I'm not sure what implications this has >>> for constraint triggers. >>> >>> The behavior should be the same, except that you cannot modify the >>> trigger (firing conditions, etc) on the partition individually -- it >>> works at the level of the whole partitioned table instead. >> >> Do you mean to fire these triggers only if the parent table (not a child >> table/partition) is addressed in the DML, right? If the table directly >> addressed in the DML is a partition whose parent has a row-level trigger, >> then that trigger should not get fired I suppose. > > No, I think that would be strange and cause data inconsistencies. > Inserting directly into the partition is seen as a performance > optimization (compared to inserted into the partitioned table), so we > don't get to skip firing the triggers defined on the parent because the > behavior would become different. In other words, the performance > optimization breaks the database. OK, that makes sense. Thanks, Amit
On 2/15/18 16:55, Alvaro Herrera wrote: > Amit Langote wrote: >> Do you mean to fire these triggers only if the parent table (not a child >> table/partition) is addressed in the DML, right? If the table directly >> addressed in the DML is a partition whose parent has a row-level trigger, >> then that trigger should not get fired I suppose. > > No, I think that would be strange and cause data inconsistencies. > Inserting directly into the partition is seen as a performance > optimization (compared to inserted into the partitioned table), so we > don't get to skip firing the triggers defined on the parent because the > behavior would become different. In other words, the performance > optimization breaks the database. > > Example: suppose the trigger is used to maintain an audit record trail. Although this situation could probably be addressed by not giving permission to write directly into the partitions, I can't think of an example where one would want a trigger that is only fired when writing into the partition root rather than into the partition directly. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera wrote: > Another option is to rethink this feature from the ground up: instead of > cloning catalog rows for each children, maybe we should have the trigger > lookup code, when running DML on the child relation (the partition), > obtain trigger entries not only for the child relation itself but also > for its parents recursively -- so triggers defined in the parent are > fired for the partitions, too. I have written this, and it seems to work fine; it's attached. Generally speaking, I like this better than my previously proposed patch: having duplicate pg_trigger rows seems lame, in hindsight. I haven't measured the performance loss, but we now scan pg_inherits each time we build a relcache entry and relhastriggers=on. Can't be good. In general, the pg_inherits stuff looks generally unnatural -- manually doing scans upwards (find parents) and downwards (find partitions). It's messy and there are no nice abstractions. Partitioning looks too much bolted-on still. We could mitigate the performance loss to some extent by adding more to RelationData. For example, a "is_partition" boolean would help: skip searching pg_inherits for a relation that is not a partition. The indexing patch already added some "has_superclass()" calls and they look somewhat out of place. Also, we could add a syscache to pg_inherits. Regarding making partitioning feel more natural, we could add some API "list all ancestors", "list all descendents". Maybe I should have used find_inheritance_children. Some cutesy: scanning multiple parents looking for potential triggers means the order of indexscan results no longer guarantees the correct ordering. I had to add a qsort() there. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 2018/02/23 8:52, Alvaro Herrera wrote: > We could mitigate the performance loss to some extent by adding more to > RelationData. For example, a "is_partition" boolean would help: skip > searching pg_inherits for a relation that is not a partition. Unless I'm missing something, doesn't rd_rel->relispartition help? Thanks, Amit
Amit Langote wrote: > On 2018/02/23 8:52, Alvaro Herrera wrote: > > We could mitigate the performance loss to some extent by adding more to > > RelationData. For example, a "is_partition" boolean would help: skip > > searching pg_inherits for a relation that is not a partition. > > Unless I'm missing something, doesn't rd_rel->relispartition help? Uh, wow, how have I missed that all this time! Yes, it probably does. I'll rework this tomorrow ... and the already committed index patch too, I think. Thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Feb 23, 2018 at 11:32 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Amit Langote wrote: >> On 2018/02/23 8:52, Alvaro Herrera wrote: >> > We could mitigate the performance loss to some extent by adding more to >> > RelationData. For example, a "is_partition" boolean would help: skip >> > searching pg_inherits for a relation that is not a partition. >> >> Unless I'm missing something, doesn't rd_rel->relispartition help? > > Uh, wow, how have I missed that all this time! Yes, it probably does. > I'll rework this tomorrow ... and the already committed index patch too, > I think. BTW, not sure if you'd noticed but I had emailed about setting relispartition on index partitions after you committed the first indexes patch. https://www.postgresql.org/message-id/12085bc4-0bc6-0f3a-4c43-57fe0681772b@lab.ntt.co.jp Thanks, Amit
Amit Langote wrote: > On Fri, Feb 23, 2018 at 11:32 AM, Alvaro Herrera > <alvherre@alvh.no-ip.org> wrote: > > Uh, wow, how have I missed that all this time! Yes, it probably does. > > I'll rework this tomorrow ... and the already committed index patch too, > > I think. > > BTW, not sure if you'd noticed but I had emailed about setting > relispartition on index partitions after you committed the first > indexes patch. I hadn't noticed. These days sadly I'm not able to keep up with all pgsql-hackers traffic, and I'm quite likely to miss things unless I'm CCed. This seems true for many others, too. Thanks for pointing it out. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Feb 22, 2018 at 6:52 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Alvaro Herrera wrote: >> Another option is to rethink this feature from the ground up: instead of >> cloning catalog rows for each children, maybe we should have the trigger >> lookup code, when running DML on the child relation (the partition), >> obtain trigger entries not only for the child relation itself but also >> for its parents recursively -- so triggers defined in the parent are >> fired for the partitions, too. > > I have written this, and it seems to work fine; it's attached. > > Generally speaking, I like this better than my previously proposed > patch: having duplicate pg_trigger rows seems lame, in hindsight. > > I haven't measured the performance loss, but we now scan pg_inherits > each time we build a relcache entry and relhastriggers=on. Can't be > good. In general, the pg_inherits stuff looks generally unnatural -- > manually doing scans upwards (find parents) and downwards (find > partitions). It's messy and there are no nice abstractions. > Partitioning looks too much bolted-on still. Elsewhere, we've put a lot of blood, sweat, and tears into making sure that we only traverse the inheritance hierarchy from top to bottom. Otherwise, we're adding deadlock hazards. I think it's categorically unacceptable to do traversals in the opposite order -- if you do, then an UPDATE on a child could deadlock with a LOCK TABLE on the parent. That will not win us any awards. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Here's another version of this patch. It is virtually identical to the previous one, except for a small doc update and whitespace changes. To recap: when a row-level trigger is created on a partitioned table, it is marked tginherits; partitions all have their pg_class row modified with relhastriggers=true. No clone of the pg_trigger row is created for the partitions. Instead, when the relcache entry for the partition is created, pg_trigger is scanned to look for entries for its ancestors. So the trigger list for a partition is created by repeatedly scanning pg_trigger and pg_inherits, until only entries with relhastriggers=f are found. I reserve the right to revise this further, as I'm going to spend a couple of hours looking at it this afternoon, particularly to see how concurrent DDL behaves, but I don't see anything obviously wrong with it. Robert Haas wrote: > Elsewhere, we've put a lot of blood, sweat, and tears into making sure > that we only traverse the inheritance hierarchy from top to bottom. > Otherwise, we're adding deadlock hazards. I think it's categorically > unacceptable to do traversals in the opposite order -- if you do, then > an UPDATE on a child could deadlock with a LOCK TABLE on the parent. > That will not win us any awards. We don't actually open relations or acquire locks in the traversal I was talking about, though; the only thing we do is scan pg_trigger using first the partition relid, then seek the ancestor(s) by scanning pg_inherits and recurse. We don't acquire locks on the involved relations, so there should be no danger of deadlocks. Changes in the definitions ought to be handled by the cache invalidations that are sent, although I admit to not having tested this specifically. I'll do that later today. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On Thu, Mar 8, 2018 at 6:17 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Here's another version of this patch. It is virtually identical to the
> previous one, except for a small doc update and whitespace changes.
What is this test for?
+create trigger failed after update on parted_trig
+ referencing old table as old_table
+ for each statement execute procedure trigger_nothing();
It doesn't fail as you apparently expected. Perhaps it was supposed
to be "for each row" so you could hit your new error with
errdetail("Triggers on partitioned tables cannot have transition
tables.")?
--
Thomas Munro
http://www.enterprisedb.com
Alvaro Herrera wrote:
> I reserve the right to revise this further, as I'm going to spend a
> couple of hours looking at it this afternoon, particularly to see how
> concurrent DDL behaves, but I don't see anything obviously wrong with
> it.
I do now. TLDR; I'm afraid this cute idea crashed and burned, so I'm
back to the idea of just cloning the pg_trigger row for each partition.
The reason for the failure is pg_trigger->tgqual, which is an expression
tree. In most cases, when set, that expression will contain references
to columns of the table, in the form of a varattno. But this varattno
references the column number of the partitioned table; and if the
partition happens to require some attribute mapping, we're screwed
because there is no way to construct that without forming the
partitioned table's tuple descriptor. But we can't do that without
grabbing a lock on the partitioned table; and we can't do that because
we would incur the deadlock risk Robert was talking about.
An example that causes the problem is:
create table parted_irreg (fd int, a int, fd2 int, b text) partition by range (b);
alter table parted_irreg drop column fd, drop column fd2;
create table parted1_irreg (b text, fd int, a int);
alter table parted1_irreg drop column fd;
alter table parted_irreg attach partition parted1_irreg for values from ('aaaa') to ('bbbb');
create trigger parted_trig after insert on parted_irreg for each row when (new.a % 1 = 0) execute procedure
trigger_notice_irreg();
insert into parted_irreg values (1, 'aardvark');
insert into parted1_irreg values ('aardwolf', 2);
drop table parted_irreg;
drop function trigger_notice_irreg();
Both inserts fail thusly:
ERROR: attribute 2 of type parted1_irreg has been dropped
Now, I can fix the first failure by taking advantage of
ResultRelInfo->ri_PartitionRoot during trigger execution; it's easy and
trouble-free to call map_variable_attnos() using that relation. But in
the second insert, ri_PartitionRoot is null (because of inserting into
the partition directly), so we have no relation to refer to for
map_variable_attnos(). I think it gets worse: if you have a three-level
partitioning scheme, and define the trigger in the second one, there is
no relation either.
Another option I think would be to always keep in the trigger descriptor
("somehow"), an open Relation on which the trigger is defined. But this
has all sorts of problems also, so I'm not doing that.
I guess another option is to store a column map somewhere.
So, unless someone has a brilliant idea on how to construct a column
mapping from partitioned table to partition, I'm going back to the
design I was proposing earlier, ie., creating individual pg_trigger rows
for each partition that are essentially adjusted copies of the ones for
the partitioned table. The only missing thing in that one was having
ALTER TABLE ENABLE/DISABLE for a trigger on the partitioned table
cascade to the partitions; I'll see about that.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Thomas Munro wrote:
> What is this test for?
>
> +create trigger failed after update on parted_trig
> + referencing old table as old_table
> + for each statement execute procedure trigger_nothing();
>
> It doesn't fail as you apparently expected. Perhaps it was supposed
> to be "for each row" so you could hit your new error with
> errdetail("Triggers on partitioned tables cannot have transition
> tables.")?
You're absolutely right. Fixed in the attached version.
I also include two requisite fixes for missing CCI calls in existing
code: one is in StorePartitionBounds which I think is backpatchable to
pg10 (this is the one that was causing me to add the one Peter
complained about in [1]), and the others are in the partition indexing
code. In terms of the current tests, the first one is necessary in
order for things to work after this patch; the ones in the second patch
I only added after code review in order to understand where the first
one was. (In that second patch I also remove one which now seems
unnecessary and in hindsight was probably there because I was lacking
the others.)
Patch 0003 is the feature at hand. Compared to v3, this version adds
some recursing logic during ENABLE/DISABLE TRIGGER, so the test that was
previously failing now works correctly.
I kept the test on "irregular" partitioning from v5, too; it works here
without any further changes.
One thing I'd like to add before claiming this committable (backend-
side) is enabling constraint triggers. AFAICT that requires a bit of
additional logic, but it shouldn't be too terrible. This would allow
for deferrable unique constraints, for example.
[1] https://postgr.es/m/171cb95a-35ec-2ace-3add-a8d16279f0bf@2ndquadrant.com
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 3/7/18 20:57, Alvaro Herrera wrote: > So, unless someone has a brilliant idea on how to construct a column > mapping from partitioned table to partition, I'm going back to the > design I was proposing earlier, ie., creating individual pg_trigger rows > for each partition that are essentially adjusted copies of the ones for > the partitioned table. Yes, that seems easiest. The idea of having only one pg_trigger entry was derived from the assumption that we wouldn't need the other ones for anything. But if that doesn't apply, then it's better to just go with the straightforward way instead of bending the single-pg_trigger way to our will. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Peter Eisentraut wrote: > On 3/7/18 20:57, Alvaro Herrera wrote: > > So, unless someone has a brilliant idea on how to construct a column > > mapping from partitioned table to partition, I'm going back to the > > design I was proposing earlier, ie., creating individual pg_trigger rows > > for each partition that are essentially adjusted copies of the ones for > > the partitioned table. > > Yes, that seems easiest. > > The idea of having only one pg_trigger entry was derived from the > assumption that we wouldn't need the other ones for anything. But if > that doesn't apply, then it's better to just go with the straightforward > way instead of bending the single-pg_trigger way to our will. Agreed. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera wrote: > One thing I'd like to add before claiming this committable (backend- > side) is enabling constraint triggers. AFAICT that requires a bit of > additional logic, but it shouldn't be too terrible. This would allow > for deferrable unique constraints, for example. v7 supports constraint triggers. I added an example using a UNIQUE DEFERRABLE constraint, and another one using plain CREATE CONSTRAINT TRIGGER. It's neat to see that the WHEN clause is executed at the time of the operation, and the trigger action is deferred (or not) till COMMIT time. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
... and this little addendum makes pg_dump work correctly. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Peter Eisentraut wrote: > On 3/7/18 20:57, Alvaro Herrera wrote: > > So, unless someone has a brilliant idea on how to construct a column > > mapping from partitioned table to partition, I'm going back to the > > design I was proposing earlier, ie., creating individual pg_trigger rows > > for each partition that are essentially adjusted copies of the ones for > > the partitioned table. > > Yes, that seems easiest. > > The idea of having only one pg_trigger entry was derived from the > assumption that we wouldn't need the other ones for anything. But if > that doesn't apply, then it's better to just go with the straightforward > way instead of bending the single-pg_trigger way to our will. I think you changed the commitfest status to "waiting on author" after posting this comment, but I had already posted an updated version which addressed this problem. I have changed it back to needs-review. thanks -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 9, 2018 at 7:06 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> Thomas Munro wrote:
>> +create trigger failed after update on parted_trig
>> + referencing old table as old_table
>> + for each statement execute procedure trigger_nothing();
>>
>> It doesn't fail as you apparently expected. Perhaps it was supposed
>> to be "for each row" so you could hit your new error with
>> errdetail("Triggers on partitioned tables cannot have transition
>> tables.")?
>
> You're absolutely right. Fixed in the attached version.
+create trigger failed after update on parted_trig
+ referencing old table as old_table
+ for each row execute procedure trigger_nothing();
+ERROR: "parted_trig" is a partitioned table
+DETAIL: Triggers on partitioned tables cannot have transition tables.
I think this should probably say "row-level". Statement-level
triggers on partitioned tables can have transition tables.
--
Thomas Munro
http://www.enterprisedb.com
On 3/9/18 16:05, Alvaro Herrera wrote: > ... and this little addendum makes pg_dump work correctly. The header file says "recursing", but the .c file calls the argument "in_partition". -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 3/9/18 15:41, Alvaro Herrera wrote: >> One thing I'd like to add before claiming this committable (backend- >> side) is enabling constraint triggers. AFAICT that requires a bit of >> additional logic, but it shouldn't be too terrible. This would allow >> for deferrable unique constraints, for example. > > v7 supports constraint triggers. I added an example using a UNIQUE > DEFERRABLE constraint, and another one using plain CREATE CONSTRAINT TRIGGER. > It's neat to see that the WHEN clause is executed at the time of the > operation, and the trigger action is deferred (or not) till COMMIT time. I'm not sure why you have the CommandCounterIncrement() changes in separate patches. It looks like there are some test cases that are essentially duplicates, e.g., +create trigger failed before insert or update or delete on parted_trig + for each row execute procedure trigger_nothing(); +create trigger trig_ins_before_1 before insert on parted_stmt_trig + for each row execute procedure trigger_notice(); Perhaps the latter is supposed to be testing statement triggers instead? Some documentation updates are needed, at least in catalogs.sgml and CREATE TRIGGER reference page. The argument names of CreateTrigger() are slightly different in the .h and .c files. I'm wondering about deferrable unique constraint triggers. In index.c, the CreateTrigger() call doesn't pass any parent trigger OID. How is this meant to work? I mean, it does work, it seems. Some comments maybe. In CloneRowTriggersToPartition(), for this piece + /* + * We only clone a) FOR EACH ROW triggers b) timed AFTER events, c) + * that are not constraint triggers. + */ + if (!TRIGGER_FOR_ROW(trigForm->tgtype) || + !TRIGGER_FOR_AFTER(trigForm->tgtype) || + OidIsValid(trigForm->tgconstraint)) + continue; I would rather have some elog(ERROR)'s if it finds triggers it can't support instead of silently skipping them. What is the story with transition tables? Why are they not supported? I don't understand this comment in CreateTrigger(): + /* + * Disallow use of transition tables. If this partitioned table + * has any partitions, the error would occur below; but if it + * doesn't then we would only hit that code when the first CREATE + * TABLE ... PARTITION OF is executed, which is too late. Check + * early to avoid the problem. + */ Earlier in the thread, others have indicated that transition tables should work. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Here's v8, which addresses all your comments except the doc updates. I added a few more tests, too. Thanks for the review! I intend to commit this shortly, probably not before Friday to give some more time for others to review/comment. Some notes: Peter Eisentraut wrote: > I'm not sure why you have the CommandCounterIncrement() changes in > separate patches. Clearly it was wise to have it separately, because it was not entirely trivial to fix the unexpected fallout :-) > I'm wondering about deferrable unique constraint triggers. In index.c, > the CreateTrigger() call doesn't pass any parent trigger OID. How is > this meant to work? I mean, it does work, it seems. Some comments maybe. Yeah, it seems pretty complicated ... it already worked this way: if you don't pass a constraint OID, the constraint is created internally. We make use of that here. > What is the story with transition tables? Why are they not supported? > I don't understand this comment in CreateTrigger(): > > + /* > + * Disallow use of transition tables. If this partitioned table > + * has any partitions, the error would occur below; but if it > + * doesn't then we would only hit that code when the first CREATE > + * TABLE ... PARTITION OF is executed, which is too late. Check > + * early to avoid the problem. > + */ > > Earlier in the thread, others have indicated that transition tables > should work. Yeah, this is a pre-existing restriction actually -- it was purposefully introduced by commit 501ed02cf6f4. Maybe it can be lifted, but I don't think it's this patch's job to do so. I reworded this comment. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
On 3/21/18 19:18, Alvaro Herrera wrote: > Here's v8, which addresses all your comments except the doc updates. I > added a few more tests, too. Thanks for the review! I intend to commit > this shortly, probably not before Friday to give some more time for > others to review/comment. Looks good, does what it needs to do. A small fixup attached. In particular, I renamed one trigger from "f", which created confusing output, looking like a boolean column. This comment in the tests I don't understand: -- verify that the FOR UPDATE OF (columns) is propagated correctly I don't see how this applies to the tests that follow. Does this have something to do with the subsequent foreign keys patch perhaps? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Вложения
Peter Eisentraut wrote: > On 3/21/18 19:18, Alvaro Herrera wrote: > > Here's v8, which addresses all your comments except the doc updates. I > > added a few more tests, too. Thanks for the review! I intend to commit > > this shortly, probably not before Friday to give some more time for > > others to review/comment. > > Looks good, does what it needs to do. > > A small fixup attached. In particular, I renamed one trigger from "f", > which created confusing output, looking like a boolean column. Thanks! > This comment in the tests I don't understand: > > -- verify that the FOR UPDATE OF (columns) is propagated correctly > > I don't see how this applies to the tests that follow. Does this have > something to do with the subsequent foreign keys patch perhaps? Not at all ... I meant "AFTER UPDATE OF columns" (used as a firing event). Not sure how I typo'ed it that badly. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Pushed. Thanks for all the review. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Mar 23, 2018 at 7:19 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Pushed. Thanks for all the review. > https://www.postgresql.org/docs/devel/static/ddl-partitioning.html section 5.10.2.3 still says "Row triggers, if necessary, must be defined on individual partitions, not the partitioned table." Should that change? Per commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96, we are supporting only AFTER row triggers. May be we should change the above line to "Before row triggers, if necessary, must ....". -- Best Wishes, Ashutosh Bapat EnterpriseDB Corporation The Postgres Database Company
On 2018/04/30 18:38, Ashutosh Bapat wrote: > On Fri, Mar 23, 2018 at 7:19 PM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: >> Pushed. Thanks for all the review. >> > > https://www.postgresql.org/docs/devel/static/ddl-partitioning.html > > section 5.10.2.3 still says > "Row triggers, if necessary, must be defined on individual partitions, > not the partitioned table." > > Should that change? > > Per commit 86f575948c773b0ec5b0f27066e37dd93a7f0a96, we are supporting > only AFTER row triggers. May be we should change the above line to > "Before row triggers, if necessary, must ....". A patch to fix that has been posted. https://www.postgresql.org/message-id/9386c128-1131-d115-cda5-63ac88d15db1%40lab.ntt.co.jp Thanks, Amit