Date: Wed, 22 May 2024 10:21:34 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Robert Haas <robertmhaas@gmail.com>
Cc: Michael Paquier <michael@paquier.xyz>,
	Alexander Lakhin <exclusion@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Avoid orphaned objects dependencies, take 3
Message-ID: <Zk3HLvMhft2taFIz@ip-10-97-1-34.eu-west-3.compute.internal>
References: <62310067-6dc4-97ab-6724-7d3602b5056d@gmail.com>
 <ZinjZmaMEEkOSfZ0@ip-10-97-1-34.eu-west-3.compute.internal>
 <403075e1-4d60-09e2-a06a-749cd9d88f66@gmail.com>
 <ZioEJafHC95wjfMk@ip-10-97-1-34.eu-west-3.compute.internal>
 <d815344e-a6a2-617a-d35f-012e5e89f29f@gmail.com>
 <Zjy/o3t/26exwj5Z@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZkQMYVxbOxFD0519@paquier.xyz>
 <ZkRy73Q2NDBT0Avq@ip-10-97-1-34.eu-west-3.compute.internal>
 <ZkSO5qlRNnFimJev@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmoYQh9y3yWm9keq5EzjLBw3y_kcQJ5-rUJtCcJHs6YNicg@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CA+TgmoYQh9y3yWm9keq5EzjLBw3y_kcQJ5-rUJtCcJHs6YNicg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/Zk3HLvMhft2taFIz%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Tue, May 21, 2024 at 08:53:06AM -0400, Robert Haas wrote:
> On Wed, May 15, 2024 at 6:31 AM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> > Please find attached v6 (only diff with v5 is moving the tests as suggested
> > above).
> 
> I don't immediately know what to think about this patch.

Thanks for looking at it!

> I've known about this issue for a long time, but I didn't think this was how we
> would fix it.

I started initially with [1] but it was focusing on function-schema only.

Then I proposed [2] making use of a dirty snapshot when recording the dependency.
But this approach appeared to be "scary" and it was still failing to close
some race conditions.

Then, Tom proposed another approach in [2] which is that "creation DDL will have
to take a lock on each referenced object that'd conflict with a lock taken by DROP".
This is the one the current patch is trying to implement.

> What you've done here instead is add locking at a much
> lower level - whenever we are adding a dependency on an object, we
> lock the object. The advantage of that approach is that we definitely
> won't miss anything.

Right, as there is much more than the ones related to schemas, for example:

- function and arg type
- function and return type
- function and function
- domain and domain
- table and type
- server and foreign data wrapper

to name a few.

> The disadvantage of that approach is that it
> means we have some very low-level code taking locks, which means it's
> not obvious which operations are taking what locks.

Right, but the new operations are "only" the ones leading to recording or altering
a dependency.

> Maybe it could
> even result in some redundancy, like the higher-level code taking a
> lock also (possibly in a different mode) and then this code taking
> another one.

The one that is added here is in AccessShareLock mode. It could conflict with
the ones in AccessExclusiveLock means (If I'm not missing any):

- AcquireDeletionLock(): which is exactly what we want
- get_object_address()
    - get_object_address_rv()
        - ExecAlterObjectDependsStmt()
    - ExecRenameStmt()
    - ExecAlterObjectDependsStmt()
    - ExecAlterOwnerStmt()
    - RemoveObjects()
- AlterPublication()

I think there is 2 cases here:

First case: the "conflicting" lock mode is for one of our own lock then LockCheckConflicts()
would report this as a NON conflict.

Second case: the "conflicting" lock mode is NOT for one of our own lock then LockCheckConflicts()
would report a conflict. But I've the feeling that the existing code would
already lock those sessions.

One example where it would be the case:

Session 1: doing "BEGIN; ALTER FUNCTION noschemas.foo2() SET SCHEMA alterschema" would
acquire the lock in AccessExclusiveLock during ExecAlterObjectSchemaStmt()->get_object_address()->LockDatabaseObject()
(in the existing code and before the new call that would occur through changeDependencyFor()->depLockAndCheckObject()
with the patch in place).

Then, session 2: doing "alter function noschemas.foo2() owner to newrole;"
would be locked in the existing code while doing ExecAlterOwnerStmt()->get_object_address()->LockDatabaseObject()).

Means that in this example, the new lock this patch is introducing would not be
responsible of session 2 beging locked.

Was your concern about "First case" or "Second case" or both?

[1]: https://www.postgresql.org/message-id/flat/5a9daaae-5538-b209-6279-e903c3ea2157%40amazon.com
[2]: https://www.postgresql.org/message-id/flat/8369ff70-0e31-f194-2954-787f4d9e21dd%40amazon.com

Regards,

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