Re: Avoid orphaned objects dependencies, take 3

Hi,

On Tue, May 26, 2026 at 09:00:11PM +0300, Heikki Linnakangas wrote:
> On 18/05/2026 15:14, Bertrand Drouvot wrote:
> > On Wed, May 13, 2026 at 04:20:21PM -0400, Robert Haas wrote:
> > > On Tue, Apr 28, 2026 at 7:17 AM Bertrand Drouvot
> > > <bertranddrouvot.pg@gmail.com> wrote:
> > > > 0003: Add Assert guard to detect permission check before lock regressions
> > > > 
> > > > Add instrumentation under USE_ASSERT_CHECKING to detect cases where object_aclcheck()
> > > > is called on a referenced object before a lock is held on it, which would widen
> > > > the TOCTOU window between the permission check and the dependency recording.
> > > 
> > > I really like the idea of having some kind of cross-check system that
> > > can detect future (or current) coding mistakes.
> > 
> > Thanks for the feedback! BTW, it detected a new one due to 4793fc41f82, so v21
> > attached does fix it to make the CI green.
> 
> I had a closer look at patch 0001.

Thanks!

> It doesn't fix all the problems, we
> definitely need patch 0002/0003 too, but it's a good step forward and I
> think it makes sense to commit it independently. 

Yeah, this will prevent orphaned objects which is a good step forward.

> So I'm focusing on that
> now.

Thanks! I'll resume working on something similar to 0002 and 0003 once 0001 gets
in.

> I noticed we are already doing essentially the same thing for shared
> dependencies, in shdepLockAndCheckObject(). I see that you even copied the
> function comment from shdepLockAndCheckObject() to LockNotPinnedObject(),
> but I didn't see it being otherwise mentioned in this thread. In any case,
> that's a good argument for doing the same for non-shared dependencies that
> we already do for shared dependencies.

Right.

> > +	if (object->classId == RelationRelationId)
> > +	{
> > +		/* skip shared relations as they are pinned */
> > +		if (IsSharedRelation(object->objectId))
> > +			return;
> > +
> > +		/*
> > +		 * We must 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 of the table...). To avoid any risks, acquire
> > +		 * a lock if there is none. That may add unnecessary lock for 2. but
> > +		 * that's worth it.
> > +		 */
> > +		if (!CheckRelationOidLockedByMe(object->objectId, AccessShareLock, true))
> > +			LockRelationOid(object->objectId, AccessShareLock);
> > +		return;
> > +	}
> 
> Hmm, shouldn't that re-check that the relation exists, after acquiring the
> lock, like the non-relation codepath does? Otherwise it's a little pointless
> to acquire the lock.

Good catch! Your change:

+               if (CheckRelationOidLockedByMe(objectId, AccessShareLock, true))
+                       return;
+               LockRelationOid(objectId, AccessShareLock);
+
+               if (SearchSysCacheExists1(RELOID, ObjectIdGetDatum(objectId)))
+                       return;
+               ereport(ERROR,
+                               (errcode(ERRCODE_UNDEFINED_OBJECT),
+                                errmsg("dependent relation was concurrently dropped")));

Looks good to me.

> I did a bunch of little refactorings and ended up with the attached. Notable
> changes:
> 
> - I renamed and moved LockNotPinnedObject() into
> dependencyLockAndCheckObject(), for consistency with
> shdepLockAndCheckObject().

Makes sense to me.

> - Removed ObjectByIdExist(), inlined it into the caller. The argument to use
> SnapshotSelf or not seemed a bit too special to be generally useful

Yeah, better to have this logic inlined as it will probably not be useful outside
of it.

> - Changed the error message and code to rhyme with the existing "role <OID>
> was concurrently dropped" errors. I didn't add the OID to the error message
> however, because that makes the testing hard.

Agreed that's better.

> - Added a test for a dependency on a role too, to cover the existing
> shdepLockAndCheckObject() function. It's currently disabled because it
> prints the OID, though.

Nit: It also adds:

+# function - role
+permutation "s1_begin" "s1_alter_function_owner" "s2_drop_role" "s1_commit"

That is not disabled and would already pass without the changes added in 0001.

So I wonder if we could just start by adding this new test (and the XXX one) in
a dedicated patch and then add 0001 that would focus on orphaned stuff only.

A few comments:

1/

+#include "catalog/index.h"

That doesn't look needed.

2/

It looks like pgindent "complains" on pg_depend.c for dependencyLockAndCheckObject().

Regards,

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