Re: Avoid orphaned objects dependencies, take 3

Hi,

On Thu, Jan 02, 2025 at 08:15:13AM +0000, Bertrand Drouvot wrote:
> rebased (v18 attached).

Thanks to all of you that have discussed this patch during the developer meeting
at FOSDEM PGDay last week [1]. I'm attaching a new version to address Álvaro's
concern about calling getObjectDescription() in the new LockNotPinnedObjectById()
function. This call was being used to provide a "meaningful" error message but
we agreed to provide the object OID instead (as anyway the object is dropped).

Note that the OIDs are reported as "errdetail" to ensure the isolation tests
added in this patch remain stable (output does not depend of the actual OIDs
values).

A quick sum up about this patch:

A. Locking is done exclusively with LockNotPinnedObject(Oid classid, Oid objid)
so that it's now always clear what object we want to acquire a lock for. It means
that we are not manipulating directly an object address or a list of objects
address as it was the case when the locking was done "directly" within the
dependency code.

B. A special case is done for objects that belong to the RelationRelationId class.
For those, we should be in one of the two following cases that would already
prevent the relation to be dropped:

B1. The relation is already locked (could be an existing relation or a relation
that we are creating).

B2. 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 the table...)

To avoid any risks for the RelationRelationId class case, we acquire a lock if
there is none. That may add unnecessary lock for B2. but that seems worth it.

That's a lot of mechanical changes so that's easy to miss one or to do it
wrong but:

1. I did my best to avoid that
2. assertions are added in recordMultipleDependencies() to "ensure" the object
is locked
3. even if one case is missing (that is not catched by the assertions because
the dependency is not covered in the tests, not sure that exists though), then it
just means that we could be in the current state (orphaned dependency), not worst
than that

During the meeting a question has been raised regarding the number of locks
increase. This has already been discussed in [2] and I think that the outcome
is that the default max_locks_per_transaction value (64) is probably still
enough in real life (and even if it is not then it can be increased to satisfy
the requirements).

[1]: https://2025.fosdempgday.org/devmeeting
[2]: https://www.postgresql.org/message-id/CA%2BTgmoaFPUubBBk52Qp2wkoL7JX7OjhewiK%2B7LSot7%3DrecbzzQ%40mail.gmail.com

Regards,

--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com