Re: Avoid orphaned objects dependencies, take 3

Hi,

On Tue, Jul 02, 2024 at 05:56:23AM +0000, Bertrand Drouvot wrote:
> Hi,
> 
> On Mon, Jul 01, 2024 at 09:39:17AM +0000, Bertrand Drouvot wrote:
> > Hi,
> > 
> > On Wed, Jun 26, 2024 at 10:24:41AM +0000, Bertrand Drouvot wrote:
> > > Hi,
> > > 
> > > On Fri, Jun 21, 2024 at 01:22:43PM +0000, Bertrand Drouvot wrote:
> > > > Another thought for the RelationRelationId class case: we could check if there
> > > > is a lock first and if there is no lock then acquire one. That way that would
> > > > ensure the relation is always locked (so no "risk" anymore), but OTOH it may
> > > > add "unecessary" locking (see 2. mentioned previously).
> > > 
> > > Please find attached v12 implementing this idea for the RelationRelationId class
> > > case. As mentioned, it may add unnecessary locking for 2. but I think that's
> > > worth it to ensure that we are always on the safe side of thing. This idea is
> > > implemented in LockNotPinnedObjectById().
> > 
> > Please find attached v13, mandatory rebase due to 0cecc908e97. In passing, make
> > use of CheckRelationOidLockedByMe() added in 0cecc908e97.
> 
> Please find attached v14, mandatory rebase due to 65b71dec2d5.

In [1] I mentioned that the object locking logic has been put outside of the 
dependency code except for:

recordDependencyOnExpr() 
recordDependencyOnSingleRelExpr()
makeConfigurationDependencies()

Please find attached v15 that also removes the logic outside of the 3 above 
functions. Well, for recordDependencyOnExpr() and recordDependencyOnSingleRelExpr()
that's now done in find_expr_references_walker(): It's somehow still in the
dependency code but at least it is now clear which objects we are locking (and
I'm not sure how we could do better than that for those 2 functions).

There is still one locking call in recordDependencyOnCurrentExtension() but I
think this one is clear enough and does not need to be put outside (for
the same reason mentioned in [1]).

So, to sum up:

A. Locking is now done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
so that it's now always clear what object we want to acquire a lock for. It means
we are not manipulating directly an object address or a list of objects address
as it was the case when the locking was done "directly" within the dependency code.

B. A special case is done for objects that belong to the RelationRelationId class.
For those, we should be in one of the two following cases that would already
prevent the relation to be dropped:

 1. The relation is already locked (could be an existing relation or a relation
 that we are creating).

 2. The relation is protected indirectly (i.e an index protected by a lock on
 its table, a table protected by a lock on a function that depends the table...)

To avoid any risks for the RelationRelationId class case, we acquire a lock if
there is none. That may add unnecessary lock for 2. but that seems worth it. 

[1]: https://www.postgresql.org/message-id/ZnAVEBhlGvpDDVOD%40ip-10-97-1-34.eu-west-3.compute.internal

Regards,

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