public inbox for [email protected]
help / color / mirror / Atom feedFrom: Bertrand Drouvot <[email protected]>
To: Heikki Linnakangas <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Jeff Davis <[email protected]>
Cc: Roman Eskin <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: [email protected]
Cc: Tom Lane <[email protected]>
Subject: Re: Avoid orphaned objects dependencies, take 3
Date: Tue, 26 May 2026 20:18:50 +0000
Message-ID: <ahYAKgbx05IQdjwR@bdtpg> (raw)
In-Reply-To: <[email protected]>
References: <CA+TgmobxpkwwjfM4EkZxyKpJ3S0CqQPXPkqQn4YmCHBeMDypdQ@mail.gmail.com>
<aD2u/GR/[email protected]>
<CA+TgmoZh8yXoQ2AF-VFSTswhin0o+BZ78AaOsWZskCW1GHBd6g@mail.gmail.com>
<[email protected]>
<[email protected]>
<ad/awXLnMuc6Rf5M@bdtpg>
<afCXPVNuvpf5ECGZ@bdtpg>
<CA+TgmobtzkG9HzGtDXbgb3KVE46qHuSvGcuKHgAGQ1aejCoLLw@mail.gmail.com>
<agsCqlLTytZCudMv@bdtpg>
<[email protected]>
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
view thread (50+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: Avoid orphaned objects dependencies, take 3
In-Reply-To: <ahYAKgbx05IQdjwR@bdtpg>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox