On Thu, May 22, 2025 at 8:15 AM Bertrand Drouvot <bertranddrouvot...@gmail.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
