Alvaro Herrera <alvhe...@2ndquadrant.com> writes:
> On 2020-Jan-14, Alvaro Herrera wrote:
>> Hmm, it seems to be doing it differently. Maybe it should be acquiring
>> locks on all objects in that nested loop and verified them for
>> existence, so that when it calls performMultipleDeletions the objects
>> are already locked, as you say.
> Yeah, this solves the reported bug.
I looked this over and think it should be fine. There will be cases
where we get a deadlock error, but such risks existed anyway, since
we'd have acquired all the same locks later in the process.
> This is not a 100% solution: there's the cases when the user is removed
> from an ACL and from a policy, and those deletions are done directly
> instead of accumulating to the end for a mass deletion.
Let's worry about that when and if we get field complaints.
> I had to export AcquireDeletionLock (previously a static in
> dependency.c). I wonder if I should export ReleaseDeletionLock too, for
> symmetry.
Hmmm ... there is an argument for doing ReleaseDeletionLock in the code
paths where you discover that the object's been deleted. Holding a lock
on a no-longer-existent object OID should be harmless from a deadlock
standpoint; but it does represent useless consumption of a shared lock
table entry, and that's a resource this operation could already burn
with abandon.
Also, if we're exporting these, it's worth expending a bit more
effort on their header comments. In particular AcquireDeletionLock
should describe its flags argument; perhaps along the lines of
"Accepts the same flags as performDeletion (though currently only
PERFORM_DELETION_CONCURRENTLY does anything)".
regards, tom lane
