Обсуждение: Remove mention in docs that foreign keys on partitioned tables arenot supported

The attached small patch removes the mention that partitioned tables
cannot have foreign keys defined on them.

This has been supported since 3de241db

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2018/04/26 13:01, David Rowley wrote:
> The attached small patch removes the mention that partitioned tables
> cannot have foreign keys defined on them.
> 
> This has been supported since 3de241db

I noticed also that the item regarding row triggers might be obsolete as
of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
care of that.

Thanks,
Amit

On 26 April 2018 at 21:03, Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> I noticed also that the item regarding row triggers might be obsolete as
> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
> care of that.

Thanks. I walked right past that one.

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 4/26/18 05:03, Amit Langote wrote:
> On 2018/04/26 13:01, David Rowley wrote:
>> The attached small patch removes the mention that partitioned tables
>> cannot have foreign keys defined on them.
>>
>> This has been supported since 3de241db
> 
> I noticed also that the item regarding row triggers might be obsolete as
> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
> care of that.

committed both

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 1 May 2018 at 23:50, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> committed both

Thanks!

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2018/05/01 20:50, Peter Eisentraut wrote:
> On 4/26/18 05:03, Amit Langote wrote:
>> On 2018/04/26 13:01, David Rowley wrote:
>>> The attached small patch removes the mention that partitioned tables
>>> cannot have foreign keys defined on them.
>>>
>>> This has been supported since 3de241db
>>
>> I noticed also that the item regarding row triggers might be obsolete as
>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>> care of that.
> 
> committed both

Thank you.

Regards,
Amit

On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 4/26/18 05:03, Amit Langote wrote:
>> On 2018/04/26 13:01, David Rowley wrote:
>>> The attached small patch removes the mention that partitioned tables
>>> cannot have foreign keys defined on them.
>>>
>>> This has been supported since 3de241db
>>
>> I noticed also that the item regarding row triggers might be obsolete as
>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>> care of that.
>
> committed both

AFAIK we still don't support BEFORE ROW triggers, so removing that
entry altogether is misleading.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/05/02 14:17, Ashutosh Bapat wrote:
> On Tue, May 1, 2018 at 5:20 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 4/26/18 05:03, Amit Langote wrote:
>>> On 2018/04/26 13:01, David Rowley wrote:
>>>> The attached small patch removes the mention that partitioned tables
>>>> cannot have foreign keys defined on them.
>>>>
>>>> This has been supported since 3de241db
>>>
>>> I noticed also that the item regarding row triggers might be obsolete as
>>> of 86f575948c7, thanks again to Alvaro!  So, I updated your patch to take
>>> care of that.
>>
>> committed both
> 
> AFAIK we still don't support BEFORE ROW triggers, so removing that
> entry altogether is misleading.

You're right.  I think that's what you were also saying on the other
thread, in reply to which I directed you to this thread.  I very clearly
missed that BEFORE ROW triggers are still disallowed.

create trigger br_insert_trig before insert on p for each row execute
procedure br_insert_trigfunc();
ERROR:  "p" is a partitioned table
DETAIL:  Partitioned tables cannot have BEFORE / FOR EACH ROW triggers.

Here is a patch that adds back a line to state this particular limitation.
Sorry about the earlier misleading patch.

Fwiw, I am a bit surprised to see the error, but that's irrelevant now.  I
see that 86f575948c7 added the following comment in CreateTrigger() above
the check that causes above error.

        /*
         * BEFORE triggers FOR EACH ROW are forbidden, because they would
         * allow the user to direct the row to another partition, which
         * isn't implemented in the executor.
         */

But if that's the only reason, I think it might be too restrictive.
Allowing them would not lead to something wrong happening, afaics.  To
illustrate, I temporarily removed the check to allow BR triggers to be
created on the parent and thus being cloned to each partition:

create table p (a int) partition by list (a);
create table p1 partition of p for values in (1);

create or replace function br_insert_trigfunc() returns trigger as
$$ begin new.a := new.a + 1; return new; end $$
language plpgsql;

create trigger br_insert_trig before insert on p for each row execute
procedure br_insert_trigfunc();

insert into p values (1, 'a') returning *;
ERROR:  new row for relation "p1" violates partition constraint
DETAIL:  Failing row contains (2, a).

Since the same error would occur if the trigger were instead defined
directly on the partition (which *is* allowed), maybe users could live
with this.  But one could very well argue that BEFORE ROW triggers on the
parent should run before performing the tuple routing and not be cloned to
individual partitions, in which case the result would not have been the
same.  Perhaps that's the motivation behind leaving this to be considered
later.

Thanks,
Amit

On Wed, May 2, 2018 at 11:56 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> But one could very well argue that BEFORE ROW triggers on the
> parent should run before performing the tuple routing and not be cloned to
> individual partitions, in which case the result would not have been the
> same.  Perhaps that's the motivation behind leaving this to be considered
> later.

+1 for that. We should associate before row triggers with the
partitioned table and not with the partitions. Those should be
executed before tuple routing (for that partition level) kicks in. But
then that would be asymetric with AFTER row trigger behaviour. From
this POV, pushing AFTER row triggers to the individual partitions
doesn't look good.

Other question that comes to my mind is what happens when rows are
inserted into a partition directly. Do we execute BEFORE trigger on
the partitioned table?

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 5/2/18 02:26, Amit Langote wrote:
> You're right.  I think that's what you were also saying on the other
> thread, in reply to which I directed you to this thread.  I very clearly
> missed that BEFORE ROW triggers are still disallowed.

fixed

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, May 2, 2018 at 9:17 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Wed, May 2, 2018 at 11:56 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> But one could very well argue that BEFORE ROW triggers on the
>> parent should run before performing the tuple routing and not be cloned to
>> individual partitions, in which case the result would not have been the
>> same.  Perhaps that's the motivation behind leaving this to be considered
>> later.
>
> +1 for that. We should associate before row triggers with the
> partitioned table and not with the partitions. Those should be
> executed before tuple routing (for that partition level) kicks in. But
> then that would be asymetric with AFTER row trigger behaviour. From
> this POV, pushing AFTER row triggers to the individual partitions
> doesn't look good.

The asymmetry doesn't seem horrible to me on its own merits, but it
would lead to some odd behavior: suppose you defined a BEFORE ROW
trigger and an AFTER ROW trigger on the parent, and then inserted one
row into the parent table and a second row directly into a partition.
It seems like the BEFORE ROW trigger would fire only for the parent
insert, but the AFTER ROW trigger would fire in both cases.

What seems like a better idea is to have the BEFORE ROW trigger get
cloned to each partition just as we do with AFTER ROW triggers, but
arrange things so that if it already got fired for the parent table,
it doesn't get fired again after tuple routing.  This would be a bit
tricky to get correct in cases where there are multiple levels of
partitioning involved, but it seems doable.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-May-03, Robert Haas wrote:

> The asymmetry doesn't seem horrible to me on its own merits, but it
> would lead to some odd behavior: suppose you defined a BEFORE ROW
> trigger and an AFTER ROW trigger on the parent, and then inserted one
> row into the parent table and a second row directly into a partition.
> It seems like the BEFORE ROW trigger would fire only for the parent
> insert, but the AFTER ROW trigger would fire in both cases.
> 
> What seems like a better idea is to have the BEFORE ROW trigger get
> cloned to each partition just as we do with AFTER ROW triggers, but
> arrange things so that if it already got fired for the parent table,
> it doesn't get fired again after tuple routing.  This would be a bit
> tricky to get correct in cases where there are multiple levels of
> partitioning involved, but it seems doable.

Hmm.  I'm adding this to the open items list.  I'll study this next
week.

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

On 2018-May-03, Robert Haas wrote:

> On Wed, May 2, 2018 at 9:17 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:

> > +1 for that. We should associate before row triggers with the
> > partitioned table and not with the partitions. Those should be
> > executed before tuple routing (for that partition level) kicks in. But
> > then that would be asymetric with AFTER row trigger behaviour. From
> > this POV, pushing AFTER row triggers to the individual partitions
> > doesn't look good.
> 
> The asymmetry doesn't seem horrible to me on its own merits, but it
> would lead to some odd behavior: suppose you defined a BEFORE ROW
> trigger and an AFTER ROW trigger on the parent, and then inserted one
> row into the parent table and a second row directly into a partition.
> It seems like the BEFORE ROW trigger would fire only for the parent
> insert, but the AFTER ROW trigger would fire in both cases.

This kind of inconsistency is what I wanted to avoid.  One of the
guiding principles here was that a partitioned table behaves just like a
regular table does; in particular, inserting directly into a partition
is an application-level optimization that must behave exactly like it
would if the insert had gone into its parent table (unless the user
explicitly makes it not so).  If we make an insertion into the partition
*not* fire the trigger that would have been fired by inserting into the
partitioned table, that's a bug.  I couldn't see a way to have the
BEFORE trigger handle that nicely, so I decided to punt on that feature.

So I stand by my original decision to disallow BEFORE triggers on
partitioned tables altogether -- we can add that as a new feature later,
keeping in mind the above ... namely to implement Robert's idea:

> What seems like a better idea is to have the BEFORE ROW trigger get
> cloned to each partition just as we do with AFTER ROW triggers, but
> arrange things so that if it already got fired for the parent table,
> it doesn't get fired again after tuple routing.  This would be a bit
> tricky to get correct in cases where there are multiple levels of
> partitioning involved, but it seems doable.

