MIME-Version: 1.0
References: <Zk3HLvMhft2taFIz@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmoYAv09Y9Y8-yOMzTCijzZeVqFrLBx6Y4+VPh0CMg476Uw@mail.gmail.com>
 <Zk7D2M2pGk1JDkS8@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmobnNpBhV13F+P6LKP+bLE3aBjzgeuf4yySnkHKceA475A@mail.gmail.com>
 <ZmFPfmSms4IyyO8T@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmoaCJr2U1TfchSFn2jNEbvLsFYmh__A1mHhRv565MMezyg@mail.gmail.com>
 <ZmLHwE4NSlzzxXDT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+Tgmob_6FpJGkBRMOg+VrJ=eMHEyAC8-7HJvjMDShQd5Tt3Fw@mail.gmail.com>
 <ZmsjuTbjoUmuS7PT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+Tgmobeqs5tE8h-_PF_Orp67kNJdRJ9b4pqDYQr1WjLT24ZEg@mail.gmail.com> <Zmv3TPfJAyQXhIdu@ip-10-97-1-34.eu-west-3.compute.internal>
In-Reply-To: <Zmv3TPfJAyQXhIdu@ip-10-97-1-34.eu-west-3.compute.internal>
From: Robert Haas <robertmhaas@gmail.com>
Date: Mon, 17 Jun 2024 12:24:46 -0400
Message-ID: <CA+TgmoaQ+Gd8rrun71Jr1GReyZz3rchyqpiD0jijXbLGJOWgLg@mail.gmail.com>
Subject: Re: Avoid orphaned objects dependencies, take 3
To: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>, Alexander Lakhin <exclusion@gmail.com>, 
	pgsql-hackers@lists.postgresql.org, Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BTgmoaQ%2BGd8rrun71Jr1GReyZz3rchyqpiD0jijXbLGJOWgLg%40mail.gmail.com>
Precedence: bulk

On Fri, Jun 14, 2024 at 3:54=E2=80=AFAM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> > Ah, right. So, I was assuming that, with either this version of your
> > patch or the earlier version, we'd end up locking the constraint
> > itself. Was I wrong about that?
>
> The child contraint itself is not locked when going through
> ConstraintSetParentConstraint().
>
> While at it, let's look at a full example and focus on your concern.

I'm not at the point of having a concern yet, honestly. I'm trying to
understand the design ideas. The commit message just says that we take
a conflicting lock, but it doesn't mention which object types that
principle does or doesn't apply to. I think the idea of skipping it
for cases where it's redundant with the relation lock could be the
right idea, but if that's what we're doing, don't we need to explain
the principle somewhere? And shouldn't we also apply it across all
object types that have the same property?

Along the same lines:

+ /*
+ * Those don't rely on LockDatabaseObject() when being dropped (see
+ * AcquireDeletionLock()). Also it looks like they can not produce
+ * orphaned dependent objects when being dropped.
+ */
+ if (object->classId =3D=3D RelationRelationId || object->classId =3D=3D
AuthMemRelationId)
+ return;

"It looks like X cannot happen" is not confidence-inspiring. At the
very least, a better comment is needed here. But also, that relation
has no exception for AuthMemRelationId, only for RelationRelationId.
And also, the exception for RelationRelationId doesn't imply that we
don't need a conflicting lock here: the special case for
RelationRelationId in AcquireDeletionLock() is necessary because the
lock tag format is different for relations than for other database
objects, not because we don't need a lock at all. If the handling here
were really symmetric with what AcquireDeletionLock(), the coding
would be to call either LockRelationOid() or LockDatabaseObject()
depending on whether classid =3D=3D RelationRelationId. Now, that isn't
actually necessary, because we already have relation-locking calls
elsewhere, but my point is that the rationale this commit gives for
WHY it isn't necessary seems to me to be wrong in multiple ways.

So to try to sum up here: I'm not sure I agree with this design. But I
also feel like the design is not as clear and consistently implemented
as it could be. So even if I just ignored the question of whether it's
the right design, it feels like we're a ways from having something
potentially committable here, because of issues like the ones I
mentioned in the last paragraph.

--=20
Robert Haas
EDB: http://www.enterprisedb.com