ALTER EXTENSION SET SCHEMA versus dependent types
I happened to notice that the comment for AlterObjectNamespace_oid
claims that
* ... it doesn't have to deal with certain special cases
* such as not wanting to process array types --- those should never
* be direct members of an extension anyway.
This struck me as probably broken in the wake of e5bc9454e
(Explicitly list dependent types as extension members in pg_depend),
and sure enough a moment's worth of testing showed it is:
regression=# create schema s1;
CREATE SCHEMA
regression=# create extension cube with schema s1;
CREATE EXTENSION
regression=# create schema s2;
CREATE SCHEMA
regression=# alter extension cube set schema s2;
ERROR: cannot alter array type s1.cube[]
HINT: You can alter type s1.cube, which will alter the array type as well.
So we need to do something about that; and the fact that this escaped
testing shows that our coverage for ALTER EXTENSION SET SCHEMA is
pretty lame.
The attached patch fixes up the code and adds a new test to
the test_extensions module. The fix basically is to skip the
pg_depend entries for dependent types, assuming that they'll
get dealt with when we process their parent objects.
regards, tom lane
Attachments:
v1-fix-set-schema-for-dependent-types.patchtext/x-diff; charset=us-ascii; name=v1-fix-set-schema-for-dependent-types.patchDownload+133-27
On Wed, May 08, 2024 at 05:52:31PM -0400, Tom Lane wrote:
The attached patch fixes up the code and adds a new test to
the test_extensions module. The fix basically is to skip the
pg_depend entries for dependent types, assuming that they'll
get dealt with when we process their parent objects.
Looks reasonable to me. The added test coverage seems particularly
valuable. If I really wanted to nitpick, I might complain about the three
consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
makes lines like
+ AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true,
+ objsMoved);difficult to interpret. But that's not necessarily the fault of this patch
and probably needn't block it.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
Looks reasonable to me. The added test coverage seems particularly
valuable. If I really wanted to nitpick, I might complain about the three
consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
makes lines like
+ AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true, + objsMoved);
difficult to interpret. But that's not necessarily the fault of this patch
and probably needn't block it.
I considered merging ignoreDependent and errorOnTableType into a
single 3-valued enum, but didn't think it was worth the trouble
given the very small number of callers; also it wasn't quite clear
how to map that to AlterTypeNamespace_oid's API. Perhaps a little
more thought is appropriate though.
One positive reason for increasing the number of parameters is that
that will be a clear API break for any outside callers, if there
are any. If I just replace a bool with an enum, such callers might
or might not get any indication that they need to take a fresh
look.
regards, tom lane
On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
Looks reasonable to me. The added test coverage seems particularly
valuable. If I really wanted to nitpick, I might complain about the three
consecutive Boolean parameters for AlterTypeNamespaceInternal(), which
makes lines like+ AlterTypeNamespaceInternal(arrayOid, nspOid, true, false, true, + objsMoved);difficult to interpret. But that's not necessarily the fault of this patch
and probably needn't block it.I considered merging ignoreDependent and errorOnTableType into a
single 3-valued enum, but didn't think it was worth the trouble
given the very small number of callers; also it wasn't quite clear
how to map that to AlterTypeNamespace_oid's API. Perhaps a little
more thought is appropriate though.One positive reason for increasing the number of parameters is that
that will be a clear API break for any outside callers, if there
are any. If I just replace a bool with an enum, such callers might
or might not get any indication that they need to take a fresh
look.
Agreed. Another option could be to just annotate the arguments with the
parameter names.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com
Nathan Bossart <nathandbossart@gmail.com> writes:
On Wed, May 08, 2024 at 07:42:18PM -0400, Tom Lane wrote:
One positive reason for increasing the number of parameters is that
that will be a clear API break for any outside callers, if there
are any. If I just replace a bool with an enum, such callers might
or might not get any indication that they need to take a fresh
look.
Agreed. Another option could be to just annotate the arguments with the
parameter names.
At the call sites you mean? Sure, I can do that.
regards, tom lane
On Wed, May 08, 2024 at 07:57:55PM -0400, Tom Lane wrote:
Nathan Bossart <nathandbossart@gmail.com> writes:
Agreed. Another option could be to just annotate the arguments with the
parameter names.At the call sites you mean? Sure, I can do that.
Yes.
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com