In the meantime, my inclination is to fix the documentation to point out
that AFTER triggers are allowed but BEFORE triggers are not.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> This kind of inconsistency is what I wanted to avoid.  One of the
> guiding principles here was that a partitioned table behaves just like a
> regular table does; in particular, inserting directly into a partition
> is an application-level optimization that must behave exactly like it
> would if the insert had gone into its parent table (unless the user
> explicitly makes it not so).  If we make an insertion into the partition
> *not* fire the trigger that would have been fired by inserting into the
> partitioned table, that's a bug.  I couldn't see a way to have the
> BEFORE trigger handle that nicely, so I decided to punt on that feature.

Reasonable, but ...

> In the meantime, my inclination is to fix the documentation to point out
> that AFTER triggers are allowed but BEFORE triggers are not.

... why doesn't the same problem apply to AFTER triggers that are attached
to the inheritance parent?

            regards, tom lane

On 2018-Jun-04, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > This kind of inconsistency is what I wanted to avoid.  One of the
> > guiding principles here was that a partitioned table behaves just like a
> > regular table does; in particular, inserting directly into a partition
> > is an application-level optimization that must behave exactly like it
> > would if the insert had gone into its parent table (unless the user
> > explicitly makes it not so).  If we make an insertion into the partition
> > *not* fire the trigger that would have been fired by inserting into the
> > partitioned table, that's a bug.  I couldn't see a way to have the
> > BEFORE trigger handle that nicely, so I decided to punt on that feature.
> 
> Reasonable, but ...
> 
> > In the meantime, my inclination is to fix the documentation to point out
> > that AFTER triggers are allowed but BEFORE triggers are not.
> 
> ... why doesn't the same problem apply to AFTER triggers that are attached
> to the inheritance parent?

The trigger is not executed as attached to the parent; it's only
executed as attached to the partition itself.  So you create it in the
parent, and clone so that all partitions have it; when you run the
command, only the cloned trigger is run, so in effect the trigger runs
exactly once.  Because the trigger runs *after* the target partition has
been selected, and the trigger cannot change that outcome (ie. it
cannot move the row to another partition) this works fine.

With a BEFORE trigger, running the trigger might change the target
partition, which has the potential for all kinds of trouble.  Also,
there's room to say that the before trigger should run before partition
routing is even invoked (hence the idea of running the triggers in the
partitioned table rather than the partition).  But in that case we must
not run the trigger again when executing the insertion in the chosen
partition; and we must do *something* if the user executes the command
targetting the partition rather than the parent table.

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2018-Jun-04, Tom Lane wrote:
>> ... why doesn't the same problem apply to AFTER triggers that are attached
>> to the inheritance parent?

> With a BEFORE trigger, running the trigger might change the target
> partition, which has the potential for all kinds of trouble.

Got it.  That seems like not just an implementation restriction, but
a pretty fundamental issue.

Could we solve it by saying that triggers on partitioned tables aren't
allowed to change the partitioning values?  (Or at least, not allowed
to change them in a way that changes the target partition.)

            regards, tom lane

On 2018-Jun-04, Tom Lane wrote:

> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> > On 2018-Jun-04, Tom Lane wrote:
> >> ... why doesn't the same problem apply to AFTER triggers that are attached
> >> to the inheritance parent?
> 
> > With a BEFORE trigger, running the trigger might change the target
> > partition, which has the potential for all kinds of trouble.
> 
> Got it.  That seems like not just an implementation restriction, but
> a pretty fundamental issue.
> 
> Could we solve it by saying that triggers on partitioned tables aren't
> allowed to change the partitioning values?  (Or at least, not allowed
> to change them in a way that changes the target partition.)

Yes, that would be one way forward.  In fact, IIUC what happens today if
you remove the restriction (as Amit tested and reported upthread[1]) is
that this already causes an error, because tuple routing is not re-done
after BEFORE triggers are run.

I was thinking it would be good to leave the option open (i.e. forbid
BEFORE triggers altogether) so that in the future we could implement
that case too, and avoid a backwards-incompatible behavior change.
Thinking about it again, this may not be a problem:  if you write a
trigger that works (it doesn't change the target partition) then when
forward-porting it to a version that does allow the target partition to
change, your trigger doesn't change behavior.  So maybe it's okay to
remove the restriction.  I'll test more tomorrow.

[1] https://postgr.es/m/1824bda1-0c47-abc4-8b97-e37414c52f6c@lab.ntt.co.jp

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

On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Could we solve it by saying that triggers on partitioned tables aren't
> allowed to change the partitioning values?  (Or at least, not allowed
> to change them in a way that changes the target partition.)

That seems like a somewhat-unfortunate restriction.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Could we solve it by saying that triggers on partitioned tables aren't
>> allowed to change the partitioning values?  (Or at least, not allowed
>> to change them in a way that changes the target partition.)

> That seems like a somewhat-unfortunate restriction.

Perhaps, but I'm having a hard time wrapping my mind around what the
semantics ought to be.  If a trigger on partition A changes the keys
so that the row shouldn't have gone into A at all, what then?  That
trigger should never have fired, eh?

            regards, tom lane

On Mon, Jun 4, 2018 at 4:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Jun 4, 2018 at 1:42 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Could we solve it by saying that triggers on partitioned tables aren't
>>> allowed to change the partitioning values?  (Or at least, not allowed
>>> to change them in a way that changes the target partition.)
>
>> That seems like a somewhat-unfortunate restriction.
>
> Perhaps, but I'm having a hard time wrapping my mind around what the
> semantics ought to be.  If a trigger on partition A changes the keys
> so that the row shouldn't have gone into A at all, what then?  That
> trigger should never have fired, eh?

Causality is for wimps.  :-)

I agree that it's weird if you think about a partition, but it's a lot
less strange if you think about a partitioned table.  If we're running
the trigger before tuple routing has been done, then we ought to be
able to change the results of tuple routing.

I think, in general, that we should try to pick semantics that make a
partitioned table behave like an unpartitioned table, provided that
all triggers are defined on the partitioned table itself.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 6/4/18 16:50, Tom Lane wrote:
> Perhaps, but I'm having a hard time wrapping my mind around what the
> semantics ought to be.  If a trigger on partition A changes the keys
> so that the row shouldn't have gone into A at all, what then?  That
> trigger should never have fired, eh?

The insert will result in an error and everything is rolled back, so you
do kind of get the "should never have" behavior.

It seems we ultimately have to answer the question whether we want
BEFORE triggers executed before or after tuple routing.  Both behaviors
might be useful, so perhaps we'll need two kinds of triggers.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Jun 4, 2018 at 4:50 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps, but I'm having a hard time wrapping my mind around what the
>> semantics ought to be.  If a trigger on partition A changes the keys
>> so that the row shouldn't have gone into A at all, what then?  That
>> trigger should never have fired, eh?

> Causality is for wimps.  :-)

Heh.

> I think, in general, that we should try to pick semantics that make a
> partitioned table behave like an unpartitioned table, provided that
> all triggers are defined on the partitioned table itself.

Well, then we lose the property Alvaro wanted, namely that if an
application chooses to insert directly into a partition, that's
just an optimization that changes no behavior (as long as it picked
the right partition).  Maybe this can be dodged by propagating
parent trigger definitions to the children, but it's going to be
complicated I'm afraid.

            regards, tom lane

On 2018/06/05 1:25, Alvaro Herrera wrote:
> In the meantime, my inclination is to fix the documentation to point out
> that AFTER triggers are allowed but BEFORE triggers are not.

Wasn't that already fixed by bcded2609ade6?

We don't say anything about AFTER triggers per se, but the following under
the limitations of partitioned tables:

BEFORE ROW triggers, if necessary, must be defined on individual
partitions, not the partitioned table.

Thanks,
Amit

On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/06/05 1:25, Alvaro Herrera wrote:
>> In the meantime, my inclination is to fix the documentation to point out
>> that AFTER triggers are allowed but BEFORE triggers are not.
>
> Wasn't that already fixed by bcded2609ade6?
>
> We don't say anything about AFTER triggers per se, but the following under
> the limitations of partitioned tables:
>
> BEFORE ROW triggers, if necessary, must be defined on individual
> partitions, not the partitioned table.

Thought correct that suggestion is problematic for upgrades. Probably
users will create triggers using same function on all the partitions.
After the upgrade when we start supporting BEFORE TRIGGER on
partitioned table, the user will have to drop these triggers and
create one trigger on the partitioned table to have the trigger to be
applicable to the new partitions created.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/06/05 16:41, Ashutosh Bapat wrote:
> On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> On 2018/06/05 1:25, Alvaro Herrera wrote:
>>> In the meantime, my inclination is to fix the documentation to point out
>>> that AFTER triggers are allowed but BEFORE triggers are not.
>>
>> Wasn't that already fixed by bcded2609ade6?
>>
>> We don't say anything about AFTER triggers per se, but the following under
>> the limitations of partitioned tables:
>>
>> BEFORE ROW triggers, if necessary, must be defined on individual
>> partitions, not the partitioned table.
> 
> Thought correct that suggestion is problematic for upgrades. Probably
> users will create triggers using same function on all the partitions.
> After the upgrade when we start supporting BEFORE TRIGGER on
> partitioned table, the user will have to drop these triggers and
> create one trigger on the partitioned table to have the trigger to be
> applicable to the new partitions created.

Hmm yes, maybe there is scope for rewording then.

Thanks,
Amit

On 2018-Jun-05, Amit Langote wrote:

> On 2018/06/05 16:41, Ashutosh Bapat wrote:
> > On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
> > <Langote_Amit_f8@lab.ntt.co.jp> wrote:
> >> On 2018/06/05 1:25, Alvaro Herrera wrote:
> >>> In the meantime, my inclination is to fix the documentation to point out
> >>> that AFTER triggers are allowed but BEFORE triggers are not.
> >>
> >> Wasn't that already fixed by bcded2609ade6?

