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

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