Re: Avoid orphaned objects dependencies, take 3

On Thu, May 22, 2025 at 8:15 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> That would probably address Robert's concern [1] "acquiring two locks on the same
> object with different lock modes where we should really only acquire one" but
> probably not this one "I think it might result in acquiring the
> locks on those other objects at a subtly wrong time (leading to race
> conditions)".
>
> For the latter I'm not sure how that could be a subtly wrong time or how could
> we determine what a subtly wrong time is (to avoid taking the lock).

Well, I admit I'm not quite sure there is any timing problem here. But
I think it's possible. For example, consider RangeVarGetRelidExtended,
and in particular the callback argument. If we do permissions checking
before locking the relation, the results can change before we actually
lock the relation; if we do it afterward, users can contrive to hold
locks on relations for which they don't have permissions. That rather
ornate callback system allows us to have the best of both worlds. That
system is also concerned with the possibility that we do a name -> OID
translation before holding a lock and by the time we acquire the lock,
concurrent DDL has happened and the answer we got is no longer the
right answer. We've had security holes due to such things.

Now I'm not entirely sure that either of those things are issues here.
My first guess is that name lookup races are not an issue here,
because we're following OID links, but permissions checks seem like
they might be an issue. We might not decide to do something as
elaborate as we did with RangeVarGetRelidExtended and ... maybe that's
OK? But in general I am skeptical of the conclusion that we can just
shove all the locking down into some subordinate layer and nothing
will go wrong, because locking doesn't exist in a vacuum -- it relates
to other things that we also need to do, and whether we do the locking
before or after other steps can affect semantics and even security.

Pushing the locking down into recordDependencyOn amounts to hoping
that we don't need to study each code path in detail and decide on the
exactly right place to acquire the lock. It amounts to hoping that
wherever the recordDependencyOn call is located, it's before things
that we want the locking to happen before and after the things that we
want the locking to happen after. And maybe that's true. Or maybe it's
close enough to true that it's still better than the status quo where
we're not taking locks at all. But on the other hand, since I can't
think of any compelling reason why it HAS to be true, maybe it isn't.

--
Robert Haas
EDB: http://www.enterprisedb.com