Ah, yes, so it's already OK.

> > Thought correct that suggestion is problematic for upgrades. Probably
> > users will create triggers using same function on all the partitions.
> > After the upgrade when we start supporting BEFORE TRIGGER on
> > partitioned table, the user will have to drop these triggers and
> > create one trigger on the partitioned table to have the trigger to be
> > applicable to the new partitions created.
> 
> Hmm yes, maybe there is scope for rewording then.

Reading that subsection in its entirety, I'm not sure how to do better
-- it's all about restrictions that currently exist and we think we
could do better in the future, with the exception of the one about an
UPDATE/DELETE not seeing the updated version after a row migrating to
another partition.  One idea would be to split it into its own list of
issues; something like:

"The following limitations apply, and might be lifted in the future:
 - no way to create exclusion constraint
 - foreign keys cannot refer to partitioned tables
 - BEFORE row triggers are not supported

The following caveat applies to partitioned tables:
 - When an UPDATE causes a row to move ..."

In other words, make a distinction of things we expect to change soon
(which might be too optimistic), vs. others we don't expect to change.

Or we could leave it alone; any behavior change would be called out in
the future version's release notes anyway.  This is what I propose.

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

On 2018/06/06 2:08, Alvaro Herrera wrote:
> On 2018-Jun-05, Amit Langote wrote:
> 
>> On 2018/06/05 16:41, Ashutosh Bapat wrote:
>>> On Tue, Jun 5, 2018 at 1:07 PM, Amit Langote
>>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>> On 2018/06/05 1:25, Alvaro Herrera wrote:
>>>>> In the meantime, my inclination is to fix the documentation to point out
>>>>> that AFTER triggers are allowed but BEFORE triggers are not.
>>>>
>>>> Wasn't that already fixed by bcded2609ade6?
> 
> Ah, yes, so it's already OK.
> 
>>> Thought correct that suggestion is problematic for upgrades. Probably
>>> users will create triggers using same function on all the partitions.
>>> After the upgrade when we start supporting BEFORE TRIGGER on
>>> partitioned table, the user will have to drop these triggers and
>>> create one trigger on the partitioned table to have the trigger to be
>>> applicable to the new partitions created.
>>
>> Hmm yes, maybe there is scope for rewording then.
> 
> Reading that subsection in its entirety, I'm not sure how to do better
> -- it's all about restrictions that currently exist and we think we
> could do better in the future, with the exception of the one about an
> UPDATE/DELETE not seeing the updated version after a row migrating to
> another partition.  One idea would be to split it into its own list of
> issues; something like:
> 
> "The following limitations apply, and might be lifted in the future:
>  - no way to create exclusion constraint
>  - foreign keys cannot refer to partitioned tables
>  - BEFORE row triggers are not supported
> 
> The following caveat applies to partitioned tables:
>  - When an UPDATE causes a row to move ..."
> 
> In other words, make a distinction of things we expect to change soon
> (which might be too optimistic), vs. others we don't expect to change.
> 
> Or we could leave it alone; any behavior change would be called out in
> the future version's release notes anyway.  This is what I propose.

That works for me. :)

Thanks,
Amit

On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:

>  - BEFORE row triggers are not supported

I think this is fine. The existing wording suggests that the user
creates the triggers on the partitioned table, and that will be
supported always, which can lead to problems. With this description,
if the user finds out that BEFORE triggers are supported on partitions
and creates those, and we implement something to support those on
partitioned table, s/he will have to change the implementation
accordingly.

>
> The following caveat applies to partitioned tables:
>  - When an UPDATE causes a row to move ..."
>

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/06/06 20:51, Ashutosh Bapat wrote:
> The existing wording suggests that the user
> creates the triggers on the partitioned table, and that will be
> supported always, which can lead to problems.

Do you mean the following wording

"BEFORE ROW triggers, if necessary, must be defined on individual
partitions, not the partitioned table."

suggests that user *can* create triggers on the partitioned table?  I
guess I can see how one may read it that way.  How about we make it say
something like:

"BEFORE ROW triggers are not supported on partitioned tables; a user can
still define them, if necessary, on individual partitions though."

> With this description,
> if the user finds out that BEFORE triggers are supported on partitions
> and creates those, and we implement something to support those on
> partitioned table, s/he will have to change the implementation
> accordingly.

So you mean that the existing wording *encourages* users to use the
feature to create BR triggers on individual partitions whereas it
shouldn't, that is, users can find out about that feature by themselves by
trying it out?  I think the revised wording I proposed above isn't too
encouraging.

Thanks,
Amit

On Thu, Jun 7, 2018 at 7:51 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/06/06 20:51, Ashutosh Bapat wrote:
>> The existing wording suggests that the user
>> creates the triggers on the partitioned table, and that will be
>> supported always, which can lead to problems.
>
> Do you mean the following wording
>
> "BEFORE ROW triggers, if necessary, must be defined on individual
> partitions, not the partitioned table."
>
> suggests that user *can* create triggers on the partitioned table?  I
> guess I can see how one may read it that way.  How about we make it say
> something like:
>
> "BEFORE ROW triggers are not supported on partitioned tables; a user can
> still define them, if necessary, on individual partitions though."

Whichever way said, I am reading it like "We don't support BEFORE ROW
triggers on partitioned tables, but you can have them by creating
those on partitions". That is going to be a trouble for us.

>
>> With this description,
>> if the user finds out that BEFORE triggers are supported on partitions
>> and creates those, and we implement something to support those on
>> partitioned table, s/he will have to change the implementation
>> accordingly.
>
> So you mean that the existing wording *encourages* users to use the
> feature to create BR triggers on individual partitions whereas it
> shouldn't,

right.

> that is, users can find out about that feature by themselves by
> trying it out?

I didn't understand that part.

Probably we just say that BEFORE ROW triggers are not supported on a
partitioned table. It's good enough not to suggest it ourselves. If
users find out that they can create triggers on partitions and use it
that way, they may or may not have to change their implementation
later when we start supporting those. But then we didn't suggest that
they do it that way.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/06/07 14:17, Ashutosh Bapat wrote:
>> that is, users can find out about that feature by themselves by
>> trying it out?
> 
> I didn't understand that part.
> 
> Probably we just say that BEFORE ROW triggers are not supported on a
> partitioned table. It's good enough not to suggest it ourselves. If
> users find out that they can create triggers on partitions and use it
> that way, they may or may not have to change their implementation
> later when we start supporting those. But then we didn't suggest that
> they do it that way.

I don't understand why you think it's too troublesome to let the users
know that there is some way to use BR triggers with partitioning.  We
didn't do that for indexes, for example, before PG 11 introduced the
ability to create them on partitioned tables.

Thanks,
Amit

On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/06/07 14:17, Ashutosh Bapat wrote:
>>> that is, users can find out about that feature by themselves by
>>> trying it out?
>>
>> I didn't understand that part.
>>
>> Probably we just say that BEFORE ROW triggers are not supported on a
>> partitioned table. It's good enough not to suggest it ourselves. If
>> users find out that they can create triggers on partitions and use it
>> that way, they may or may not have to change their implementation
>> later when we start supporting those. But then we didn't suggest that
>> they do it that way.
>


> I don't understand why you think it's too troublesome to let the users
> know that there is some way to use BR triggers with partitioning.  We
> didn't do that for indexes, for example, before PG 11 introduced the
> ability to create them on partitioned tables.

By looking at the index keys it's easy to decide whether the two
indexes are same. When we add an index on a partitioned table in v11,
we skip creating an index on the partition if there exists an index
similar to the one being created. So, a user can have indexes on
partitions created in v10, upgrade to v11 and create an index on the
partitioned table. Nothing changes. But that's not true for a trigger.
It's not easy to check whether two triggers are same or not unless the
underlying function is same. User may or may not be using same trigger
function for all the partitions, which is more true, if the column
order differs between partitions. So, if the user has created triggers
on the partitions in v10, upgrades to v11, s/he may have to drop them
all and recreate the trigger on the partitioned table.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018-Jun-07, Ashutosh Bapat wrote:

> On Thu, Jun 7, 2018 at 10:58 AM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:

> > I don't understand why you think it's too troublesome to let the users
> > know that there is some way to use BR triggers with partitioning.  We
> > didn't do that for indexes, for example, before PG 11 introduced the
> > ability to create them on partitioned tables.
> 
> By looking at the index keys it's easy to decide whether the two
> indexes are same. When we add an index on a partitioned table in v11,
> we skip creating an index on the partition if there exists an index
> similar to the one being created. So, a user can have indexes on
> partitions created in v10, upgrade to v11 and create an index on the
> partitioned table. Nothing changes. But that's not true for a trigger.
> It's not easy to check whether two triggers are same or not unless the
> underlying function is same. User may or may not be using same trigger
> function for all the partitions, which is more true, if the column
> order differs between partitions. So, if the user has created triggers
> on the partitions in v10, upgrades to v11, s/he may have to drop them
> all and recreate the trigger on the partitioned table.

Actually, the column order doesn't matter for a trigger function,
because these don't refer to columns by number but by name.  So unless
users write trigger functions in C and use hardcoded column numbers
(extremely unlikely), I think this is not an issue.  In other words, in
the interesting cases the trigger functions are the same for all
partitions -- therefore upgrading from separate per-partition triggers
to one master trigger in the partitioned table is not going to be that
difficult, ISTM.

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

On 2018-Jun-08, Alvaro Herrera wrote:

