Re: Avoid orphaned objects dependencies, take 3

Hi,

On Tue, May 20, 2025 at 02:12:41PM -0700, Jeff Davis wrote:
> On Mon, 2025-05-19 at 14:07 -0400, Robert Haas wrote:
> > I agree with that, but I think that it may also be error-prone to
> > assume that it's OK to acquire heavyweight locks on other catalog
> > objects at any place in the code where we record a dependency. I will
> > not be surprised at all if that turns out to have some negative
> > consequences. For example, I think it might result in acquiring the
> > locks on those other objects at a subtly wrong time (leading to race
> > conditions) or acquiring two locks on the same object with different
> > lock modes where we should really only acquire one. I'm all in favor
> > of solving this problem using heavyweight locks, but I think that
> > implicitly acquiring them as a side effect of recording dependencies
> > feels too surprising.
>
> I see what you mean now, in the sense that other code that calls
> LockDatabaseObject (and other variants of LockAcquire) are mostly
> higher-level operations like AlterPublication(), and not side-effects
> of something else.
>
> But relation_open() is sort of an exception. There are lots of places
> where that takes a lock because we happen to want something out of the
> relcache, like generate_partition_qual() taking a lock on the parent or
> CheckAttributeType() taking a lock on the typrelid. You could say those
> are fairly obvious, but that's because we already know, and we could
> make it more widely known that recording a dependency takes a lock.
>
> One compromise might be to have recordDependencyOn() take a LOCKMODE
> parameter, which would both inform the caller that a lock will be
> taken, and allow the caller to do it their own way and specify NoLock
> if necessary. That still results in a huge diff, but the end result
> would not be any more complex than the current code.

Thanks for sharing your thoughts! I had in mind to "just" check if there
is an existing lock (and if so, skip acquiring a new one) but your proposal
sounds better. Indeed it would make the locking behavior explicit and also
be flexible (allowing the callers to choose the LOCKMODE).

I'll prepare a new version implementing your proposal.

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com