Horton

Thread

Commits

  1. Allow FOR EACH ROW triggers on partitioned tables

  2. Fix CommandCounterIncrement in partition-related DDL

  3. Mention trigger name in trigger test

  4. Fix transition tables for partition/inheritance.

  1. FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2017-12-29T22:53:19Z

    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

  2. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-03T03:12:07Z

    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

  3. Re: FOR EACH ROW triggers on partitioned tables

    Simon Riggs <simon@2ndquadrant.com> — 2018-01-06T12:54:31Z

    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

  4. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-01-23T22:10:27Z

    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

  5. Re: FOR EACH ROW triggers on partitioned tables

    Robert Haas <robertmhaas@gmail.com> — 2018-01-24T18:22:14Z

    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

  6. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-29T20:30:12Z

    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

  7. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-30T09:49:49Z

    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

  8. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-01-31T00:44:02Z

    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

  9. Re: [Sender Address Forgery]Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-01-31T00:50:50Z

    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

  10. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-02-01T15:04:12Z

    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

  11. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-02-14T21:26:24Z

    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

  12. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-02-15T07:25:54Z

    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

  13. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-02-15T21:55:25Z

    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

  14. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-02-16T02:06:12Z

    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

  15. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-02-16T20:30:15Z

    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

  16. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-02-22T23:52:12Z

    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

  17. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-02-23T01:56:47Z

    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

  18. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-02-23T02:32:40Z

    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

  19. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <amitlangote09@gmail.com> — 2018-02-23T03:36:55Z

    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

  20. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-02-23T13:15:07Z

    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

  21. Re: FOR EACH ROW triggers on partitioned tables

    Robert Haas <robertmhaas@gmail.com> — 2018-02-23T13:15:10Z

    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

  22. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-03-07T17:17:53Z

    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

  23. Re: FOR EACH ROW triggers on partitioned tables

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-03-07T20:36:17Z

    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

  24. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-03-08T01:57:08Z

    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

  25. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-08T18:06:49Z

    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

  26. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-09T13:40:08Z

    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

  27. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-09T20:07:50Z

    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

  28. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-03-09T20:41:43Z

    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

  29. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2018-03-09T21:05:14Z

    ... and this little addendum makes pg_dump work correctly.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  30. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-10T18:02:27Z

    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

  31. Re: FOR EACH ROW triggers on partitioned tables

    Thomas Munro <thomas.munro@enterprisedb.com> — 2018-03-12T02:13:13Z

    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

  32. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-12T13:39:40Z

    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

  33. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-12T14:31:08Z

    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

  34. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-21T23:18:39Z

    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

  35. Re: FOR EACH ROW triggers on partitioned tables

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2018-03-22T16:27:00Z

    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

  36. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-22T16:40:23Z

    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

  37. Re: FOR EACH ROW triggers on partitioned tables

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2018-03-23T13:49:44Z

    Pushed.  Thanks for all the review.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  38. Re: FOR EACH ROW triggers on partitioned tables

    Ashutosh Bapat <ashutosh.bapat@enterprisedb.com> — 2018-04-30T09:38:13Z

    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

  39. Re: FOR EACH ROW triggers on partitioned tables

    Amit Langote <langote_amit_f8@lab.ntt.co.jp> — 2018-05-01T01:53:59Z

    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