> Actually, the column order doesn't matter for a trigger function,
> because these don't refer to columns by number but by name.  So unless
> users write trigger functions in C and use hardcoded column numbers
> (extremely unlikely), I think this is not an issue.  In other words, in
> the interesting cases the trigger functions are the same for all
> partitions -- therefore upgrading from separate per-partition triggers
> to one master trigger in the partitioned table is not going to be that
> difficult, ISTM.

One last thing.  This wording has been there since pg10; the only thing
that changed is that it now says "BEFORE ROW triggers" instead of "Row
triggers".  I don't think leaving it there one more year is going to get
us too much grief, TBH.

I'm going to leave this as an open item for one or two more weeks and
see if there's more support for Ashutosh's PoV; but if not, my proposal
is to close it without changing anything further.

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

On Sat, Jun 9, 2018 at 3:48 AM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> On 2018-Jun-08, Alvaro Herrera wrote:
>
>> Actually, the column order doesn't matter for a trigger function,
>> because these don't refer to columns by number but by name.  So unless
>> users write trigger functions in C and use hardcoded column numbers
>> (extremely unlikely), I think this is not an issue.  In other words, in
>> the interesting cases the trigger functions are the same for all
>> partitions -- therefore upgrading from separate per-partition triggers
>> to one master trigger in the partitioned table is not going to be that
>> difficult, ISTM.
>
> One last thing.  This wording has been there since pg10; the only thing
> that changed is that it now says "BEFORE ROW triggers" instead of "Row
> triggers".  I don't think leaving it there one more year is going to get
> us too much grief, TBH.

I am worried about following scenario

-- create partitioned table
create table t1 (a int, b int) partition by range(a);

-- create some tables which will be attached to the partitioned table in future
create table t1p1 (like t1);
create table t1p2 (like t1);

-- those tables have indexes
create index t1p1_a on t1p1(a);
create index t1p2_a on t1p2(a);

-- attach partitions
alter table t1 attach partition t1p1 for values from (0) to (100);
alter table t1 attach partition t1p2 for values from (100) to (200);

-- create index on partitioned table, it doesn't create new indexes on
t1p1 and t1p2 since there are already indexes on a
create index t1_a on t1(a);

-- create triggers, user may create different trigger functions one
for each partition, unless s/he understands that the tables can share
trigger functions
create function trig_t1p1() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1();
create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1();

create function trig_t1p2() returns trigger as $$ begin return new;
end;$$ language plpgsql;
create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();

When this schema gets upgraded to v12 or whatever version supports
BEFORE ROW triggers on a partitioned table and the user tries to
create a trigger on a partitioned table
1. somehow the code has to detect that there are already triggers on
the partitions so no need to create new triggers on the partitions. I
don't see how this can be done, unless the user is smart enough to use
the same trigger function for all partitions.

2. user has to drop those triggers and trigger functions and create
trigger on the partitioned table.

May be I am underestimating users but I am sure there will be some
users who will end up with scenario.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/06/12 22:22, Ashutosh Bapat wrote:
> -- create triggers, user may create different trigger functions one
> for each partition, unless s/he understands that the tables can share
> trigger functions
> create function trig_t1p1() returns trigger as $$ begin return new;
> end;$$ language plpgsql;
> create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1();
> create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1();
> 
> create function trig_t1p2() returns trigger as $$ begin return new;
> end;$$ language plpgsql;
> create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();
> 
> When this schema gets upgraded to v12 or whatever version supports
> BEFORE ROW triggers on a partitioned table and the user tries to
> create a trigger on a partitioned table
> 1. somehow the code has to detect that there are already triggers on
> the partitions so no need to create new triggers on the partitions. I
> don't see how this can be done, unless the user is smart enough to use
> the same trigger function for all partitions.
> 
> 2. user has to drop those triggers and trigger functions and create
> trigger on the partitioned table.
> 
> May be I am underestimating users but I am sure there will be some
> users who will end up with scenario.

Hmm, if a user *knowingly* makes triggers on different partitions call
different functions, then they wouldn't want to use the feature to create
the trigger on partitioned tables, because that feature implies same
trigger definition for all partitions.

Maybe, just as a reminder to users, we could mention in the docs that it
is in fact possible to call the same function from different triggers
(that is, triggers defined on different partitions) and that's what they
should do if they intend to later use the feature to define that same
trigger on the partitioned table.

Thanks,
Amit

On Thu, Jun 14, 2018 at 1:54 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> On 2018/06/12 22:22, Ashutosh Bapat wrote:
>> -- create triggers, user may create different trigger functions one
>> for each partition, unless s/he understands that the tables can share
>> trigger functions
>> create function trig_t1p1() returns trigger as $$ begin return new;
>> end;$$ language plpgsql;
>> create trigger trig_t1p1_1 before update on t1p1 execute procedure trig_t1p1();
>> create trigger trig_t1p1_2 before update on t1p1 execute procedure trig_t1p1();
>>
>> create function trig_t1p2() returns trigger as $$ begin return new;
>> end;$$ language plpgsql;
>> create trigger trig_t1p2 before update on t1p2 execute procedure trig_t1p2();
>>
>> When this schema gets upgraded to v12 or whatever version supports
>> BEFORE ROW triggers on a partitioned table and the user tries to
>> create a trigger on a partitioned table
>> 1. somehow the code has to detect that there are already triggers on
>> the partitions so no need to create new triggers on the partitions. I
>> don't see how this can be done, unless the user is smart enough to use
>> the same trigger function for all partitions.
>>
>> 2. user has to drop those triggers and trigger functions and create
>> trigger on the partitioned table.
>>
>> May be I am underestimating users but I am sure there will be some
>> users who will end up with scenario.
>
> Hmm, if a user *knowingly* makes triggers on different partitions call
> different functions, then they wouldn't want to use the feature to create
> the trigger on partitioned tables, because that feature implies same
> trigger definition for all partitions.

How many users would do that *knowingly*?

>
> Maybe, just as a reminder to users, we could mention in the docs that it
> is in fact possible to call the same function from different triggers
> (that is, triggers defined on different partitions) and that's what they
> should do if they intend to later use the feature to define that same
> trigger on the partitioned table.

+1 for something like this, but wording is important.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Mon, Jun 4, 2018 at 5:40 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think, in general, that we should try to pick semantics that make a
>> partitioned table behave like an unpartitioned table, provided that
>> all triggers are defined on the partitioned table itself.
>
> Well, then we lose the property Alvaro wanted, namely that if an
> application chooses to insert directly into a partition, that's
> just an optimization that changes no behavior (as long as it picked
> the right partition).  Maybe this can be dodged by propagating
> parent trigger definitions to the children, but it's going to be
> complicated I'm afraid.

Isn't this basically what I already proposed in
http://postgr.es/m/CA+TgmoYQD1xSM7=XrY6rv2a-W43gKpcTH76F3nSp5o2SGWeCkA@mail.gmail.com
?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>
>>  - BEFORE row triggers are not supported
>
> I think this is fine. The existing wording suggests that the user
> creates the triggers on the partitioned table, and that will be
> supported always, which can lead to problems. With this description,
> if the user finds out that BEFORE triggers are supported on partitions
> and creates those, and we implement something to support those on
> partitioned table, s/he will have to change the implementation
> accordingly.

It sounds like you want to try to hide from users the fact that they
can create triggers on the individual partitions.  I think that's the
wrong approach.  First of all, it's not going to work, because people
are going to find out about it and do it anyway.  Secondly, the
documentation's job is to tell you about the system's capabilities,
not conceal them from you.  Third, any speculation about what upgrade
problems people might have in the future is just that: speculation.
As the saying goes, it's tough to make predictions, especially about
the future.

To put that another way, if we really think nobody should do this, we
should prohibit it, not leave it out of the documentation.  But I
think prohibiting it would be a terrible idea: it would break upgrades
from existing releases and inconvenience users to no good purpose.
Very few, if any, users say, well, I don't really need a trigger on
this table, but PostgreSQL is really quite a bit faster than I need it
to be, so I think I'll add one anyway just to slow things down.  We
should assume that anyone who wants a BEFORE trigger has a good reason
for wanting it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Wed, Jun 6, 2018 at 7:51 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> On Tue, Jun 5, 2018 at 10:38 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>>
>>>  - BEFORE row triggers are not supported
>>
>> I think this is fine. The existing wording suggests that the user
>> creates the triggers on the partitioned table, and that will be
>> supported always, which can lead to problems. With this description,
>> if the user finds out that BEFORE triggers are supported on partitions
>> and creates those, and we implement something to support those on
>> partitioned table, s/he will have to change the implementation
>> accordingly.
>
> It sounds like you want to try to hide from users the fact that they
> can create triggers on the individual partitions.

No. I never said that in my mails (see [1], [2]) I object to the
explicit suggestion that they can/should create BEFORE ROW triggers on
partitions since those are not supported on the partitioned table.

>  I think that's the
> wrong approach.  First of all, it's not going to work, because people
> are going to find out about it and do it anyway.  Secondly, the
> documentation's job is to tell you about the system's capabilities,
> not conceal them from you.

Neither is documentation's job to "suggest" something that can lead to
problems in future.

[1] https://www.postgresql.org/message-id/CAFjFpRfzCVnuL0L3_qAFqr5xFN-EynbMoW4gf-2moO7gNazADA@mail.gmail.com
[2] https://www.postgresql.org/message-id/CAFjFpRfzCVnuL0L3_qAFqr5xFN-EynbMoW4gf-2moO7gNazADA%40mail.gmail.com

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/06/14 22:45, Ashutosh Bapat wrote:
> On Thu, Jun 14, 2018 at 6:49 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> It sounds like you want to try to hide from users the fact that they
>> can create triggers on the individual partitions.
> 
> No. I never said that in my mails (see [1], [2]) I object to the
> explicit suggestion that they can/should create BEFORE ROW triggers on
> partitions since those are not supported on the partitioned table.

