On Thu, Jun 6, 2024 at 1:56 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
> v9 is more invasive (as it changes code in much more places) than v8 but it is
> easier to follow (as it is now clear where the new lock is acquired).
Hmm, this definitely isn't what I had in mind. Possibly that's a sign
that what I had in mind was dumb, but for sure it's not what I
imagined. What I thought you were going to do was add calls like
LockDatabaseObject(NamespaceRelationId, schemaid, 0, AccessShareLock)
in various places, or perhaps LockRelationOid(reloid,
AccessShareLock), or whatever the case may be. Here you've got stuff
like this:
- record_object_address_dependencies(&conobject, addrs_auto,
- DEPENDENCY_AUTO);
+ lock_record_object_address_dependencies(&conobject, addrs_auto,
+ DEPENDENCY_AUTO);
...which to me looks like the locking is still pushed down inside the
dependency code.
And you also have stuff like this:
ObjectAddressSet(referenced, RelationRelationId, childTableId);
+ depLockAndCheckObject(&referenced);
recordDependencyOn(&depender, &referenced, DEPENDENCY_PARTITION_SEC);
But in depLockAndCheckObject you have:
+ if (object->classId == RelationRelationId || object->classId ==
AuthMemRelationId)
+ return;
That doesn't seem right, because then it seems like the call isn't
doing anything, but there isn't really any reason for it to not be
doing anything. If we're dropping a dependency on a table, then it
seems like we need to have a lock on that table. Presumably the reason
why we don't end up with dangling dependencies in such cases now is
because we're careful about doing LockRelation() in the right places,
but we're not similarly careful about other operations e.g.
ConstraintSetParentConstraint is called by DefineIndex which calls
table_open(childRelId, ...) first, but there's no logic in DefineIndex
to lock the constraint.
Thoughts?
--
Robert Haas
EDB: http://www.enterprisedb.com