Re: Avoid orphaned objects dependencies, take 3 - Mailing list pgsql-hackers

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. 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. So I'm focusing on that 
now.

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.

> +    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.

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().

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

- 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. It'd be nice 
to have a general masking mechanism for OIDs and XIDs in error messages 
in tests, but now is not the time for that.

- 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.


- Heikki