I understand Ashutosh's worry as follows:

A workaround for the limitation that BR triggers cannot be defined on
partitioned tables yet is to define that *same* trigger on all partitions,
which works from the beginning (PG 10), so that gets the job done.  But...
some users may however fail to ensure that the trigger's definition is
exactly alike for each partition, especially they are likely to make each
trigger call a different function, although each of those functions may
have the same body of code.  When in some future release, one is able to
define the trigger on the partitioned table and does so, the trigger will
end up being created again on each partition, because the existing trigger
on each partition (after upgrading) would be different due to a different
function being called in each.

I proposed that we reword the sentence that describes this particular
limitation to warn users about that.  See if the attached patch is any
good for that.

Thanks,
Amit

On Thu, Jun 14, 2018 at 9:45 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
>> It sounds like you want to try to hide from users the fact that they
>> can create triggers on the individual partitions.
>
> No. I never said that in my mails (see [1], [2]) I object to the
> explicit suggestion that they can/should create BEFORE ROW triggers on
> partitions since those are not supported on the partitioned table.
>
>>  I think that's the
>> wrong approach.  First of all, it's not going to work, because people
>> are going to find out about it and do it anyway.  Secondly, the
>> documentation's job is to tell you about the system's capabilities,
>> not conceal them from you.
>
> Neither is documentation's job to "suggest" something that can lead to
> problems in future.

Well, perhaps what this boils down to is that I don't agree that it
can lead to problems in the future.  If the user creates a trigger on
each partition, whether they are all the same or some are different
from others, they can stick with that configuration in future
releases, too.  I don't think we're going to remove the ability to
have separate triggers on each partition; we're just going to add, as
an additional possibility, the ability to have a trigger on the
partitioned table that cascades to the individual partitions.  It's
true that if people want to get the benefit of that feature, they'll
have to change the configuration, but so what?  That's true of many
new features, and I don't think it's right to characterize that as a
problem.

By that logic, we should not have suggested that anyone use table
inheritance, because they would have to change their configuration
when we implemented table partitioning.  Indeed, switching from table
inheritance to table partitioning is much more intrusive than dropping
triggers on individual partitions and adding one on the partitioned
table.  The latter only requires DDL on existing objects, but the
former requires creating entirely new objects and moving all of your
data.

I think that the existing wording is fine and there's really no need
to change anything.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Sat, Jun 16, 2018 at 9:29 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> By that logic, we should not have suggested that anyone use table
> inheritance, because they would have to change their configuration
> when we implemented table partitioning.  Indeed, switching from table
> inheritance to table partitioning is much more intrusive than dropping
> triggers on individual partitions and adding one on the partitioned
> table.  The latter only requires DDL on existing objects, but the
> former requires creating entirely new objects and moving all of your
> data.

That's a wrong comparison. Inheritance based partitioning works even
after declarative partitioning feature is added. So, users can
continue using inheritance based partitioning if they don't want to
move to declarative partitioning. That's not true here. A user creates
a BEFORE ROW trigger on each partition that mimics partitioned table
level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
partitioned table will cascade the trigger down to each partition. If
that fails to recognize that there is already an equivalent trigger, a
partition table will get two triggers doing the same thing silently
without any warning or notice. That's fine if the trigger changes the
salaries to $50K but not OK if each of those increases salary by 10%.
The database has limited ability to recognize what a trigger is doing.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Mon, Jun 18, 2018 at 1:20 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> That's a wrong comparison. Inheritance based partitioning works even
> after declarative partitioning feature is added. So, users can
> continue using inheritance based partitioning if they don't want to
> move to declarative partitioning. That's not true here. A user creates
> a BEFORE ROW trigger on each partition that mimics partitioned table
> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
> partitioned table will cascade the trigger down to each partition. If
> that fails to recognize that there is already an equivalent trigger, a
> partition table will get two triggers doing the same thing silently
> without any warning or notice. That's fine if the trigger changes the
> salaries to $50K but not OK if each of those increases salary by 10%.
> The database has limited ability to recognize what a trigger is doing.

I don't even know what to say about that.  You are correct that if a
user creates a new trigger on a partitioned table and doesn't check
what happens to any existing triggers that they already have, bad
things might happen.  But that seems like a pretty stupid thing to do.
If you make *any* kind of critical change to your production database
without testing it, bad things may happen.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018-Jun-18, Ashutosh Bapat wrote:

> That's a wrong comparison. Inheritance based partitioning works even
> after declarative partitioning feature is added. So, users can
> continue using inheritance based partitioning if they don't want to
> move to declarative partitioning. That's not true here. A user creates
> a BEFORE ROW trigger on each partition that mimics partitioned table
> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
> partitioned table will cascade the trigger down to each partition. If
> that fails to recognize that there is already an equivalent trigger, a
> partition table will get two triggers doing the same thing silently
> without any warning or notice. That's fine if the trigger changes the
> salaries to $50K but not OK if each of those increases salary by 10%.
> The database has limited ability to recognize what a trigger is doing.

I agree with Robert that nobody in their right minds would be caught by
this problem by adding new triggers without thinking about it and
without testing them.  If you add a partitioned-table-level trigger to
raise salaries by 10% but there was already one in the partition level
that does the same thing, you'll readily notice that salaries have been
increased by 21% instead.

So like Robert I'm inclined to not change the wording in the
documentation.

What does worry me a little bit now, reading this discussion, is whether
we've made the triggers in partitions visible enough.  We'll have this
problem once we implement BEFORE ROW triggers as proposed, and I think
we already have this problem now.  Let's suppose a user creates a
duplicated after trigger:

create table parent (a int) partition by range (a);
create table child partition of parent for values from (0) to (100);
create function noise() returns trigger language plpgsql as $$ begin raise notice 'nyaa'; return null; end; $$;
create trigger trig_p after insert on parent for each row execute procedure noise();
create trigger trig_c after insert on child for each row execute procedure noise();

Now let's try it:

alvherre=# insert into child values (1);
NOTICE:  nyaa
NOTICE:  nyaa
INSERT 0 1

OK, so where does that one come from?

alvherre=# \d child
                Tabla «public.child»
 Columna │  Tipo   │ Collation │ Nullable │ Default 
─────────┼─────────┼───────────┼──────────┼─────────
 a       │ integer │           │          │ 
Partition of: parent FOR VALUES FROM (0) TO (100)
Triggers:
    trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()

Hmm, there's only one trigger here, why does it appear twice?  To find
out, you have to know where to look:

alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
 tgname │ tgrelid │ tgisinternal 
────────┼─────────┼──────────────
 trig_p │ parent  │ f
 trig_p │ child   │ t
 trig_c │ child   │ f
(3 filas)

So there is a trigger in table child, but it's hidden because
tgisinternal.  Of course, you can see it if you look at the parent's
definition:

alvherre=# \d parent
               Tabla «public.parent»
 Columna │  Tipo   │ Collation │ Nullable │ Default 
─────────┼─────────┼───────────┼──────────┼─────────
 a       │ integer │           │          │ 
Partition key: RANGE (a)
Triggers:
    trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
Number of partitions: 1 (Use \d+ to list them.)

I think it'd be useful to have a list of triggers that have been
inherited from ancestors, or maybe simply a list of internal triggers

Or maybe this is not something to worry about?

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

On Mon, Jun 18, 2018 at 12:59 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2018-Jun-18, Ashutosh Bapat wrote:
>
>> That's a wrong comparison. Inheritance based partitioning works even
>> after declarative partitioning feature is added. So, users can
>> continue using inheritance based partitioning if they don't want to
>> move to declarative partitioning. That's not true here. A user creates
>> a BEFORE ROW trigger on each partition that mimics partitioned table
>> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
>> partitioned table will cascade the trigger down to each partition. If
>> that fails to recognize that there is already an equivalent trigger, a
>> partition table will get two triggers doing the same thing silently
>> without any warning or notice. That's fine if the trigger changes the
>> salaries to $50K but not OK if each of those increases salary by 10%.
>> The database has limited ability to recognize what a trigger is doing.
>
> I agree with Robert that nobody in their right minds would be caught by
> this problem by adding new triggers without thinking about it and
> without testing them.  If you add a partitioned-table-level trigger to
> raise salaries by 10% but there was already one in the partition level
> that does the same thing, you'll readily notice that salaries have been
> increased by 21% instead.
>
> So like Robert I'm inclined to not change the wording in the
> documentation.
>
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
>
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure noise();
> create trigger trig_c after insert on child for each row execute procedure noise();
>
> Now let's try it:
>
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
>
> OK, so where does that one come from?
>
> alvherre=# \d child
>                 Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
>     trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
>
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
>
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal
> ────────┼─────────┼──────────────
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
>
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
>
> alvherre=# \d parent
>                Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │
> Partition key: RANGE (a)
> Triggers:
>     trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
>
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers
>
> Or maybe this is not something to worry about?
>

