Re: Add more sanity checks around callers of changeDependencyFor()

On Thu, Jun 29, 2023 at 10:06:35AM +0300, Heikki Linnakangas wrote:
> The error messages like "failed to change schema dependency for extension"
> don't conform to the usual error message style. "could not change schema
> dependency for extension" would be more conformant. I see that you
> copy-pasted that from existing messages, and we have a bunch of other
> "failed to" messages in the repository too, so I'm OK with leaving it as it
> is for now. Or maybe change the wording of all the changeDependencyFor()
> callers now, and consider all the other "failed to" messages separately
> later.

I'm OK to change the messages for all changeDependencyFor() now that
these are being touched.  I am counting 7 of them.

> If changeDependencyFor() returns >= 2, the message is a bit misleading.
> That's what the existing callers did too, so maybe that's fine.
> 
> I can hit the above error with the attached test case. That seems wrong,
> although I don't know if it means that the check is wrong or it exposed a
> long-standing bug.

Coming back to this one, I think that my check and you have found an
old bug in AlterExtensionNamespace() where the sequence of objects
manipulated breaks the namespace OIDs used to change the normal
dependency of the extension when calling changeDependencyFor().  The
check I have added looks actually correct to me because there should
be always have one 'n' pg_depend entry to change between the extension
and its schema, and we should always change it.

A little bit of debugging is showing me that at the stage of "ALTER
EXTENSION test_ext_req_schema1 SET SCHEMA test_func_dep3;", oldNspOid
is set to the OID of test_func_dep2, and nspOid is the OID of
test_func_dep3.  So the new OID is correct, but the old one points to
the schema test_func_dep2 used by the function because it is the first
object it has been picked up while scanning pg_depend, and not the
schema test_func_dep1 used by the extension.  This causes the command
to fail to update the schema dependency between the schema and the
extension.

The origin of the confusing comes to the handling of oldNspOid, in my
opinion.  I don't quite see why it is necessary to save the old OID of
the namespace from the object scanned while we know the previous
schema used by the extension thanks to its pg_extension entry.

Also, note that there is a check in AlterExtensionNamespace() to
prevent the command from happening if an object is not in the same
schema as the extension, but it fails to trigger here.  I have written
a couple of extra queries to show the difference.

Please find attached a patch to fix this issue with ALTER EXTENSION
.. SET SCHEMA, and the rest.  The patch does everything discussed, but
it had better be split into two patches for different branches.  Here
are my thoughts:
- Fix and backpatch the ALTER EXTENSION business, *without* the new
sanity check for changeDependencyFor() in AlterExtensionNamespace(),
with its regression test.
- Add all the sanity checks and reword the error messages related to
changeDependencyFor() only on HEAD.

Thoughts?
--
Michael