Re: Avoid orphaned objects dependencies, take 3

On Thu, May 09, 2024 at 12:20:51PM +0000, Bertrand Drouvot wrote:
> Oh I see, your test updates an existing dependency. v4 took care about brand new
> dependencies creation (recordMultipleDependencies()) but forgot to take care
> about changing an existing dependency (which is done in another code path:
> changeDependencyFor()).
>
> Please find attached v5 that adds:
>
> - a call to the new depLockAndCheckObject() function in changeDependencyFor().
> - a test when altering an existing dependency.
>
> With v5 applied, I don't see the issue anymore.

+ if (object_description)
+ ereport(ERROR, errmsg("%s does not exist", object_description));
+ else
+ ereport(ERROR, errmsg("a dependent object does not ex

This generates an internal error code. Is that intended?

--- /dev/null
+++ b/src/test/modules/test_dependencies_locks/specs/test_dependencies_locks.spec

This introduces a module with only one single spec. I could get
behind an extra module if splitting the tests into more specs makes
sense or if there is a restriction on installcheck. However, for
one spec file filed with a bunch of objects, and note that I'm OK to
let this single spec grow more for this range of tests, it seems to me
that this would be better under src/test/isolation/.

+ if (use_dirty_snapshot)
+ {
+ InitDirtySnapshot(DirtySnapshot);
+ snapshot = &DirtySnapshot;
+ }
+ else
+ snapshot = NULL;

I'm wondering if Robert has a comment about that. It looks backwards
in a world where we try to encourage MVCC snapshots for catalog
scans (aka 568d4138c646), especially for the part related to
dependency.c and ObjectAddresses.
--
Michael