One of the things I was thinking about while reading this thread is
that the scenario of creating "duplicate" triggers on a table meaning
two triggers doing the same thing is entirely possible now but we
don't really do anything to prevent it, which is Ok. I've never seen
much merit in the argument "people should test" (it assumes a certain
infallibility that just isn't true) but we've also generally been
pretty good about exposing what is going on so people can debug this
type of unexpected behavior.

So +1 for thinking we do need to worry about it. I'm not exactly sure
how we want to expose that info; with \d+ we list various "Partition
X:" sections, perhaps adding one for "Partition triggers:" would be
enough, although I am inclined to think it needs exposure at the \d
level. One other thing to consider is firing order of said triggers...
if all parent level triggers fire before child level triggers then the
above separation is straightforward, but if the execution order is, as
I suspect, mixed, then it becomes more complicated.


Robert Treat
http://xzilla.net

Hi.

On 2018/06/19 1:59, Alvaro Herrera wrote:
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
> 
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure noise();
> create trigger trig_c after insert on child for each row execute procedure noise();
> 
> Now let's try it:
> 
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
> 
> OK, so where does that one come from?
> 
> alvherre=# \d child
>                 Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default 
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │ 
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
>     trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
> 
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
> 
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal 
> ────────┼─────────┼──────────────
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
> 
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
> 
> alvherre=# \d parent
>                Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default 
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │ 
> Partition key: RANGE (a)
> Triggers:
>     trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
> 
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers

In CreateTrigger(), 86f575948c7 did this.

-    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
+    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
in_partition);

I'm not sure why it had to be done, but undoing this change doesn't seem
to break any regression tests (neither those added by 86f575948c7 nor of
the partitioned table foreign key commit).  Did we really intend to change
the meaning of tginternal like this here?

Thanks,
Amit

On Tue, Jun 19, 2018 at 3:51 AM, Robert Treat <rob@xzilla.net> wrote:
>
> So +1 for thinking we do need to worry about it. I'm not exactly sure
> how we want to expose that info; with \d+ we list various "Partition
> X:" sections, perhaps adding one for "Partition triggers:" would be
> enough, although I am inclined to think it needs exposure at the \d
> level.

Something like this or what Alvaro proposed will be helpful.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Mon, Jun 18, 2018 at 10:29 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> On 2018-Jun-18, Ashutosh Bapat wrote:
>
>> That's a wrong comparison. Inheritance based partitioning works even
>> after declarative partitioning feature is added. So, users can
>> continue using inheritance based partitioning if they don't want to
>> move to declarative partitioning. That's not true here. A user creates
>> a BEFORE ROW trigger on each partition that mimics partitioned table
>> level BEFORE ROW trigger. The proposed BEFORE ROW trigger on
>> partitioned table will cascade the trigger down to each partition. If
>> that fails to recognize that there is already an equivalent trigger, a
>> partition table will get two triggers doing the same thing silently
>> without any warning or notice. That's fine if the trigger changes the
>> salaries to $50K but not OK if each of those increases salary by 10%.
>> The database has limited ability to recognize what a trigger is doing.
>
> I agree with Robert that nobody in their right minds would be caught by
> this problem by adding new triggers without thinking about it and
> without testing them.  If you add a partitioned-table-level trigger to
> raise salaries by 10% but there was already one in the partition level
> that does the same thing, you'll readily notice that salaries have been
> increased by 21% instead.
>
> So like Robert I'm inclined to not change the wording in the
> documentation.
>
> What does worry me a little bit now, reading this discussion, is whether
> we've made the triggers in partitions visible enough.  We'll have this
> problem once we implement BEFORE ROW triggers as proposed, and I think
> we already have this problem now.  Let's suppose a user creates a
> duplicated after trigger:
>
> create table parent (a int) partition by range (a);
> create table child partition of parent for values from (0) to (100);
> create function noise() returns trigger language plpgsql as $$ begin raise notice 'nyaa'; return null; end; $$;
> create trigger trig_p after insert on parent for each row execute procedure noise();
> create trigger trig_c after insert on child for each row execute procedure noise();
>
> Now let's try it:
>
> alvherre=# insert into child values (1);
> NOTICE:  nyaa
> NOTICE:  nyaa
> INSERT 0 1
>
> OK, so where does that one come from?
>
> alvherre=# \d child
>                 Tabla «public.child»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
>     trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
>
> Hmm, there's only one trigger here, why does it appear twice?  To find
> out, you have to know where to look:
>
> alvherre=# select tgname, tgrelid::regclass, tgisinternal from pg_trigger;
>  tgname │ tgrelid │ tgisinternal
> ────────┼─────────┼──────────────
>  trig_p │ parent  │ f
>  trig_p │ child   │ t
>  trig_c │ child   │ f
> (3 filas)
>
> So there is a trigger in table child, but it's hidden because
> tgisinternal.  Of course, you can see it if you look at the parent's
> definition:
>
> alvherre=# \d parent
>                Tabla «public.parent»
>  Columna │  Tipo   │ Collation │ Nullable │ Default
> ─────────┼─────────┼───────────┼──────────┼─────────
>  a       │ integer │           │          │
> Partition key: RANGE (a)
> Triggers:
>     trig_p AFTER INSERT ON parent FOR EACH ROW EXECUTE PROCEDURE noise()
> Number of partitions: 1 (Use \d+ to list them.)
>
> I think it'd be useful to have a list of triggers that have been
> inherited from ancestors, or maybe simply a list of internal triggers

That's a very good description of the problem people will face. Thanks
for elaborating it this way.

--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018-Jun-19, Amit Langote wrote:

> In CreateTrigger(), 86f575948c7 did this.
> 
> -    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
> +    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
> in_partition);
> 
> I'm not sure why it had to be done, but undoing this change doesn't seem
> to break any regression tests (neither those added by 86f575948c7 nor of
> the partitioned table foreign key commit).  Did we really intend to change
> the meaning of tginternal like this here?

I'm pretty sure pg_dump breaks if you turn that flag off for triggers in
partitions, because pg_dump is going to emit commands to create those
triggers, and those fail.

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

Gmail users: this comes from https://postgr.es/m/20180627191819.6g73wu7ck23fdhv6@alvherre.pgsql

On 2018-Jun-27, Alvaro Herrera wrote:

> On 2018-Jun-19, Amit Langote wrote:
> 
> > In CreateTrigger(), 86f575948c7 did this.
> > 
> > -    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal);
> > +    values[Anum_pg_trigger_tgisinternal - 1] = BoolGetDatum(isInternal ||
> > in_partition);
> > 
> > I'm not sure why it had to be done, but undoing this change doesn't seem
> > to break any regression tests (neither those added by 86f575948c7 nor of
> > the partitioned table foreign key commit).  Did we really intend to change
> > the meaning of tginternal like this here?
> 
> I'm pretty sure pg_dump breaks if you turn that flag off for triggers in
> partitions, because pg_dump is going to emit commands to create those
> triggers, and those fail.

After idlying some more on this, I think one possibility is to add
another column to pg_trigger to differentiate triggers that should not
be shown by psql nor dumped, from triggers that should be shown by psql
but not dumped.

Two possibilities:
a) a boolean flag, "trgpartition" or something like that, indicating
that this is a trigger in a partition.  pg_dump does not dump such
triggers, but psql shows them.

b) an OID, pointing to the parent trigger.  This could theoretically
help a future implementation of BEFORE ROW triggers, as discussed
before (if trigger trgparent has already run for this row, don't run the
current trigger).  However, it's easy to revamp pg_trigger definition in
pg12 to add this column and remove the boolean one, so unless there is
some other immediate use for trgparent then there's no point.

Another angle is that we're already in beta3 and there may be concerns
about altering catalog definition this late in the cycle.  Anybody?
Maybe psql can just match tgisinternal triggers by name, and if one
match occurs then we assume it's a clone that should be listed as a
partition trigger.

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

On 28 June 2018 at 09:01, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Another angle is that we're already in beta3 and there may be concerns
> about altering catalog definition this late in the cycle.  Anybody?
> Maybe psql can just match tgisinternal triggers by name, and if one
> match occurs then we assume it's a clone that should be listed as a
> partition trigger.

Are catversion bumps really a huge problem in beta?  Looks like there
were quite a few during the v10 cycle.

$ git log REL_10_BETA2..REL_10_BETA3 --oneline --
src\include\catalog\catversion.h | wc -l
      1

$ git log REL_10_BETA1..REL_10_BETA2 --oneline --
src\include\catalog\catversion.h | wc -l
      9

-- 
 David Rowley                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

On 2018-Jun-28, David Rowley wrote:

> On 28 June 2018 at 09:01, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> > Another angle is that we're already in beta3 and there may be concerns
> > about altering catalog definition this late in the cycle.  Anybody?
> > Maybe psql can just match tgisinternal triggers by name, and if one
> > match occurs then we assume it's a clone that should be listed as a
> > partition trigger.
> 
> Are catversion bumps really a huge problem in beta?  Looks like there
> were quite a few during the v10 cycle.

Hm, I remember they were somewhat of a big deal.  Maybe it's not so
anymore and just can pg_upgrade easily?  Of course, once a beta contains
one, and subsequent ones before the next beta are "free".

Your patch for partition pruning will also require one, by the looks, so
I guess it's pretty much settled ...

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

On 2018-Jun-18, Robert Treat wrote:

> One of the things I was thinking about while reading this thread is
> that the scenario of creating "duplicate" triggers on a table meaning
> two triggers doing the same thing is entirely possible now but we
> don't really do anything to prevent it, which is Ok. I've never seen
> much merit in the argument "people should test" (it assumes a certain
> infallibility that just isn't true) but we've also generally been
> pretty good about exposing what is going on so people can debug this
> type of unexpected behavior.
> 
> So +1 for thinking we do need to worry about it. I'm not exactly sure
> how we want to expose that info; with \d+ we list various "Partition
> X:" sections, perhaps adding one for "Partition triggers:" would be
> enough, although I am inclined to think it needs exposure at the \d
> level. One other thing to consider is firing order of said triggers...
> if all parent level triggers fire before child level triggers then the
> above separation is straightforward, but if the execution order is, as
> I suspect, mixed, then it becomes more complicated.

The firing order is alphabetical across the whole set, i.e. parent/child
triggers are interleaved.  See the regression test output at the bottom
of this email.

I looked into adding a "Partition triggers" section because it seemed a
nice idea, but looking at the code I realized it's a bit tough because
we already split triggers in "categories": normal triggers, disabled
triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c
line 2795).  Since the trigger being in a partition is orthogonal to
that classification, we would end up with eight groups.  Not sure that's
great.

