Thread: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables
This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH FOR" was first allowed on partition tables (86f575948). I thought this would work like partitioned indexes (8b08f7d48), where detaching a partition makes its index non-inherited, and attaching a partition marks a pre-existing, matching partition as inherited rather than creating a new one. DROP TABLE t, t1; CREATE TABLE t(i int)PARTITION BY RANGE(i); CREATE TABLE t1 PARTITION OF t FOR VALUES FROM(1)TO(2); CREATE OR REPLACE FUNCTION trigf() RETURNS trigger LANGUAGE plpgsql AS $$ BEGIN END $$; CREATE TRIGGER trig AFTER INSERT ON t FOR EACH ROW EXECUTE FUNCTION trigf(); SELECT tgrelid::regclass, * FROM pg_trigger WHERE tgrelid='t1'::regclass; ALTER TABLE t DETACH PARTITION t1; ALTER TABLE t ATTACH PARTITION t1 FOR VALUES FROM (1)TO(2); ERROR: trigger "trig" for relation "t1" already exists DROP TRIGGER trig ON t1; ERROR: cannot drop trigger trig on table t1 because trigger trig on table t requires it HINT: You can drop trigger trig on table t instead. I remember these, but they don't seem to be relevant to this issue, which seems to be independant. 1fa846f1c9 Fix cloning of row triggers to sub-partitions b9b408c487 Record parents of triggers The commit for partitioned indexes talks about using an pre-existing index on the child as a "convenience gadget", puts indexes into pg_inherit, and introduces "ALTER INDEX..ATTACH PARTITION" and "CREATE INDEX..ON ONLY". It's probably rare for a duplicate index to be useful (unless rebuilding to be more optimal, which is probably not reasonably interspersed with altering inheritence). But I don't know if that's equally true for triggers. So I'm not sure what the intended behavior is, so I've stopped after implementing a partial fix.
Attachment
On 2020-Apr-08, Justin Pryzby wrote: > This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH FOR" > was first allowed on partition tables (86f575948). > > I thought this would work like partitioned indexes (8b08f7d48), where detaching > a partition makes its index non-inherited, and attaching a partition marks a > pre-existing, matching partition as inherited rather than creating a new one. Hmm. Let's agree to what behavior we want, and then we implement that. It seems to me there are two choices: 1. on detach, keep the trigger but make it independent of the trigger on parent. (This requires that the trigger is made dependent on the trigger on parent, if the table is attached as partition again; otherwise you'd end up with multiple copies of the trigger if you detach/attach multiple times). 2. on detach, remove the trigger from the partition. I think (2) is easier to implement, but (1) is the more convenient behavior. (The current behavior is obviously a bug.) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 08, 2020 at 12:02:39PM -0400, Alvaro Herrera wrote: > On 2020-Apr-08, Justin Pryzby wrote: > > > This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH FOR" > > was first allowed on partition tables (86f575948). > > > > I thought this would work like partitioned indexes (8b08f7d48), where detaching > > a partition makes its index non-inherited, and attaching a partition marks a > > pre-existing, matching partition as inherited rather than creating a new one. > > Hmm. Let's agree to what behavior we want, and then we implement that. > It seems to me there are two choices: > > 1. on detach, keep the trigger but make it independent of the trigger on > parent. (This requires that the trigger is made dependent on the > trigger on parent, if the table is attached as partition again; > otherwise you'd end up with multiple copies of the trigger if you > detach/attach multiple times). > > 2. on detach, remove the trigger from the partition. > > I think (2) is easier to implement, but (1) is the more convenient > behavior. At telsasoft, we don't care (we uninherit tables before ALTERing parents to avoid disruptive locking and to avoid worst-case disk use). (1) is consistent with the behavior for indexes, which is a slight advantage for users' ability to understand and keep track of the behavior. But adding triggers is pretty different so I'm not sure it's a totally compelling parallel. -- Justin
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Hmm. Let's agree to what behavior we want, and then we implement that.
> It seems to me there are two choices:
> 1. on detach, keep the trigger but make it independent of the trigger on
> parent. (This requires that the trigger is made dependent on the
> trigger on parent, if the table is attached as partition again;
> otherwise you'd end up with multiple copies of the trigger if you
> detach/attach multiple times).
> 2. on detach, remove the trigger from the partition.
> I think (2) is easier to implement, but (1) is the more convenient
> behavior.
I think that #1 would soon lead to needing all the same infrastructure
as we have for inherited columns and constraints, ie triggers would need
equivalents of attislocal and attinhcount. I don't really want to go
there, so I'd vote for #2.
regards, tom lane
On 2020-Apr-08, Tom Lane wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > Hmm. Let's agree to what behavior we want, and then we implement that. > > It seems to me there are two choices: > > > 1. on detach, keep the trigger but make it independent of the trigger on > > parent. (This requires that the trigger is made dependent on the > > trigger on parent, if the table is attached as partition again; > > otherwise you'd end up with multiple copies of the trigger if you > > detach/attach multiple times). > > > 2. on detach, remove the trigger from the partition. > > > I think (2) is easier to implement, but (1) is the more convenient > > behavior. > > I think that #1 would soon lead to needing all the same infrastructure > as we have for inherited columns and constraints, ie triggers would need > equivalents of attislocal and attinhcount. I don't really want to go > there, so I'd vote for #2. Hmm. Those things are used for the legacy inheritance case supporting multiple inheritance, where we need to figure out which parent the table is being detached (disinherited) from. But for partitioning we know which parent it is, since there can only be one. So I don't think that argument applies. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Apr-08, Tom Lane wrote:
>> I think that #1 would soon lead to needing all the same infrastructure
>> as we have for inherited columns and constraints, ie triggers would need
>> equivalents of attislocal and attinhcount. I don't really want to go
>> there, so I'd vote for #2.
> Hmm. Those things are used for the legacy inheritance case supporting
> multiple inheritance, where we need to figure out which parent the table
> is being detached (disinherited) from. But for partitioning we know
> which parent it is, since there can only be one. So I don't think that
> argument applies.
My point is that so long as you only allow the case of exactly one parent,
you can just delete the child trigger, because it must belong to that
parent. As soon as there's any flexibility, you are going to end up
reinventing all the stuff we had to invent to manage
maybe-or-maybe-not-inherited columns. So I think the "detach" idea
is the first step on that road, and I counsel not taking that step.
(This implies that when creating a child trigger, we should error out,
*not* allow the case, if there's already a trigger by that name. Not
sure if that's what happens today, but again I'd say that's what we
should do to avoid complicated cases.)
regards, tom lane
On Thu, Apr 9, 2020 at 3:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > Alvaro Herrera <alvherre@2ndquadrant.com> writes: > > On 2020-Apr-08, Tom Lane wrote: > >> I think that #1 would soon lead to needing all the same infrastructure > >> as we have for inherited columns and constraints, ie triggers would need > >> equivalents of attislocal and attinhcount. I don't really want to go > >> there, so I'd vote for #2. > > > Hmm. Those things are used for the legacy inheritance case supporting > > multiple inheritance, where we need to figure out which parent the table > > is being detached (disinherited) from. But for partitioning we know > > which parent it is, since there can only be one. So I don't think that > > argument applies. > > My point is that so long as you only allow the case of exactly one parent, > you can just delete the child trigger, because it must belong to that > parent. As soon as there's any flexibility, you are going to end up > reinventing all the stuff we had to invent to manage > maybe-or-maybe-not-inherited columns. So I think the "detach" idea > is the first step on that road, and I counsel not taking that step. As mentioned upthread, we have behavior #1 for indexes (attach existing / detach & keep), without any of the *islocal, *inhcount infrastructure. It is a bit complex, because we need logic to check the equivalence of an existing index on the partition being attached, so implementing the same behavior for trigger is going to have to be almost as complex. Considering that #2 will be much simpler to implement, but would be asymmetric with everything else. -- Amit Langote EnterpriseDB: http://www.enterprisedb.com
Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Apr 9, 2020 at 3:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> My point is that so long as you only allow the case of exactly one parent,
>> you can just delete the child trigger, because it must belong to that
>> parent. As soon as there's any flexibility, you are going to end up
>> reinventing all the stuff we had to invent to manage
>> maybe-or-maybe-not-inherited columns. So I think the "detach" idea
>> is the first step on that road, and I counsel not taking that step.
> As mentioned upthread, we have behavior #1 for indexes (attach
> existing / detach & keep), without any of the *islocal, *inhcount
> infrastructure. It is a bit complex, because we need logic to check
> the equivalence of an existing index on the partition being attached,
> so implementing the same behavior for trigger is going to have to be
> almost as complex. Considering that #2 will be much simpler to
> implement, but would be asymmetric with everything else.
I think there is justification for jumping through some hoops for
indexes, because they can be extremely expensive to recreate.
The same argument doesn't hold even a little bit for child
triggers, though.
Also it can be expected that an index will still behave sensibly after
its table is standalone, whereas that's far from obvious for a trigger
that was meant to work on partition member tables.
regards, tom lane
On Thu, Apr 09, 2020 at 09:46:38AM -0400, Tom Lane wrote: > Amit Langote <amitlangote09@gmail.com> writes: > > On Thu, Apr 9, 2020 at 3:09 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> My point is that so long as you only allow the case of exactly one parent, > >> you can just delete the child trigger, because it must belong to that > >> parent. As soon as there's any flexibility, you are going to end up > >> reinventing all the stuff we had to invent to manage > >> maybe-or-maybe-not-inherited columns. So I think the "detach" idea > >> is the first step on that road, and I counsel not taking that step. > > > As mentioned upthread, we have behavior #1 for indexes (attach > > existing / detach & keep), without any of the *islocal, *inhcount > > infrastructure. It is a bit complex, because we need logic to check > > the equivalence of an existing index on the partition being attached, > > so implementing the same behavior for trigger is going to have to be > > almost as complex. Considering that #2 will be much simpler to > > implement, but would be asymmetric with everything else. > > I think there is justification for jumping through some hoops for > indexes, because they can be extremely expensive to recreate. > The same argument doesn't hold even a little bit for child > triggers, though. > > Also it can be expected that an index will still behave sensibly after > its table is standalone, whereas that's far from obvious for a trigger > that was meant to work on partition member tables. I haven't heard a compelling argument for or against either way. Maybe the worst behavior might be if, during ATTACH, we searched for a matching trigger, and "merged" it (marked it inherited) if it matched. That could be bad if someone *wanted* two triggers, which seems unlikely, but to each their own. I implemented the simple way (and, as an experiment, 75% of the hard way). It occured to me that we don't currently distinguish between a trigger on a child table, and a trigger on a parent table which was recursively created on a child. That makes sense for indexes and constraints, since the objects persist if the table is detached, so it doesn't matter how it was defined. But if we remove trigger during DETACH, then it's *not* the same as a trigger that was defined on the child, and I suggest that in v13 we should make that visible. -- Justin
Attachment
v3 fixes a brown-paper-bag logic error. -- Justin
Attachment
On 2020-Apr-18, Justin Pryzby wrote: > I haven't heard a compelling argument for or against either way. > > Maybe the worst behavior might be if, during ATTACH, we searched for a matching > trigger, and "merged" it (marked it inherited) if it matched. That could be > bad if someone *wanted* two triggers, which seems unlikely, but to each their > own. I think the simplicity argument trumps the other ones, so I agree to go with your v3 patch proposed downthread. What happens if you detach the parent? I mean, should the trigger removal recurse to children? > It occured to me that we don't currently distinguish between a trigger on a > child table, and a trigger on a parent table which was recursively created on a > child. That makes sense for indexes and constraints, since the objects persist > if the table is detached, so it doesn't matter how it was defined. > > But if we remove trigger during DETACH, then it's *not* the same as a trigger > that was defined on the child, and I suggest that in v13 we should make that > visible. Hmm, interesting point -- whether the trigger is partition or not is important because it affects what happens on detach. I agree that we should make it visible. Is the proposed single bit "PARTITION" good enough, or should we indicate what's the ancestor table that defines the partition? -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Apr 08, 2020 at 11:44:33AM -0500, Justin Pryzby wrote: > On Wed, Apr 08, 2020 at 12:02:39PM -0400, Alvaro Herrera wrote: > > On 2020-Apr-08, Justin Pryzby wrote: > > > > > This seems to be a bug in master, v12, and (probably) v11, where "FOR EACH FOR" > > > was first allowed on partition tables (86f575948). > > > > > > I thought this would work like partitioned indexes (8b08f7d48), where detaching > > > a partition makes its index non-inherited, and attaching a partition marks a > > > pre-existing, matching partition as inherited rather than creating a new one. > > > > Hmm. Let's agree to what behavior we want, and then we implement that. > > It seems to me there are two choices: > > > > 1. on detach, keep the trigger but make it independent of the trigger on > > parent. (This requires that the trigger is made dependent on the > > trigger on parent, if the table is attached as partition again; > > otherwise you'd end up with multiple copies of the trigger if you > > detach/attach multiple times). > > > > 2. on detach, remove the trigger from the partition. > > > > I think (2) is easier to implement, but (1) is the more convenient > > behavior. > > At telsasoft, we don't care (we uninherit tables before ALTERing parents to > avoid disruptive locking and to avoid worst-case disk use). I realized that I was wrong about what would be most desirable for us, for an uncommon case: Our loader INSERTs into the child table, not the parent (I think I did that to try to implement UPSERT before partitioned indexes in v11). All but the newest partitions are DETACHed when we need to promote a column. It's probably rare that we'd be inserting into a table old enough to be detached, and normally that would be ok, but if a trigger were missing, it would misbehave. In our use-case, we're creating trigger on the parent as a convenient way to maintain them on the partitions, which doesn't work if a table exists but detached.. So we'd actually prefer the behavior of indexes/constraints, where the trigger is preserved if the child is detached. I'm not requesting to do that just for our use case, which may be atypical or not a good model, but adding our one data point. -- Justin
On 2020-Apr-19, Justin Pryzby wrote: > It's probably rare that we'd be inserting into a table old enough to be > detached, and normally that would be ok, but if a trigger were missing, it > would misbehave. In our use-case, we're creating trigger on the parent as a > convenient way to maintain them on the partitions, which doesn't work if a > table exists but detached.. > > So we'd actually prefer the behavior of indexes/constraints, where the trigger > is preserved if the child is detached. I'm not requesting to do that just for > our use case, which may be atypical or not a good model, but adding our one > data point. I think the easiest way to implement this is to have two triggers -- the one that's direct in the partition checks whether the table is a partition and does nothing in that case. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote: > On 2020-Apr-18, Justin Pryzby wrote: > > I haven't heard a compelling argument for or against either way. > > > > Maybe the worst behavior might be if, during ATTACH, we searched for a matching > > trigger, and "merged" it (marked it inherited) if it matched. That could be > > bad if someone *wanted* two triggers, which seems unlikely, but to each their > > own. > > I think the simplicity argument trumps the other ones, so I agree to go > with your v3 patch proposed downthread. > > What happens if you detach the parent? I mean, should the trigger > removal recurse to children? It think it should probably exactly undo what CloneRowTriggersToPartition does. ..and I guess you're trying to politely say that it didn't. I tried to fix in v4 - please check if that's right. > > It occured to me that we don't currently distinguish between a trigger on a > > child table, and a trigger on a parent table which was recursively created on a > > child. That makes sense for indexes and constraints, since the objects persist > > if the table is detached, so it doesn't matter how it was defined. > > > > But if we remove trigger during DETACH, then it's *not* the same as a trigger > > that was defined on the child, and I suggest that in v13 we should make that > > visible. > > Hmm, interesting point -- whether the trigger is partition or not is > important because it affects what happens on detach. I agree that we > should make it visible. Is the proposed single bit "PARTITION" good > enough, or should we indicate what's the ancestor table that defines the > partition? Yea, it's an obvious thing to do. One issue is that tgparentid is new, so showing the partition status of the trigger when connected to an pre-13 server would be nontrivial: b9b408c48. -- Justin
Attachment
On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote:
> > On 2020-Apr-18, Justin Pryzby wrote:
> > > I haven't heard a compelling argument for or against either way.
> > >
> > > Maybe the worst behavior might be if, during ATTACH, we searched for a matching
> > > trigger, and "merged" it (marked it inherited) if it matched. That could be
> > > bad if someone *wanted* two triggers, which seems unlikely, but to each their
> > > own.
> >
> > I think the simplicity argument trumps the other ones, so I agree to go
> > with your v3 patch proposed downthread.
> >
> > What happens if you detach the parent? I mean, should the trigger
> > removal recurse to children?
>
> It think it should probably exactly undo what CloneRowTriggersToPartition does.
> ..and I guess you're trying to politely say that it didn't. I tried to fix in
> v4 - please check if that's right.
Looks correct to me. Maybe have a test cover that?
> > > It occured to me that we don't currently distinguish between a trigger on a
> > > child table, and a trigger on a parent table which was recursively created on a
> > > child. That makes sense for indexes and constraints, since the objects persist
> > > if the table is detached, so it doesn't matter how it was defined.
> > >
> > > But if we remove trigger during DETACH, then it's *not* the same as a trigger
> > > that was defined on the child, and I suggest that in v13 we should make that
> > > visible.
> >
> > Hmm, interesting point -- whether the trigger is partition or not is
> > important because it affects what happens on detach. I agree that we
> > should make it visible. Is the proposed single bit "PARTITION" good
> > enough, or should we indicate what's the ancestor table that defines the
> > partition?
>
> Yea, it's an obvious thing to do.
This:
+ "false AS tgisinternal"),
+ (pset.sversion >= 13000 ?
+ "pg_partition_root(t.tgrelid) AS parent" :
+ "'' AS parent"),
+ oid);
looks wrong, because the actual partition root may not also be the
trigger parent root, for example:
create table f (a int references p) partition by list (a);
create table f1 partition of f for values in (1) partition by list (a);
create table f11 partition of f for values in (1);
create function trigfunc() returns trigger language plpgsql as $$
begin raise notice '%', new; return new; end; $$;
create trigger trig before insert on f1 for each row execute function
trigfunc();
\d f11
Table "public.f11"
Column | Type | Collation | Nullable | Default
--------+---------+-----------+----------+---------
a | integer | | |
Partition of: f1 FOR VALUES IN (1)
Triggers:
trig BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION
trigfunc(), ON TABLE f
Here, ON TABLE should say "f1".
The following gets the correct parent for me:
- (pset.sversion >= 13000 ?
- "pg_partition_root(t.tgrelid) AS parent" :
- "'' AS parent"),
+ (pset.sversion >= 130000 ?
+ "(SELECT relid"
+ " FROM pg_trigger, pg_partition_ancestors(t.tgrelid)"
+ " WHERE tgname = t.tgname AND tgrelid = relid"
+ " AND tgparentid = 0) AS parent" :
+ " null AS parent"),
The server version number being compared against was missing a zero in
your patch.
Also, how about, for consistency, making the parent table labeling of
the trigger look similar to that for the foreign constraint, so
instead of:
Triggers:
trig BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION
trigfunc(), ON TABLE f1
how about:
Triggers:
TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW
EXECUTE FUNCTION trigfunc()
--
Amit Langote
EnterpriseDB: http://www.enterprisedb.com
On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote: > On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby <pryzby@telsasoft.com> wrote: > > On Sun, Apr 19, 2020 at 03:13:29PM -0400, Alvaro Herrera wrote: > > > What happens if you detach the parent? I mean, should the trigger > > > removal recurse to children? > > > > It think it should probably exactly undo what CloneRowTriggersToPartition does. > > ..and I guess you're trying to politely say that it didn't. I tried to fix in > > v4 - please check if that's right. > > Looks correct to me. Maybe have a test cover that? I included such a test with the v4 patch. > > > > But if we remove trigger during DETACH, then it's *not* the same as a trigger > > > > that was defined on the child, and I suggest that in v13 we should make that > > > > visible. > > > > > > Hmm, interesting point -- whether the trigger is partition or not is > > > important because it affects what happens on detach. I agree that we > > > should make it visible. Is the proposed single bit "PARTITION" good > > > enough, or should we indicate what's the ancestor table that defines the > > > partition? > > > > Yea, it's an obvious thing to do. > > This: > > + "false AS tgisinternal"), > + (pset.sversion >= 13000 ? > + "pg_partition_root(t.tgrelid) AS parent" : > + "'' AS parent"), > + oid); > > > looks wrong, because the actual partition root may not also be the > trigger parent root, for example: Sigh, right. > The following gets the correct parent for me: > > - (pset.sversion >= 13000 ? > - "pg_partition_root(t.tgrelid) AS parent" : > - "'' AS parent"), > + (pset.sversion >= 130000 ? > + "(SELECT relid" > + " FROM pg_trigger, pg_partition_ancestors(t.tgrelid)" > + " WHERE tgname = t.tgname AND tgrelid = relid" > + " AND tgparentid = 0) AS parent" : > + " null AS parent"), I'm happy to see that this doesn't require a recursive cte, at least. I was trying to think how to break it by returning multiple results or results out of order, but I think that can't happen. > Also, how about, for consistency, making the parent table labeling of > the trigger look similar to that for the foreign constraint, so > Triggers: > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION trigfunc() I'll leave that for committer to decide. I split into separate patches since only 0001 should be backpatched (with s/OidIsValid(tgparentid)/isPartitionTrigger/). -- Justin
Attachment
> + deleteDependencyRecordsFor(TriggerRelationId, > + pg_trigger->oid, > + false); > + deleteDependencyRecordsFor(RelationRelationId, > + pg_trigger->oid, > + false); > + > + CommandCounterIncrement(); > + ObjectAddressSet(object, TriggerRelationId, pg_trigger->oid); > + performDeletion(&object, DROP_RESTRICT, PERFORM_DELETION_INTERNAL); > + } > + > + systable_endscan(scan); > + table_close(tgrel, RowExclusiveLock); > + } Two small issues here. First, your second call to deleteDependencyRecordsFor did nothing, because your first call deleted all the dependency records. I changed that to two deleteDependencyRecordsForClass() calls, that actually do what you intended. The other is that instead of deleting each trigger, we can accumulate them to delete with a single performMultipleDeletions call; this also means we get to do CommandCounterIncrement just once. v6 fixes those things and AFAICS is ready to push. I haven't reviewed your 0002 carefully, but (as inventor of the "TABLE t" marker for FK constraints) I agree with Amit that we should imitate that instead of coming up with a new way to show it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On 2020-Apr-20, Alvaro Herrera wrote:
> + while (HeapTupleIsValid(trigtup = systable_getnext(scan)))
> + {
> + Form_pg_trigger pg_trigger = (Form_pg_trigger) GETSTRUCT(trigtup);
> + ObjectAddress trig;
> +
> + /* Ignore triggers that weren't cloned */
> + if (!OidIsValid(pg_trigger->tgparentid) ||
> + !pg_trigger->tgisinternal ||
> + !TRIGGER_FOR_ROW(pg_trigger->tgtype))
> + continue;
Actually, shouldn't we be checking just "!OidIsValid(pg_trigger->tgparentid)"
here? Surely the other two conditions should already not matter either
way if tgparentid is set. I can't see us starting to clone
for-statement triggers, but I'm not sure I trust the internal marking to
remain one way or the other.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
I think I also owe the attached doc updates. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Tue, Apr 21, 2020 at 12:20:38PM -0400, Alvaro Herrera wrote: > diff --git a/doc/src/sgml/ref/alter_table.sgml b/doc/src/sgml/ref/alter_table.sgml > index 7595e609b5..233905552c 100644 > --- a/doc/src/sgml/ref/alter_table.sgml > +++ b/doc/src/sgml/ref/alter_table.sgml > @@ -941,13 +943,14 @@ WITH ( MODULUS <replaceable class="parameter">numeric_literal</replaceable>, REM > <term><literal>DETACH PARTITION</literal> <replaceable class="parameter">partition_name</replaceable></term> > <listitem> > <para> > This form detaches specified partition of the target table. The detached > partition continues to exist as a standalone table, but no longer has any > ties to the table from which it was detached. Any indexes that were > - attached to the target table's indexes are detached. > + attached to the target table's indexes are detached. Any triggers that > + were created to mirror those in the target table are removed. Can you say "cloned" here instead of mirror ? > + attached to the target table's indexes are detached. Any triggers that > + were created as clones of triggers in the target table are removed. Also, I see in the surrounding context a missing word? This form detaches THE specified partition of the target table. -- Justin
On 2020-Apr-20, Justin Pryzby wrote:
> On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote:
> > Also, how about, for consistency, making the parent table labeling of
> > the trigger look similar to that for the foreign constraint, so
> > Triggers:
> > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION trigfunc()
>
> I'll leave that for committer to decide.
Pushed. Many thanks for this!
Changes: I thought that printing the "ON TABLE" bit when it's defined in
the same table is pointless and ugly, so I added a NULLIF to prevent it
in that case (it's not every day that you can put NULLIF to work). I
also changed the empty string to NULL for the case with older servers,
so that it doesn't print a lame "ON TABLE " clause for them. Lastly,
added pg_catalog qualifications everywhere needed.
Contrary to what I had said, I decided to leave the output as submitted;
the constraint lines are not really precedent against it:
55432 13devel 24286=# \d lev3
Partitioned table "public.lev3"
Column │ Type │ Collation │ Nullable │ Default
────────┼─────────┼───────────┼──────────┼─────────
a │ integer │ │ not null │
Partition of: lev2 FOR VALUES IN (3)
Partition key: LIST (a)
Indexes:
"lev3_pkey" PRIMARY KEY, btree (a)
Foreign-key constraints:
TABLE "lev1" CONSTRAINT "lev1_a_fkey" FOREIGN KEY (a) REFERENCES lev1(a)
Referenced by:
TABLE "lev1" CONSTRAINT "lev1_a_fkey" FOREIGN KEY (a) REFERENCES lev1(a)
Triggers:
tt AFTER UPDATE ON lev3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing(), ON TABLE lev2
Number of partitions: 1 (Use \d+ to list them.)
In the "FK constraints" and "referenced by" entries, it looks natural
since the constraint refers to a table. Not so in the trigger case.
--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, Apr 21, 2020 at 07:03:30PM -0400, Alvaro Herrera wrote: > On 2020-Apr-20, Justin Pryzby wrote: > > > On Mon, Apr 20, 2020 at 06:35:44PM +0900, Amit Langote wrote: > > > > Also, how about, for consistency, making the parent table labeling of > > > the trigger look similar to that for the foreign constraint, so > > > Triggers: > > > TABLE "f1" TRIGGER "trig" BEFORE INSERT ON f11 FOR EACH ROW EXECUTE FUNCTION trigfunc() > > > > I'll leave that for committer to decide. > > Pushed. Many thanks for this! Thanks for polishing it. I was just about to convince myself of the merits of doing it Amit's way :) I noticed a few issues: - should put \n's around Amit's subquery to make psql -E look pretty; - maybe should quote the TABLE, like \"%s\" ? #3 is that *if* we did it Amit's way, I *think* maybe we should show the parent's triggerdef, not the childs. It seems strange to me to say "TABLE trigpart .* INSERT ON trigpart3" - TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart3 FOR EACH ROW EXECUTE FUNCTION trigger_nothing() + TABLE "trigpart" TRIGGER trg1 AFTER INSERT ON trigpart FOR EACH ROW EXECUTE FUNCTION trigger_nothing() -- Justin