Shayon Mukherjee <shayonj@gmail.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