The attached patch (catversion bump not included -- beware of server
crash if you run it without initdb'ing) keeps the categories the same.
So with my previous example setup, you can see the duplicate triggers in
psql:

alvherre=# \d child 
               Table "public.child"
 Column │  Type   │ Collation │ Nullable │ Default 
────────┼─────────┼───────────┼──────────┼─────────
 a      │ integer │           │          │ 
Partition of: parent FOR VALUES FROM (0) TO (100)
Triggers:
    trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
    trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()

and as soon as you try to drop the second one, you'll learn the truth
about it:

alvherre=# drop trigger trig_p on child;
ERROR:  cannot drop trigger trig_p on table child because trigger trig_p on table parent requires it
SUGERENCIA:  You can drop trigger trig_p on table parent instead.
Time: 1,443 ms

I say this is sufficient.


-- Verify that triggers fire in alphabetical order
create table parted_trig (a int) partition by range (a);
create table parted_trig_1 partition of parted_trig for values from (0) to (1000)
   partition by range (a);
create table parted_trig_1_1 partition of parted_trig_1 for values from (0) to (100);
create table parted_trig_2 partition of parted_trig for values from (1000) to (2000);
create trigger zzz after insert on parted_trig for each row execute procedure trigger_notice();
create trigger mmm after insert on parted_trig_1_1 for each row execute procedure trigger_notice();
create trigger aaa after insert on parted_trig_1 for each row execute procedure trigger_notice();
create trigger bbb after insert on parted_trig for each row execute procedure trigger_notice();
create trigger qqq after insert on parted_trig_1_1 for each row execute procedure trigger_notice();
insert into parted_trig values (50), (1500);
NOTICE:  trigger aaa on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger bbb on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger mmm on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger qqq on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger zzz on parted_trig_1_1 AFTER INSERT for ROW
NOTICE:  trigger bbb on parted_trig_2 AFTER INSERT for ROW
NOTICE:  trigger zzz on parted_trig_2 AFTER INSERT for ROW

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

On 2018/06/28 7:58, Alvaro Herrera wrote:
> On 2018-Jun-18, Robert Treat wrote:
>> So +1 for thinking we do need to worry about it. I'm not exactly sure
>> how we want to expose that info; with \d+ we list various "Partition
>> X:" sections, perhaps adding one for "Partition triggers:" would be
>> enough, although I am inclined to think it needs exposure at the \d
>> level. One other thing to consider is firing order of said triggers...
>> if all parent level triggers fire before child level triggers then the
>> above separation is straightforward, but if the execution order is, as
>> I suspect, mixed, then it becomes more complicated.
> 
> The firing order is alphabetical across the whole set, i.e. parent/child
> triggers are interleaved.  See the regression test output at the bottom
> of this email.
> 
> I looked into adding a "Partition triggers" section because it seemed a
> nice idea, but looking at the code I realized it's a bit tough because
> we already split triggers in "categories": normal triggers, disabled
> triggers, ALWAYS triggers and REPLICA triggers (src/bin/psql/describe.c
> line 2795).  Since the trigger being in a partition is orthogonal to
> that classification, we would end up with eight groups.  Not sure that's
> great.
> 
> The attached patch (catversion bump not included -- beware of server
> crash if you run it without initdb'ing) keeps the categories the same.

Thanks for creating the patch.

> So with my previous example setup, you can see the duplicate triggers in
> psql:
> 
> alvherre=# \d child 
>                Table "public.child"
>  Column │  Type   │ Collation │ Nullable │ Default 
> ────────┼─────────┼───────────┼──────────┼─────────
>  a      │ integer │           │          │ 
> Partition of: parent FOR VALUES FROM (0) TO (100)
> Triggers:
>     trig_c AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
>     trig_p AFTER INSERT ON child FOR EACH ROW EXECUTE PROCEDURE noise()
> 
> and as soon as you try to drop the second one, you'll learn the truth
> about it:
> 
> alvherre=# drop trigger trig_p on child;
> ERROR:  cannot drop trigger trig_p on table child because trigger trig_p on table parent requires it
> SUGERENCIA:  You can drop trigger trig_p on table parent instead.
> Time: 1,443 ms
> 
> I say this is sufficient.

Yeah.  We do same with constraints for example:

create table p (a int check (a >= 0), b int) partition by list (a);
create table p1 partition of p (b check (b >= 0)) for values in (1);
\d p1
                 Table "public.p1"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           |          |
 b      | integer |           |          |
Partition of: p FOR VALUES IN (1)
Check constraints:
    "p1_b_check" CHECK (b >= 0)
    "p_a_check" CHECK (a >= 0)

alter table p1 drop constraint p_a_check;
ERROR:  cannot drop inherited constraint "p_a_check" of relation "p1"

More specifically, inherited triggers are not listed separately.

By the way, picking up on the word "inherited" in the error message shown
above, I wonder if you decided against using similar terminology
intentionally.  I guess the thinking there is that the terminology being
used extensively with columns and constraints ("inherited column/check
constraint cannot be dropped", etc.) is just a legacy of partitioning
sharing implementation with inheritance.

Thanks,
Amit

On 6/27/18 23:01, Alvaro Herrera wrote:
> Another angle is that we're already in beta3 and there may be concerns
> about altering catalog definition this late in the cycle.  Anybody?
> Maybe psql can just match tgisinternal triggers by name, and if one
> match occurs then we assume it's a clone that should be listed as a
> partition trigger.

Couldn't psql chase the pg_depend links to find inherited triggers?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Jun-28, Peter Eisentraut wrote:

> On 6/27/18 23:01, Alvaro Herrera wrote:
> > Another angle is that we're already in beta3 and there may be concerns
> > about altering catalog definition this late in the cycle.  Anybody?
> > Maybe psql can just match tgisinternal triggers by name, and if one
> > match occurs then we assume it's a clone that should be listed as a
> > partition trigger.
> 
> Couldn't psql chase the pg_depend links to find inherited triggers?

Yeah, I thought this would be exceedingly ugly, but apparently it's not
*that* bad -- see the attached patch, which relies on the fact that
triggers don't otherwise depend on other triggers.  We don't use this
technique elsewhere in psql though.

I admit I'm inclined to go this route mostly because it's simpler.

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

On 6/28/18 22:52, Alvaro Herrera wrote:
>> Couldn't psql chase the pg_depend links to find inherited triggers?
> 
> Yeah, I thought this would be exceedingly ugly, but apparently it's not
> *that* bad -- see the attached patch, which relies on the fact that
> triggers don't otherwise depend on other triggers.  We don't use this
> technique elsewhere in psql though.

Yeah, relying on pg_depend to detect relationships between catalog
objects is a bit evil.  We do use this for detecting sequences linked to
tables, which also has its share of problems.  Ideally, there would be a
column in pg_trigger, perhaps, that makes this link explicit.  But we
are looking here for a solution without catalog changes, I believe.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018/06/29 6:23, Peter Eisentraut wrote:
> On 6/28/18 22:52, Alvaro Herrera wrote:
>>> Couldn't psql chase the pg_depend links to find inherited triggers?
>>
>> Yeah, I thought this would be exceedingly ugly, but apparently it's not
>> *that* bad -- see the attached patch, which relies on the fact that
>> triggers don't otherwise depend on other triggers.  We don't use this
>> technique elsewhere in psql though.
> 
> Yeah, relying on pg_depend to detect relationships between catalog
> objects is a bit evil.  We do use this for detecting sequences linked to
> tables, which also has its share of problems.  Ideally, there would be a
> column in pg_trigger, perhaps, that makes this link explicit.  But we
> are looking here for a solution without catalog changes, I believe.

+1 if that gets the job done.

Thanks,
Amit

On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
> By the way, picking up on the word "inherited" in the error message shown
> above, I wonder if you decided against using similar terminology
> intentionally.

Good question.

> I guess the thinking there is that the terminology being
> used extensively with columns and constraints ("inherited column/check
> constraint cannot be dropped", etc.) is just a legacy of partitioning
> sharing implementation with inheritance.

It seems to me that we can talk about things being inherited by
partitions even if we're calling the feature partitioning, rather than
inheritance.  Maybe that's confusing, but then again, maybe it's not
that confusing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 06/29/2018 02:30 PM, Robert Haas wrote:
> On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>> By the way, picking up on the word "inherited" in the error message shown
>> above, I wonder if you decided against using similar terminology
>> intentionally.
> 
> Good question.
> 
>> I guess the thinking there is that the terminology being
>> used extensively with columns and constraints ("inherited column/check
>> constraint cannot be dropped", etc.) is just a legacy of partitioning
>> sharing implementation with inheritance.
> 
> It seems to me that we can talk about things being inherited by
> partitions even if we're calling the feature partitioning, rather than
> inheritance.  Maybe that's confusing, but then again, maybe it's not
> that confusing.
> 

my 2c: I don't think it's confusing. Inheritance is a generic concept, 
not attached exclusively to the "table inheritance". If you look at docs 
from other databases, you'll see "partition inherits" all the time.

regards

-- 
Tomas Vondra                  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 2018-Jun-29, Amit Langote wrote:

> On 2018/06/29 6:23, Peter Eisentraut wrote:
> > On 6/28/18 22:52, Alvaro Herrera wrote:
> >>> Couldn't psql chase the pg_depend links to find inherited triggers?
> >>
> >> Yeah, I thought this would be exceedingly ugly, but apparently it's not
> >> *that* bad -- see the attached patch, which relies on the fact that
> >> triggers don't otherwise depend on other triggers.  We don't use this
> >> technique elsewhere in psql though.
> > 
> > Yeah, relying on pg_depend to detect relationships between catalog
> > objects is a bit evil.  We do use this for detecting sequences linked to
> > tables, which also has its share of problems.  Ideally, there would be a
> > column in pg_trigger, perhaps, that makes this link explicit.  But we
> > are looking here for a solution without catalog changes, I believe.
> 
> +1 if that gets the job done.

Okay, pushed that way.

We can redo this if/when we add support for cloning BEFORE row triggers,
which are going to need the trigger link info, I suspect.

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

On Fri, Jun 29, 2018 at 6:35 PM, Tomas Vondra
<tomas.vondra@2ndquadrant.com> wrote:
>
>
> On 06/29/2018 02:30 PM, Robert Haas wrote:
>>
>> On Wed, Jun 27, 2018 at 10:26 PM, Amit Langote
>> <Langote_Amit_f8@lab.ntt.co.jp> wrote:
>>>
>>> By the way, picking up on the word "inherited" in the error message shown
>>> above, I wonder if you decided against using similar terminology
>>> intentionally.
>>
>>
>> Good question.
>>
>>> I guess the thinking there is that the terminology being
>>> used extensively with columns and constraints ("inherited column/check
>>> constraint cannot be dropped", etc.) is just a legacy of partitioning
>>> sharing implementation with inheritance.
>>
>>
>> It seems to me that we can talk about things being inherited by
>> partitions even if we're calling the feature partitioning, rather than
>> inheritance.  Maybe that's confusing, but then again, maybe it's not
>> that confusing.
>>
>
> my 2c: I don't think it's confusing. Inheritance is a generic concept, not
> attached exclusively to the "table inheritance". If you look at docs from
> other databases, you'll see "partition inherits" all the time.

I generally agree with this. But I think we need to think more in the
context of the particular example Amit gave.

-- quoting Amit's example,

               Table "public.p1"
 Column |  Type   | Collation | Nullable | Default
--------+---------+-----------+----------+---------
 a      | integer |           |          |
 b      | integer |           |          |
Partition of: p FOR VALUES IN (1)
Check constraints:
    "p1_b_check" CHECK (b >= 0)
    "p_a_check" CHECK (a >= 0)

alter table p1 drop constraint p_a_check;
ERROR:  cannot drop inherited constraint "p_a_check" of relation "p1"

--unquote

This constraint was added to the partitioned table and inherited from
there. If user wants to drop that constraint for some reason, this
error message doesn't help. The error message tells why he can't drop
it, but doesn't tell, directly or indirectly, the user what he should
do in order to drop it. When there was only a single way, i.e table
inheritance, to "inherit" things users could probably guess that. But
now there are multiple ways to inherit things, we have to help user a
bit more. The user might figure out that ti's a partition of a table,
so the constraint is inherited from the partitioned table. But it will
help if we give a hint about from where the constraint was inherited
like the error message itself reads like "can not drop constraint
"p_a_check" on relation "p1" inherited from "partitioned table's name"
OR a hint "you may drop constraint "p_a_check" on the partitioned
table "partitioned table's name". It might even suffice to say
"partition p1" instead "relation p1" so that the user gets a clue.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On 2018/07/02 14:46, Ashutosh Bapat wrote:
> This constraint was added to the partitioned table and inherited from
> there. If user wants to drop that constraint for some reason, this
> error message doesn't help. The error message tells why he can't drop
> it, but doesn't tell, directly or indirectly, the user what he should
> do in order to drop it. When there was only a single way, i.e table
> inheritance, to "inherit" things users could probably guess that. But
> now there are multiple ways to inherit things, we have to help user a
> bit more. The user might figure out that ti's a partition of a table,
> so the constraint is inherited from the partitioned table. But it will
> help if we give a hint about from where the constraint was inherited
> like the error message itself reads like "can not drop constraint
> "p_a_check" on relation "p1" inherited from "partitioned table's name"
> OR a hint "you may drop constraint "p_a_check" on the partitioned
> table "partitioned table's name". It might even suffice to say
> "partition p1" instead "relation p1" so that the user gets a clue.

It might be a good idea to mention if the table is a partition in the HINT
message.  ERROR message can remain the same.

Thanks,
Amit

On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
<ashutosh.bapat@enterprisedb.com> wrote:
> This constraint was added to the partitioned table and inherited from
> there. If user wants to drop that constraint for some reason, this
> error message doesn't help. The error message tells why he can't drop
> it, but doesn't tell, directly or indirectly, the user what he should
> do in order to drop it.

That doesn't really sound like an actual problem to me.  If the error
is that the constraint is inherited, that suggests that the solution
is to find the place from which it got inherited and drop it there.
And that's in fact what you have to do.  What's the problem?  I mean,
we could add a hint, but it's possible to make yourself sound dumb by
giving hints that are basically obvious implications from the error
message.  Nobody wants this sort of thing:

rhaas=# drop table foo;
ERROR:  table "foo" does not exist
HINT: Try dropping a table with a different name that does exist, or
first create this table before trying to drop it.

A hint here wouldn't be as silly as that, but I think it is
unnecessary.  I doubt there's likely to be much confusion here.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2018/07/03 11:49, Robert Haas wrote:
> On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> This constraint was added to the partitioned table and inherited from
>> there. If user wants to drop that constraint for some reason, this
>> error message doesn't help. The error message tells why he can't drop
>> it, but doesn't tell, directly or indirectly, the user what he should
>> do in order to drop it.
> 
> That doesn't really sound like an actual problem to me.  If the error
> is that the constraint is inherited, that suggests that the solution
> is to find the place from which it got inherited and drop it there.
> And that's in fact what you have to do.  What's the problem?  I mean,
> we could add a hint, but it's possible to make yourself sound dumb by
> giving hints that are basically obvious implications from the error
> message.  Nobody wants this sort of thing:
> 
> rhaas=# drop table foo;
> ERROR:  table "foo" does not exist
> HINT: Try dropping a table with a different name that does exist, or
> first create this table before trying to drop it.
> 
> A hint here wouldn't be as silly as that, but I think it is
> unnecessary.  I doubt there's likely to be much confusion here.

I have to agree with that.  "cannot drop inherited ..." conveys enough for
a user to find out more.

Thanks,
Amit

On Tue, Jul 3, 2018 at 8:19 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Jul 2, 2018 at 1:46 AM, Ashutosh Bapat
> <ashutosh.bapat@enterprisedb.com> wrote:
>> This constraint was added to the partitioned table and inherited from
>> there. If user wants to drop that constraint for some reason, this
>> error message doesn't help. The error message tells why he can't drop
>> it, but doesn't tell, directly or indirectly, the user what he should
>> do in order to drop it.
>
> That doesn't really sound like an actual problem to me.  If the error
> is that the constraint is inherited, that suggests that the solution
> is to find the place from which it got inherited and drop it there.
> And that's in fact what you have to do.  What's the problem?  I mean,
> we could add a hint, but it's possible to make yourself sound dumb by
> giving hints that are basically obvious implications from the error
> message.  Nobody wants this sort of thing:
>
> rhaas=# drop table foo;
> ERROR:  table "foo" does not exist
> HINT: Try dropping a table with a different name that does exist, or
> first create this table before trying to drop it.

Again a wrong example and wrong comparison. I think I was clear about
the problem when I wrote

--
When there was only a single way, i.e table
inheritance, to "inherit" things users could probably guess that. But
now there are multiple ways to inherit things, we have to help user a
bit more. The user might figure out that ti's a partition of a table,
so the constraint is inherited from the partitioned table. But it will
help if we give a hint about from where the constraint was inherited
like the error message itself reads like "can not drop constraint
"p_a_check" on relation "p1" inherited from "partitioned table's name"
OR a hint "you may drop constraint "p_a_check" on the partitioned
table "partitioned table's name".
--

For some reason you have chosen to remove this from the email and
commented on previous part of it.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Thu, Jul 5, 2018 at 6:44 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>
> Well, as far as I know, it's up to me which parts of your emails I want to
> quote in my reply. I did read this part. It did not change my opinion.  My
> fundamental objection to your proposal is that I think it is too wordy. I
> think users will generally know whether they are using partitioning or
> inheritance, and if they don't it's not hard to figure out.   I don't think
> quoting only part of what you wrote made the quote misleading, but it did
> allow me to express my opinion.

I would usually believe that people have read complete email before
replying. But when the reply raises a question without quoting a
portion of my mail which I think has the answer, I am confused.
There's no straight forward method to avoid that confusion given it's
written and turn based communication. But I try to get clarity on that
confusion.

> I understand that you don't agree, which is
> fine, but I stand by my position.
>

I am fine with disagreement, now that I know why there's disagreement.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

On Fri, Jun 29, 2018 at 12:06 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Okay, pushed that way.
>
> We can redo this if/when we add support for cloning BEFORE row triggers,
> which are going to need the trigger link info, I suspect.

Yeah, having an explicit representation seems less likely to be
fragile.  Following dependencies might not work if there's an extra
dependency that you aren't expecting for some reason.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company