Re: DETACH PARTITION and FOR EACH ROW triggers on partitioned tables

On Mon, Apr 20, 2020 at 5:49 AM Justin Pryzby <pryzby(at)telsasoft(dot)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