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(at)telsasoft(dot)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