Date: Thu, 22 Jul 2021 13:41:33 -0400
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Arne Roland <A.Roland@index.de>
Cc: vignesh C <vignesh21@gmail.com>, Zhihong Yu <zyu@yugabyte.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Rename of triggers for partitioned tables
Message-ID: <202107221741.6gb46lytggyj@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <29dca60bc79e4c2ab26fa328a94b1289@index.de>
Archived-At: 
 <https://www.postgresql.org/message-id/202107221741.6gb46lytggyj%40alvherre.pgsql>
Precedence: bulk

On 2021-Jul-22, Arne Roland wrote:

> I just noticed that apparently the
> ALTER TABLE [ IF EXISTS ] [ ONLY ] name [ ENABLE | DISABLE ] TRIGGER ...
> syntax albeit looking totally different and it already recurses, it
> has precisely the same issue with pg_dump. In that case the ONLY
> syntax isn't optional and the 2. from your above post does inevitably
> apply.
> 
> Since it is sort of the same problem, I think it might be worthwhile
> to address it as well within this patch. Adding two to four ereports
> doesn't sound like scope creeping to me, even though it touches
> completely different code. I'll look into that as well.

Oh!  For some reason I failed to notice that you were talking about
ENABLE / DISABLE, not RENAME anymore.  It turns out that I recently
spent some time in this area; see commits below (these are from branch
REL_11_STABLE).  Are you talking about something that need to be handled
*beyond* these fixes?

commit ccfc3cbb341abf515d930097c4851d9e2152504f
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Fri Jul 16 17:29:22 2021 -0400
CommitDate: Fri Jul 16 17:29:22 2021 -0400

    Fix pg_dump for disabled triggers on partitioned tables
    
    pg_dump failed to preserve the 'enabled' flag (which can be not only
    disabled, but also REPLICA or ALWAYS) for partitions which had it
    changed from their respective parents.  Attempt to handle that by
    including a definition for such triggers in the dump, but replace the
    standard CREATE TRIGGER line with an ALTER TRIGGER line.
    
    Backpatch to 11, where these triggers can exist.  In branches 11 and 12,
    pick up a few test lines from commit b9b408c48724 to verify that
    pg_upgrade is okay with these arrangements.
    
    Co-authored-by: Justin Pryzby <pryzby@telsasoft.com>
    Co-authored-by: Álvaro Herrera <alvherre@alvh.no-ip.org>
    Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com

commit fed35bd4a65032f714af6bc88c102d0e90422dee
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Fri Jul 16 13:01:43 2021 -0400
CommitDate: Fri Jul 16 13:01:43 2021 -0400

    Preserve firing-on state when cloning row triggers to partitions
    
    When triggers are cloned from partitioned tables to their partitions,
    the 'tgenabled' flag (origin/replica/always/disable) was not propagated.
    Make it so that the flag on the trigger on partition is initially set to
    the same value as on the partitioned table.
    
    Add a test case to verify the behavior.
    
    Backpatch to 11, where this appeared in commit 86f575948c77.
    
    Author: Álvaro Herrera <alvherre@alvh.no-ip.org>
    Reported-by: Justin Pryzby <pryzby@telsasoft.com>
    Discussion: https://postgr.es/m/20200930223450.GA14848@telsasoft.com

commit bbb927b4db9b3b449ccd0f76c1296de382a2f0c1
Author:     Alvaro Herrera <alvherre@alvh.no-ip.org> [Álvaro Herrera <alvherre@alvh.no-ip.org>]
AuthorDate: Tue Oct 20 19:22:09 2020 -0300
CommitDate: Tue Oct 20 19:22:09 2020 -0300

    Fix ALTER TABLE .. ENABLE/DISABLE TRIGGER recursion
    
    More precisely, correctly handle the ONLY flag indicating not to
    recurse.  This was implemented in 86f575948c77 by recursing in
    trigger.c, but that's the wrong place; use ATSimpleRecursion instead,
    which behaves properly.  However, because legacy inheritance has never
    recursed in that situation, make sure to do that only for new-style
    partitioning.
    
    I noticed this problem while testing a fix for another bug in the
    vicinity.
    
    This has been wrong all along, so backpatch to 11.
    
    Discussion: https://postgr.es/m/20201016235925.GA29829@alvherre.pgsql

-- 
Álvaro Herrera              Valdivia, Chile  —  https://www.EnterpriseDB.com/