Date: Fri, 7 Jun 2024 08:41:36 +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, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Avoid orphaned objects dependencies, take 3
Message-ID: <ZmLHwE4NSlzzxXDT@ip-10-97-1-34.eu-west-3.compute.internal>
References: <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>
 <Zk3HLvMhft2taFIz@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmoYAv09Y9Y8-yOMzTCijzZeVqFrLBx6Y4+VPh0CMg476Uw@mail.gmail.com>
 <Zk7D2M2pGk1JDkS8@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmobnNpBhV13F+P6LKP+bLE3aBjzgeuf4yySnkHKceA475A@mail.gmail.com>
 <ZmFPfmSms4IyyO8T@ip-10-97-1-34.eu-west-3.compute.internal>
 <CA+TgmoaCJr2U1TfchSFn2jNEbvLsFYmh__A1mHhRv565MMezyg@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+TgmoaCJr2U1TfchSFn2jNEbvLsFYmh__A1mHhRv565MMezyg@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZmLHwE4NSlzzxXDT%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Thu, Jun 06, 2024 at 04:00:23PM -0400, Robert Haas wrote:
> 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.

I see what you’re saying, doing things like:

LockDatabaseObject(TypeRelationId, returnType, 0, AccessShareLock);
in ProcedureCreate() for example.

> 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.

Yes but it’s now located in places where, I think, it’s easier to understand
what’s going on (as compare to v8), except maybe for:

recordDependencyOnExpr()
makeOperatorDependencies()
GenerateTypeDependencies()
makeParserDependencies()
makeDictionaryDependencies()
makeTSTemplateDependencies()
makeConfigurationDependencies()

but probably for:

heap_create_with_catalog()
StorePartitionKey()
index_create()
AggregateCreate()
CastCreate()
CreateConstraintEntry()
ProcedureCreate()
RangeCreate()
InsertExtensionTuple()
CreateTransform()
CreateProceduralLanguage()

The reasons I keep it linked to the dependency code are:

- To ensure we don’t miss anything (well, with the new Assert in place that’s
probably a tangential argument)

- It’s not only about locking the object: it’s also about 1) verifying the object
is pinned, 2) checking it still exists and 3) provide a description in the error
message if we can (in case the object does not exist anymore). Relying on an
already build object (in the dependency code) avoid to 1) define the object(s)
one more time or 2) create new functions that would do the same as isObjectPinned()
and getObjectDescription() with a different set of arguments.

That may sounds like weak arguments but it has been my reasoning.

Do you still find the code hard to maintain with v9?

> 
> 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,

Yeah, that's what I think: we're already careful when we deal with relations.

> 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.

table_open(childRelId, ...) would lock any "ALTER TABLE <childRelId> DROP CONSTRAINT"
already. Not sure I understand your concern here.

Regards,

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