Re: [PATCH] Proposal: Allow reads to proceed during FK/trigger drops by reducing relation-level lock from AccessExclusive to ShareRowExclusive

Thank you so much for the review and feedback Tom

On Wed, Oct 8, 2025 at 5:39 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> Shayon Mukherjee <shayonj(at)gmail(dot)com> writes:
> > Following up on the previous thread - I took a stab at trying to see
> what a
> > full patch for the proposal to reduce lock levels during FK/trigger drops
> > would look like, and this is what I ended up with.
>
> I don't think this is safe, at least not for FK removal. There's a
> comment in AlterTableGetLockLevel explaining why:
>
> /*
> * Removing constraints can affect SELECTs that have been
> * optimized assuming the constraint holds true. See also
> * CloneFkReferenced.
> */
>

I tried to map my mental model and walk down the code paths, I knew I was
missing something :^). Thank you for pointing this out.

>
> Adding a foreign key can (and I think does) take a lesser lock,
> because the additional constraint won't invalidate any proofs the
> optimizer may have made beforehand. But dropping one seems
> problematic.
>

You're 100% correct that the table owning the constraint (fktable) needs
AccessExclusive because of the requirement that dropping the FK can
invalidate plans that used it for proofs. That said, I wonder if we can
reliably do something where only the referenced table’s RI action triggers
are weakened to ShareRowExclusive in trigger deletion, which would help us
achieve the criteria that the weaker locks on the referenced table (not the
table being dropped), can accept SELECTs/reads?

> Another issue is that the proposed patch looks like it reduces
> the locking level for more types of constraints than just FKs.
> That would require further analysis, but I believe that (for
> example) dropping a unique constraint likewise risks breaking
> existing query plans, even when they aren't directly using that
> index.
>

That makes sense, yes. I attempted at an updated patch with reduced locks
such that it's only for FK RI action triggers on the referenced table with
no lock reduction applied to referenced tables for other constraint types.
That is, if my understanding and mental model is right here.

I attached an updated patch with the new discoveries and the changes made
are:

- RemoveConstraintById(): Reverted to AccessExclusiveLock
- RemoveTriggerById(): Now only uses ShareRowExclusiveLock for internal RI
action triggers on the referenced table (confrelid). All other triggers
(user triggers, RI check triggers on the FK table) still use
AccessExclusiveLock.
- dropconstraint_internal(): ShareRowExclusiveLock only for FK drops on the
referenced table (already inside `if CONSTRAINT_FOREIGN` block)
- ATPostAlterTypeCleanup(): Added constraint type check; only FK rebuilds
use ShareRowExclusive

I feel like I could still be missing something, so I really appreciate any
feedback. Definitely not trying to push hard on this and more so just using
this as a learning opportunity as well.

Thank you
Shayon