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
> > > <[email protected]> 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
