Feedback-ID: ic6394509:Fastmail
MIME-Version: 1.0
Date: Wed, 09 Oct 2024 11:14:35 +0200
From: "Joel Jacobson" <joel@compiler.org>
To: "Amul Sul" <sulamul@gmail.com>,
 pgsql-hackers <pgsql-hackers@postgresql.org>
Message-Id: <239bcb22-3e2b-4d94-be36-a76f554b94df@app.fastmail.com>
In-Reply-To: 
 <CAAJ_b962c5AcYW9KUt_R_ER5qs3fUGbe4az-SP-vuwPS-w-AGA@mail.gmail.com>
References: 
 <CAAJ_b962c5AcYW9KUt_R_ER5qs3fUGbe4az-SP-vuwPS-w-AGA@mail.gmail.com>
Subject: Re: NOT ENFORCED constraint feature
Content-Type: text/plain
Content-Transfer-Encoding: 7bit
Archived-At: <https://www.postgresql.org/message-id/239bcb22-3e2b-4d94-be36-a76f554b94df%40app.fastmail.com>
Precedence: bulk

On Tue, Oct 8, 2024, at 11:06, Amul Sul wrote:
> The attached patch proposes adding the ability to define CHECK and
> FOREIGN KEY constraints as NOT ENFORCED.

Thanks for working on this!

> Adding NOT ENFORCED to CHECK constraints is simple, see 0001 patch,

I've looked at the 0001 patch and think it looks simple and straight forward.

> but implementing it for FOREIGN KEY constraints requires more code due
> to triggers, see 0002 - 0005 patches.

I can't say that much yet about the code changes in 0002 - 0005 yet,
but I've tested the patches and successfully experimented with the feature.

Also think the documentation is good and sound. Only found a minor typo:
-    Adding a enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>
+    Adding an enforced <literal>CHECK</literal> or <literal>NOT NULL</literal>

> There are various approaches for
> implementing NOT ENFORCED foreign keys, what I thought of:
>
> 1. When defining a NOT ENFORCED foreign key, skip the creation of
> triggers used for referential integrity check, while defining an
> ENFORCED foreign key, remain the same as the current behaviour. If an
> existing foreign key is changed to NOT ENFORCED, the triggers are
> dropped, and when switching it back to ENFORCED, the triggers are
> recreated.
>
> 2. Another approach could be to create the NOT ENFORCED constraint
> with the triggers as usual, but disable those triggers by updating the
> pg_trigger catalog so that they are never executed for the check. And
> enable them when the constraint is changed back to ENFORCED.
>
> 3. Similarly, a final approach would involve updating the logic where
> trigger execution is decided and skipping the execution if the
> constraint is not enforced, rather than modifying the pg_trigger
> catalog.
>
> In the attached patch, the first approach has been implemented. This
> requires more code changes but prevents unused triggers from being
> left in the database and avoids the need for changes all over the
> place to skip trigger execution, which could be missed in future code
> additions.

I also like the first approach, since I think it's nice the pg_trigger
entires are inserted / deleted upon enforced / not enforced.

> The ALTER CONSTRAINT operation in the patch added code to handle
> dropping and recreating triggers. An alternative approach would be to
> simplify the process by dropping and recreating the FK constraint,
> which would automatically handle skipping or creating triggers for NOT
> ENFORCED or ENFORCED FK constraints. However, I wasn't sure if this
> was the right approach, as I couldn't find any existing ALTER
> operations that follow this pattern.

I think the current approach of dropping and recreating the triggers is best,
since if we would instead be dropping and recreating the FK constraint,
that would cause problems if some other future SQL feature would need to
introduce dependencies on the FK constraints via pg_depend.

Best regards,

Joel