On Fri, Jun 14, 2024 at 3:54 AM Bertrand Drouvot <bertranddrouvot...@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 == RelationRelationId || object->classId == 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 == 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. -- Robert Haas EDB: http://www.enterprisedb.com
