Re: Avoid orphaned objects dependencies, take 3

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(dot)pg(at)gmail(dot)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